ScopedLambda should have a lifetime story that makes sense to the compiler
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 May 2016 21:58:42 +0000 (21:58 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 May 2016 21:58:42 +0000 (21:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158118

Reviewed by Mark Lam.
Source/WTF:

Prior to this change, there were two lifetime bugs in ScopedLambda:

- scopedLambda(Functor&&) would bind Functor to const lambda&, so the resulting ScopedLambdaFunctor
  would hold a reference to the original lambda. This would have surprising behavior; for example
  it meant that this code was wrong:

  auto l = scopedLambda<things>([&] ...);

  The solution is to have explicit copy/move versions of scopedLambda() rather than rely on perfect
  forwarding.

- ScopedLambdaFunctor did not override its copy or move operations, so if the compiler did not RVO
  scopedLambda(), it would return a ScopedLambdaFunctor whose m_arg points to a dead temporary
  ScopedLambdaFunctor instance. The solution is to have explicit copy/move constructors and
  operators, which preserve the invariant that ScopedLambda::m_arg points to this.

One nice side-effect of all of these constructors and operators being explicit is that we can rely
on WTFMove's excellent assertions, which helped catch the first issue.

This reverts ParkingLot to use ScopedLambda again.

* wtf/ParkingLot.cpp:
(WTF::ParkingLot::parkConditionallyImpl):
(WTF::ParkingLot::unparkOne):
(WTF::ParkingLot::unparkOneImpl):
* wtf/ParkingLot.h:
(WTF::ParkingLot::parkConditionally):
(WTF::ParkingLot::unparkOne):
* wtf/ScopedLambda.h:
(WTF::scopedLambda):

Tools:

Added a test case. This test crashes before the fix and now it passes.

* TestWebKitAPI/CMakeLists.txt:
* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WTF/ScopedLambda.cpp: Added.
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@201433 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/ParkingLot.cpp
Source/WTF/wtf/ParkingLot.h
Source/WTF/wtf/ScopedLambda.h
Tools/ChangeLog
Tools/TestWebKitAPI/CMakeLists.txt
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WTF/ScopedLambda.cpp [new file with mode: 0644]

index db7f712..55c9cf0 100644 (file)
@@ -1,3 +1,41 @@
+2016-05-26  Filip Pizlo  <fpizlo@apple.com>
+
+        ScopedLambda should have a lifetime story that makes sense to the compiler
+        https://bugs.webkit.org/show_bug.cgi?id=158118
+
+        Reviewed by Mark Lam.
+
+        Prior to this change, there were two lifetime bugs in ScopedLambda:
+        
+        - scopedLambda(Functor&&) would bind Functor to const lambda&, so the resulting ScopedLambdaFunctor
+          would hold a reference to the original lambda. This would have surprising behavior; for example
+          it meant that this code was wrong:
+          
+          auto l = scopedLambda<things>([&] ...);
+          
+          The solution is to have explicit copy/move versions of scopedLambda() rather than rely on perfect
+          forwarding.
+        
+        - ScopedLambdaFunctor did not override its copy or move operations, so if the compiler did not RVO
+          scopedLambda(), it would return a ScopedLambdaFunctor whose m_arg points to a dead temporary
+          ScopedLambdaFunctor instance. The solution is to have explicit copy/move constructors and
+          operators, which preserve the invariant that ScopedLambda::m_arg points to this.
+        
+        One nice side-effect of all of these constructors and operators being explicit is that we can rely
+        on WTFMove's excellent assertions, which helped catch the first issue.
+        
+        This reverts ParkingLot to use ScopedLambda again.
+
+        * wtf/ParkingLot.cpp:
+        (WTF::ParkingLot::parkConditionallyImpl):
+        (WTF::ParkingLot::unparkOne):
+        (WTF::ParkingLot::unparkOneImpl):
+        * wtf/ParkingLot.h:
+        (WTF::ParkingLot::parkConditionally):
+        (WTF::ParkingLot::unparkOne):
+        * wtf/ScopedLambda.h:
+        (WTF::scopedLambda):
+
 2016-05-25  Anders Carlsson  <andersca@apple.com>
 
         Get rid of WTF/Functional.h
index 9a67ac2..60962e0 100644 (file)
@@ -532,8 +532,8 @@ bool dequeue(
 
 NEVER_INLINE bool ParkingLot::parkConditionallyImpl(
     const void* address,
-    std::function<bool()> validation,
-    std::function<void()> beforeSleep,
+    const ScopedLambda<bool()>& validation,
+    const ScopedLambda<void()>& beforeSleep,
     Clock::time_point timeout)
 {
     if (verbose)
@@ -649,9 +649,9 @@ NEVER_INLINE ParkingLot::UnparkResult ParkingLot::unparkOne(const void* address)
     return result;
 }
 
-NEVER_INLINE void ParkingLot::unparkOne(
+NEVER_INLINE void ParkingLot::unparkOneImpl(
     const void* address,
-    std::function<void(ParkingLot::UnparkResult)> callback)
+    const ScopedLambda<void(ParkingLot::UnparkResult)>& callback)
 {
     if (verbose)
         dataLog(toString(currentThread(), ": unparking one the hard way.\n"));
index bfe54f6..337aa95 100644 (file)
@@ -29,6 +29,7 @@
 #include <chrono>
 #include <functional>
 #include <wtf/Atomics.h>
+#include <wtf/ScopedLambda.h>
 #include <wtf/Threading.h>
 
 namespace WTF {
@@ -60,8 +61,8 @@ public:
     {
         return parkConditionallyImpl(
             address,
-            std::function<bool()>(std::forward<ValidationFunctor>(validation)),
-            std::function<void()>(std::forward<BeforeSleepFunctor>(beforeSleep)),
+            scopedLambda<bool()>(std::forward<ValidationFunctor>(validation)),
+            scopedLambda<void()>(std::forward<BeforeSleepFunctor>(beforeSleep)),
             timeout);
     }
 
@@ -100,9 +101,11 @@ public:
     // that race - see Rusty Russel's well-known usersem library - but it's not pretty. This form
     // allows that race to be completely avoided, since there is no way that a thread can be parked
     // while the callback is running.
-    WTF_EXPORT_PRIVATE static void unparkOne(
-        const void* address,
-        std::function<void(ParkingLot::UnparkResult)> callback);
+    template<typename Callback>
+    static void unparkOne(const void* address, Callback&& callback)
+    {
+        unparkOneImpl(address, scopedLambda<void(UnparkResult)>(std::forward<Callback>(callback)));
+    }
 
     // Unparks every thread from the queue associated with the given address, which cannot be null.
     WTF_EXPORT_PRIVATE static void unparkAll(const void* address);
@@ -125,12 +128,12 @@ public:
 private:
     WTF_EXPORT_PRIVATE static bool parkConditionallyImpl(
         const void* address,
-        std::function<bool()> validation,
-        std::function<void()> beforeSleep,
+        const ScopedLambda<bool()>& validation,
+        const ScopedLambda<void()>& beforeSleep,
         Clock::time_point timeout);
     
     WTF_EXPORT_PRIVATE static void unparkOneImpl(
-        const void* address, std::function<void(UnparkResult)> callback);
+        const void* address, const ScopedLambda<void(UnparkResult)>& callback);
 
     WTF_EXPORT_PRIVATE static void forEachImpl(const std::function<void(ThreadIdentifier, const void*)>&);
 };
index 2d8dfd4..c83a2f1 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -71,6 +71,32 @@ public:
         , m_functor(std::forward<PassedFunctor>(functor))
     {
     }
+    
+    // We need to make sure that copying and moving ScopedLambdaFunctor results in a ScopedLambdaFunctor
+    // whose ScopedLambda supertype still points to this rather than other.
+    ScopedLambdaFunctor(const ScopedLambdaFunctor& other)
+        : ScopedLambda<ResultType (ArgumentTypes...)>(implFunction, this)
+        , m_functor(other.m_functor)
+    {
+    }
+
+    ScopedLambdaFunctor(ScopedLambdaFunctor&& other)
+        : ScopedLambda<ResultType (ArgumentTypes...)>(implFunction, this)
+        , m_functor(WTFMove(other.m_functor))
+    {
+    }
+    
+    ScopedLambdaFunctor& operator=(const ScopedLambdaFunctor& other)
+    {
+        m_functor = other.m_functor;
+        return *this;
+    }
+    
+    ScopedLambdaFunctor& operator=(ScopedLambdaFunctor&& other)
+    {
+        m_functor = WTFMove(other.m_functor);
+        return *this;
+    }
 
 private:
     static ResultType implFunction(void* argument, ArgumentTypes... arguments)
@@ -81,10 +107,23 @@ private:
     Functor m_functor;
 };
 
+// Can't simply rely on perfect forwarding because then the ScopedLambdaFunctor would point to the functor
+// by const reference. This would be surprising in situations like:
+//
+// auto scopedLambda = scopedLambda<Foo(Bar)>([&] (Bar) -> Foo { ... });
+//
+// We expected scopedLambda to be valid for its entire lifetime, but if it computed the lambda by reference
+// then it would be immediately invalid.
+template<typename FunctionType, typename Functor>
+ScopedLambdaFunctor<FunctionType, Functor> scopedLambda(const Functor& functor)
+{
+    return ScopedLambdaFunctor<FunctionType, Functor>(functor);
+}
+
 template<typename FunctionType, typename Functor>
 ScopedLambdaFunctor<FunctionType, Functor> scopedLambda(Functor&& functor)
 {
-    return ScopedLambdaFunctor<FunctionType, Functor>(std::forward<Functor>(functor));
+    return ScopedLambdaFunctor<FunctionType, Functor>(WTFMove(functor));
 }
 
 } // namespace WTF
index aadcd9f..991d910 100644 (file)
@@ -1,3 +1,17 @@
+2016-05-26  Filip Pizlo  <fpizlo@apple.com>
+
+        ScopedLambda should have a lifetime story that makes sense to the compiler
+        https://bugs.webkit.org/show_bug.cgi?id=158118
+
+        Reviewed by Mark Lam.
+        
+        Added a test case. This test crashes before the fix and now it passes.
+
+        * TestWebKitAPI/CMakeLists.txt:
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WTF/ScopedLambda.cpp: Added.
+        (TestWebKitAPI::TEST):
+
 2016-05-26  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Build fix
index 70b2255..11ba5ac 100644 (file)
@@ -67,6 +67,7 @@ set(TestWTF_SOURCES
     ${TESTWEBKITAPI_DIR}/Tests/WTF/RefPtr.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/SHA1.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/SaturatedArithmeticOperations.cpp
+    ${TESTWEBKITAPI_DIR}/Tests/WTF/ScopedLambda.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/StringBuilder.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/StringHasher.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/StringImpl.cpp
index 5211c03..bc36161 100644 (file)
                CE50D8CA1C8665CE0072EA5A /* OptionSet.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CE50D8C81C8665CE0072EA5A /* OptionSet.cpp */; };
                CEA6CF2819CCF69D0064F5A7 /* open-and-close-window.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = CEA6CF2719CCF69D0064F5A7 /* open-and-close-window.html */; };
                CEBABD491B71687C0051210A /* should-open-external-schemes.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = CEBABD481B71687C0051210A /* should-open-external-schemes.html */; };
