Diff
Modified: trunk/Source/WTF/ChangeLog (201432 => 201433)
--- trunk/Source/WTF/ChangeLog 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Source/WTF/ChangeLog 2016-05-26 21:58:42 UTC (rev 201433)
@@ -1,3 +1,41 @@
+2016-05-26 Filip Pizlo <[email protected]>
+
+ 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 <[email protected]>
Get rid of WTF/Functional.h
Modified: trunk/Source/WTF/wtf/ParkingLot.cpp (201432 => 201433)
--- trunk/Source/WTF/wtf/ParkingLot.cpp 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Source/WTF/wtf/ParkingLot.cpp 2016-05-26 21:58:42 UTC (rev 201433)
@@ -532,8 +532,8 @@
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 @@
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"));
Modified: trunk/Source/WTF/wtf/ParkingLot.h (201432 => 201433)
--- trunk/Source/WTF/wtf/ParkingLot.h 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Source/WTF/wtf/ParkingLot.h 2016-05-26 21:58:42 UTC (rev 201433)
@@ -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 @@
{
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 @@
// 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 @@
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*)>&);
};
Modified: trunk/Source/WTF/wtf/ScopedLambda.h (201432 => 201433)
--- trunk/Source/WTF/wtf/ScopedLambda.h 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Source/WTF/wtf/ScopedLambda.h 2016-05-26 21:58:42 UTC (rev 201433)
@@ -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,7 +71,33 @@
, 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 @@
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
Modified: trunk/Tools/ChangeLog (201432 => 201433)
--- trunk/Tools/ChangeLog 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Tools/ChangeLog 2016-05-26 21:58:42 UTC (rev 201433)
@@ -1,3 +1,17 @@
+2016-05-26 Filip Pizlo <[email protected]>
+
+ 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 <[email protected]>
Build fix
Modified: trunk/Tools/TestWebKitAPI/CMakeLists.txt (201432 => 201433)
--- trunk/Tools/TestWebKitAPI/CMakeLists.txt 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Tools/TestWebKitAPI/CMakeLists.txt 2016-05-26 21:58:42 UTC (rev 201433)
@@ -67,6 +67,7 @@
${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
Modified: trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (201432 => 201433)
--- trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2016-05-26 21:57:58 UTC (rev 201432)
+++ trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2016-05-26 21:58:42 UTC (rev 201433)
@@ -369,6 +369,7 @@
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 */; };
@@ -906,6 +907,7 @@
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>"; };
@@ -1372,6 +1374,7 @@
93A427A8180D9B0700CD24D7 /* RefPtr.cpp */,
E4C9ABC71B3DB1710040A987 /* RunLoop.cpp */,
14F3B11215E45EAB00210069 /* SaturatedArithmeticOperations.cpp */,
+ DC69AA621CF77C6500C6272F /* ScopedLambda.cpp */,
CD5393C91757BAC400C07123 /* SHA1.cpp */,
81B50192140F232300D9EB58 /* StringBuilder.cpp */,
93ABA80816DDAB91002DB2FA /* StringHasher.cpp */,
@@ -1905,6 +1908,7 @@
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 */,
Added: trunk/Tools/TestWebKitAPI/Tests/WTF/ScopedLambda.cpp (0 => 201433)
--- trunk/Tools/TestWebKitAPI/Tests/WTF/ScopedLambda.cpp (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/ScopedLambda.cpp 2016-05-26 21:58:42 UTC (rev 201433)
@@ -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
+
