WTF::Function does not allow for reference / non-default constructible return types. This is an unfortunate limitation as it means you can't return a Ref<> from a WTF::Function. The reason WTF::Function does not allow this is that it ensures that function that has been set with a nullptr can still be called, and will return the default constructed value of the return value. I am unclear when this useful.
Created attachment 317472 [details] Patch
Created attachment 317474 [details] Patch
Comment on attachment 317474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317474&action=review > Source/WTF/wtf/Function.h:55 > + ASSERT(m_callableWrapper); Aren't there places where we call the WTF::Function without checking if it wraps something? I believe we made this assumption is some places.
(In reply to Chris Dumez from comment #3) > Comment on attachment 317474 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317474&action=review > > > Source/WTF/wtf/Function.h:55 > > + ASSERT(m_callableWrapper); > > Aren't there places where we call the WTF::Function without checking if it > wraps something? I believe we made this assumption is some places. I will find out soon from the tests. Do you remember in which places that assumption was made? Or why?
Comment on attachment 317474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317474&action=review >>> Source/WTF/wtf/Function.h:55 >>> + ASSERT(m_callableWrapper); >> >> Aren't there places where we call the WTF::Function without checking if it wraps something? I believe we made this assumption is some places. > > I will find out soon from the tests. > > Do you remember in which places that assumption was made? Or why? When I reviewed people's patches, I said, you don't need this "if" check here as calling an unassigned WTF::Function is safe :/ I think there is some of this in the ResourceLoadStatistics code.
Also note that WTF::Function is used a lot in WebKit2 and there is no debug WK2 EWS.
(In reply to Chris Dumez from comment #6) > Also note that WTF::Function is used a lot in WebKit2 and there is no debug > WK2 EWS. Exciting.
Comment on attachment 317474 [details] Patch Attachment 317474 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4272649 Number of test failures exceeded the failure limit.
Created attachment 317502 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317474 [details] Patch Attachment 317474 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4272792 Number of test failures exceeded the failure limit.
Created attachment 317510 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 317474 [details] Patch Attachment 317474 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4272796 Number of test failures exceeded the failure limit.
Created attachment 317512 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317474 [details] Patch Attachment 317474 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4272962 Number of test failures exceeded the failure limit.
Created attachment 317522 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 317534 [details] Patch
Comment on attachment 317534 [details] Patch Attachment 317534 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4274551 Number of test failures exceeded the failure limit.
Created attachment 317541 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 317542 [details] Patch
Created attachment 317543 [details] Patch
Attachment 317543 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317583 [details] Patch
Attachment 317583 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317583 [details] Patch Let's try.
Comment on attachment 317583 [details] Patch Clearing flags on attachment: 317583 Committed r220457: <http://trac.webkit.org/changeset/220457>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33801582>
(In reply to WebKit Commit Bot from comment #25) > Comment on attachment 317583 [details] > Patch > > Clearing flags on attachment: 317583 > > Committed r220457: <http://trac.webkit.org/changeset/220457> This change introduced API test failures: Tests that failed: WTF_Function.AssignBeforeDestroy WTF_Function.AssignBeforeDestroy WTF_Function.Basics WTF_Function.Basics https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/builds/3840
Reverted r220457 for reason: This change introduced API test failures. Committed r220465: <http://trac.webkit.org/changeset/220465>
ahah API tests. Didn't think about those :)
Sam landed a fix after the rollout in https://trac.webkit.org/changeset/220466/webkit, so we need to re-land the original patch.
Comment on attachment 317583 [details] Patch Going to try landing it with the queue again.
Comment on attachment 317583 [details] Patch Clearing flags on attachment: 317583 Committed r220477: <http://trac.webkit.org/changeset/220477>
Another follow up was landed in https://trac.webkit.org/changeset/220487, but WTF_Function.Basics is still failing / crashing https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/3783 Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 TestWTF 0x00000001050f56ef TestWebKitAPI::WTF_Function_Basics_Test::TestBody() + 701 (Function.cpp:156) 1 TestWTF 0x0000000105273428 testing::Test::Run() + 92 2 TestWTF 0x0000000105273c00 testing::internal::TestInfoImpl::Run() + 178 3 TestWTF 0x0000000105273ff4 testing::TestCase::Run() + 188 4 TestWTF 0x00000001052774d9 testing::internal::UnitTestImpl::RunAllTests() + 583 5 TestWTF 0x00000001051e5421 TestWebKitAPI::TestsController::run(int, char**) + 131 (TestsController.cpp:84) 6 TestWTF 0x000000010526b5fd main + 344 (mainMac.mm:52) 7 libdyld.dylib 0x00007fffacb88235 start + 1
I am going to roll out all of the changes.
Re-opened since this is blocked by bug 175411
Committed r220601: <http://trac.webkit.org/changeset/220601>