+               DC69AA641CF77C7D00C6272F /* ScopedLambda.cpp in Sources */ = {isa = PBXBuildFile; fileRef = DC69AA621CF77C6500C6272F /* ScopedLambda.cpp */; settings = {COMPILER_FLAGS = "-fno-elide-constructors"; }; };
                E1220DCA155B28AA0013E2FC /* MemoryCacheDisableWithinResourceLoadDelegate.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = E1220DC9155B287D0013E2FC /* MemoryCacheDisableWithinResourceLoadDelegate.html */; };
                E194E1BD177E53C7009C4D4E /* StopLoadingFromDidReceiveResponse.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = E194E1BC177E534A009C4D4E /* StopLoadingFromDidReceiveResponse.html */; };
                E19DB9791B32137C00DB38D4 /* NavigatorLanguage.mm in Sources */ = {isa = PBXBuildFile; fileRef = E19DB9781B32137C00DB38D4 /* NavigatorLanguage.mm */; };
                CEA6CF2219CCF5BD0064F5A7 /* OpenAndCloseWindow.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = OpenAndCloseWindow.mm; sourceTree = "<group>"; };
                CEA6CF2719CCF69D0064F5A7 /* open-and-close-window.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "open-and-close-window.html"; sourceTree = "<group>"; };
                CEBABD481B71687C0051210A /* should-open-external-schemes.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "should-open-external-schemes.html"; sourceTree = "<group>"; };
+               DC69AA621CF77C6500C6272F /* ScopedLambda.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ScopedLambda.cpp; sourceTree = "<group>"; };
                E1220D9F155B25480013E2FC /* MemoryCacheDisableWithinResourceLoadDelegate.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MemoryCacheDisableWithinResourceLoadDelegate.mm; sourceTree = "<group>"; };
                E1220DC9155B287D0013E2FC /* MemoryCacheDisableWithinResourceLoadDelegate.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = MemoryCacheDisableWithinResourceLoadDelegate.html; sourceTree = "<group>"; };
                E194E1BA177E5145009C4D4E /* StopLoadingFromDidReceiveResponse.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = StopLoadingFromDidReceiveResponse.mm; sourceTree = "<group>"; };
                                93A427A8180D9B0700CD24D7 /* RefPtr.cpp */,
                                E4C9ABC71B3DB1710040A987 /* RunLoop.cpp */,
                                14F3B11215E45EAB00210069 /* SaturatedArithmeticOperations.cpp */,
