Bug 201249 - CompletionHandler default constructor does not initialize `m_wasConstructedOnMainThread`
Summary: CompletionHandler default constructor does not initialize `m_wasConstructedOn...
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: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-28 16:26 PDT by Joseph Pecoraro
Modified: 2019-08-29 10:12 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.46 KB, patch)
2019-08-28 16:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (1.56 KB, patch)
2019-08-29 08:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-08-28 16:26:12 PDT
CompletionHandler default constructor does not initialize `m_wasConstructedOnMainThread`
Comment 1 Joseph Pecoraro 2019-08-28 16:28:43 PDT
Debug only member for assertions. I'm seeing assertions!
Comment 2 Chris Dumez 2019-08-28 16:31:06 PDT
(In reply to Joseph Pecoraro from comment #1)
> Debug only member for assertions. I'm seeing assertions!

Oh, I guess if somebody default-constructs a CompletionHandler and then later on assigns a lambda to it, and then calls the completion handler on the wrong thread. You found cases like this?
Comment 3 Joseph Pecoraro 2019-08-28 16:39:56 PDT
(In reply to Chris Dumez from comment #2)
> (In reply to Joseph Pecoraro from comment #1)
> > Debug only member for assertions. I'm seeing assertions!
> 
> Oh, I guess if somebody default-constructs a CompletionHandler and then
> later on assigns a lambda to it, and then calls the completion handler on
> the wrong thread. You found cases like this?

I'm adding code that is essentially like:

    CompletionHandler<void> handler;
    callOnMainThread([handler = WTFMove(handler)] mutable {
        handler();
    });

And it is asserting.

If I instead did:

    CompletionHandler<void> handler = []{};
    callOnMainThread([handler = WTFMove(handler)] mutable {
        handler();
    });

it is not asserting.
Comment 4 Chris Dumez 2019-08-28 16:41:09 PDT
(In reply to Joseph Pecoraro from comment #3)
> (In reply to Chris Dumez from comment #2)
> > (In reply to Joseph Pecoraro from comment #1)
> > > Debug only member for assertions. I'm seeing assertions!
> > 
> > Oh, I guess if somebody default-constructs a CompletionHandler and then
> > later on assigns a lambda to it, and then calls the completion handler on
> > the wrong thread. You found cases like this?
> 
> I'm adding code that is essentially like:
> 
>     CompletionHandler<void> handler;
>     callOnMainThread([handler = WTFMove(handler)] mutable {
>         handler();
>     });
> 
> And it is asserting.
> 
> If I instead did:
> 
>     CompletionHandler<void> handler = []{};
>     callOnMainThread([handler = WTFMove(handler)] mutable {
>         handler();
>     });
> 
> it is not asserting.

Yes, you're not allowed to call a default constructed Function or CompletionHandler, it is by design and has nothing to do with threading.
Comment 5 Joseph Pecoraro 2019-08-28 16:46:26 PDT
> Yes, you're not allowed to call a default constructed Function or
> CompletionHandler, it is by design and has nothing to do with threading.

Okay, sounds good. I can check if the completion handler was initialized (operator bool) in these cases. Note that `CompletionHandlerCallingScope` does just that.

Thanks!
Comment 6 Chris Dumez 2019-08-28 16:47:39 PDT
Why are we closing this. We should probably still fix the bug where m_wasConstructedOnMainThread is not initialized.
Comment 7 Chris Dumez 2019-08-28 16:47:57 PDT
Will fix.
Comment 8 Joseph Pecoraro 2019-08-28 16:49:11 PDT
Fair enough, but if we fix this won't it just assert next that the function is not callable next. So some assertion will happen.
Comment 9 Chris Dumez 2019-08-28 16:50:30 PDT
(In reply to Joseph Pecoraro from comment #8)
> Fair enough, but if we fix this won't it just assert next that the function
> is not callable next. So some assertion will happen.

Right, but still seems wrong to have an uninitialized data member.
Comment 10 Chris Dumez 2019-08-28 16:51:06 PDT
Created attachment 377511 [details]
Patch
Comment 11 Joseph Pecoraro 2019-08-28 16:52:02 PDT
Comment on attachment 377511 [details]
Patch

r=me
Comment 12 Joseph Pecoraro 2019-08-28 16:52:32 PDT
> Right, but still seems wrong to have an uninitialized data member.

Sounds good!
Comment 13 Alex Christensen 2019-08-28 16:54:09 PDT
Comment on attachment 377511 [details]
Patch

Why not just put this in an initializer list and keep the =default?
Comment 14 Chris Dumez 2019-08-28 16:56:53 PDT
(In reply to Alex Christensen from comment #13)
> Comment on attachment 377511 [details]
> Patch
> 
> Why not just put this in an initializer list and keep the =default?

Oh, I can try this.
Comment 15 Chris Dumez 2019-08-29 08:24:06 PDT
Created attachment 377587 [details]
Patch
Comment 16 WebKit Commit Bot 2019-08-29 10:11:12 PDT
Comment on attachment 377587 [details]
Patch

Clearing flags on attachment: 377587

Committed r249280: <https://trac.webkit.org/changeset/249280>
Comment 17 WebKit Commit Bot 2019-08-29 10:11:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-08-29 10:12:27 PDT
<rdar://problem/54841787>