Bug 201249 – CompletionHandler default constructor does not initialize `m_wasConstructedOnMainThread`

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

[Joseph Pecoraro]

CompletionHandler default constructor does not initialize `m_wasConstructedOnMainThread`

[Joseph Pecoraro]

Debug only member for assertions. I'm seeing assertions!

[Chris Dumez]

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

[Joseph Pecoraro]

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

[Chris Dumez]

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

[Joseph Pecoraro]

> 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!

[Chris Dumez]

Why are we closing this. We should probably still fix the bug where m_wasConstructedOnMainThread is not initialized.

[Chris Dumez]

Will fix.

[Joseph Pecoraro]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 377511 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 377511 [details]
Patch

r=me

[Joseph Pecoraro]

> Right, but still seems wrong to have an uninitialized data member.

Sounds good!

[Alex Christensen]

Comment on attachment 377511 [details]
Patch

Why not just put this in an initializer list and keep the =default?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 377587 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377587 [details]
Patch

Clearing flags on attachment: 377587

Committed r249280: <https://trac.webkit.org/changeset/249280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54841787>