+                               DC69AA621CF77C6500C6272F /* ScopedLambda.cpp */,
                                CD5393C91757BAC400C07123 /* SHA1.cpp */,
                                81B50192140F232300D9EB58 /* StringBuilder.cpp */,
                                93ABA80816DDAB91002DB2FA /* StringHasher.cpp */,
                                7CCE7F2C1A411B1000447C4C /* PreventImageLoadWithAutoResizing.mm in Sources */,
                                7CCE7F0C1A411AE600447C4C /* PrivateBrowsingPushStateNoHistoryCallback.cpp in Sources */,
                                7CCE7EC81A411A7E00447C4C /* PublicSuffix.mm in Sources */,
+                               DC69AA641CF77C7D00C6272F /* ScopedLambda.cpp in Sources */,
                                7CCE7F3E1A411B8E00447C4C /* RedBlackTree.cpp in Sources */,
                                7CCE7F3F1A411B8E00447C4C /* Ref.cpp in Sources */,
                                7CCE7F401A411B8E00447C4C /* RefCounter.cpp in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WTF/ScopedLambda.cpp b/Tools/TestWebKitAPI/Tests/WTF/ScopedLambda.cpp
new file mode 100644 (file)
index 0000000..3dfee86
--- /dev/null
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2016 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include <wtf/ScopedLambda.h>
+#include <wtf/Vector.h>
+
+using namespace WTF;
+
+namespace TestWebKitAPI {
+
+// This test relies on this module being compiled with -fno-elide-constructors
+TEST(WTF_ScopedLambda, NoRVOLivenessBug)
+{
+    Vector<int> vector;
+    for (unsigned i = 0; i < 10; ++i)
+        vector.append(i);
+    
+    auto lambda = scopedLambda<int(size_t)>(
+        [=] (size_t i) -> int {
+            return vector[i];
+        });
+    
+    for (unsigned i = 0; i < 10; ++i)
+        EXPECT_EQ(i, static_cast<unsigned>(lambda(i)));
+}
+
+} // namespace TestWebKitAPI
+