Bug 175244 - WTF::Function does not allow for reference / non-default constructible return types
Summary: WTF::Function does not allow for reference / non-default constructible return...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on: 175411 175685
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-06 08:45 PDT by Sam Weinig
Modified: 2017-08-17 13:32 PDT (History)
15 users (show)

See Also:


Attachments
Patch (1.99 KB, patch)
2017-08-07 14:44 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2017-08-07 15:08 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.28 MB, application/zip)
2017-08-07 16:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (640.27 KB, application/zip)
2017-08-07 16:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (353.02 KB, application/zip)
2017-08-07 17:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (669.90 KB, application/zip)
2017-08-07 17:34 PDT, Build Bot
no flags Details
Patch (2.47 KB, patch)
2017-08-07 20:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.43 MB, application/zip)
2017-08-07 21:59 PDT, Build Bot
no flags Details
Patch (3.33 KB, patch)
2017-08-07 22:04 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2017-08-07 22:09 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (7.28 KB, patch)
2017-08-08 10:50 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-08-06 08:45:43 PDT
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.
Comment 1 Sam Weinig 2017-08-07 14:44:10 PDT
Created attachment 317472 [details]
Patch
Comment 2 Sam Weinig 2017-08-07 15:08:51 PDT
Created attachment 317474 [details]
Patch
Comment 3 Chris Dumez 2017-08-07 15:11:00 PDT
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.
Comment 4 Sam Weinig 2017-08-07 15:39:57 PDT
(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 5 Chris Dumez 2017-08-07 15:45:37 PDT
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.
Comment 6 Chris Dumez 2017-08-07 15:55:44 PDT
Also note that WTF::Function is used a lot in WebKit2 and there is no debug WK2 EWS.
Comment 7 Sam Weinig 2017-08-07 16:04:56 PDT
(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 8 Build Bot 2017-08-07 16:44:53 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-08-07 16:44:54 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-08-07 16:57:41 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-08-07 16:57:43 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-08-07 17:01:39 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-08-07 17:01:40 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-08-07 17:34:31 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-08-07 17:34:33 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2017-08-07 20:18:56 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-08-07 21:59:35 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-08-07 21:59:37 PDT Comment hidden (obsolete)
Comment 19 Sam Weinig 2017-08-07 22:04:47 PDT Comment hidden (obsolete)
Comment 20 Sam Weinig 2017-08-07 22:09:25 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2017-08-07 22:12:09 PDT Comment hidden (obsolete)
Comment 22 Sam Weinig 2017-08-08 10:50:15 PDT
Created attachment 317583 [details]
Patch
Comment 23 Build Bot 2017-08-08 10:52:46 PDT
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 24 Chris Dumez 2017-08-08 20:32:29 PDT
Comment on attachment 317583 [details]
Patch

Let's try.
Comment 25 WebKit Commit Bot 2017-08-09 08:59:05 PDT
Comment on attachment 317583 [details]
Patch

Clearing flags on attachment: 317583

Committed r220457: <http://trac.webkit.org/changeset/220457>
Comment 26 WebKit Commit Bot 2017-08-09 08:59:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2017-08-09 08:59:50 PDT
<rdar://problem/33801582>
Comment 28 Ryan Haddad 2017-08-09 10:52:46 PDT
(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
Comment 29 Ryan Haddad 2017-08-09 10:58:18 PDT
Reverted r220457 for reason:

This change introduced API test failures.

Committed r220465: <http://trac.webkit.org/changeset/220465>
Comment 30 Chris Dumez 2017-08-09 10:59:19 PDT
ahah API tests. Didn't think about those :)
Comment 31 Ryan Haddad 2017-08-09 11:20:29 PDT
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 32 Sam Weinig 2017-08-09 13:28:18 PDT
Comment on attachment 317583 [details]
Patch

Going to try landing it with the queue again.
Comment 33 WebKit Commit Bot 2017-08-09 13:57:55 PDT
Comment on attachment 317583 [details]
Patch

Clearing flags on attachment: 317583

Committed r220477: <http://trac.webkit.org/changeset/220477>
Comment 34 WebKit Commit Bot 2017-08-09 13:57:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Ryan Haddad 2017-08-09 17:28:10 PDT
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
Comment 36 Ryan Haddad 2017-08-09 17:33:31 PDT
I am going to roll out all of the changes.
Comment 37 WebKit Commit Bot 2017-08-09 17:35:09 PDT
Re-opened since this is blocked by bug 175411
Comment 38 Sam Weinig 2017-08-11 09:56:39 PDT
Committed r220601: <http://trac.webkit.org/changeset/220601>