Bug 175244 – WTF::Function does not allow for reference / non-default constructible return types

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 317472 [details]
Patch

[Sam Weinig]

Created attachment 317474 [details]
Patch

[Chris Dumez]

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.

[Sam Weinig]

(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?

[Chris Dumez]

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.

[Chris Dumez]

Also note that WTF::Function is used a lot in WebKit2 and there is no debug WK2 EWS.

[Sam Weinig]

(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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Sam Weinig]

Created attachment 317534 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Sam Weinig]

Created attachment 317542 [details]
Patch

[Sam Weinig]

Created attachment 317543 [details]
Patch

[Build Bot]

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.

[Sam Weinig]

Created attachment 317583 [details]
Patch

[Build Bot]

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.

[Chris Dumez]

Comment on attachment 317583 [details]
Patch

Let's try.

[WebKit Commit Bot]

Comment on attachment 317583 [details]
Patch

Clearing flags on attachment: 317583

Committed r220457: <http://trac.webkit.org/changeset/220457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33801582>

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r220457 for reason:

This change introduced API test failures.

Committed r220465: <http://trac.webkit.org/changeset/220465>

[Chris Dumez]

ahah API tests. Didn't think about those :)

[Ryan Haddad]

Sam landed a fix after the rollout in https://trac.webkit.org/changeset/220466/webkit, so we need to re-land the original patch.

[Sam Weinig]

Comment on attachment 317583 [details]
Patch

Going to try landing it with the queue again.

[WebKit Commit Bot]

Comment on attachment 317583 [details]
Patch

Clearing flags on attachment: 317583

Committed r220477: <http://trac.webkit.org/changeset/220477>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Ryan Haddad]

I am going to roll out all of the changes.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 175411

[Sam Weinig]

Committed r220601: <http://trac.webkit.org/changeset/220601>