Bug 121910 – [WK2] Crash at at com.apple.WebKit2: WebKit::VoidCallback::invalidate + 46

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[WK2] Crash at at com.apple.WebKit2: WebKit::VoidCallback::invalidate + 46

[Jer Noble]

Created attachment 212585 [details]
Patch

[Jer Noble]

<rdar://problem/13389393>

[Build Bot]

Comment on attachment 212585 [details]
Patch

Attachment 212585 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2181004

[Build Bot]

Comment on attachment 212585 [details]
Patch

Attachment 212585 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2166250

[Jer Noble]

Created attachment 212599 [details]
Patch

[Build Bot]

Comment on attachment 212599 [details]
Patch

Attachment 212599 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2266057

[Build Bot]

Comment on attachment 212599 [details]
Patch

Attachment 212599 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2281054

[Jer Noble]

Created attachment 212635 [details]
Patch

[Darin Adler]

Comment on attachment 212635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212635&action=review

I would say review+ except for the confusing assertions.

> Source/WebKit2/ChangeLog:10
> +        Store a copy of the VoidCallback passed to WKPage, and cancel the callback
> +        during dealloc by changing it's context to 0. This requires a small change
> +        to CallbackBase to add the ability to change the context post-creation.

You changed the patch to use invalidate, so the comment about CallbackBase is no longer correct.

> Source/WebKit2/ChangeLog:13
> +        * UIProcess/GenericCallback.h:
> +        (WebKit::CallbackBase::setContext): Added; simple setter.

This change is not in the patch.

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:115
> +    ASSERT(!_repaintCallback);

This is really confusing. How does calling invalidate on the callback cause it to become a nullptr?

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:409
> +    ASSERT(!_repaintCallback);

This is really confusing. How does calling invalidate on the callback cause it to become a nullptr?

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:416
> +    _repaintCallback = 0;

We like nullptr for this in newer code.

[Jer Noble]

(In reply to comment #9)
> (From update of attachment 212635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212635&action=review
> 
> I would say review+ except for the confusing assertions.
> 
> > Source/WebKit2/ChangeLog:10
> > +        Store a copy of the VoidCallback passed to WKPage, and cancel the callback
> > +        during dealloc by changing it's context to 0. This requires a small change
> > +        to CallbackBase to add the ability to change the context post-creation.
> 
> You changed the patch to use invalidate, so the comment about CallbackBase is no longer correct.

Whoops; i'll update the ChangeLog.

> > Source/WebKit2/ChangeLog:13
> > +        * UIProcess/GenericCallback.h:
> > +        (WebKit::CallbackBase::setContext): Added; simple setter.
> 
> This change is not in the patch.

And here as well.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:115
> > +    ASSERT(!_repaintCallback);
> 
> This is really confusing. How does calling invalidate on the callback cause it to become a nullptr?

When you call VoidCallback::invalidate(), it immediately calls the callback method with an error parameter.  The callback method will clear the _repaintCallback.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:409
> > +    ASSERT(!_repaintCallback);
> 
> This is really confusing. How does calling invalidate on the callback cause it to become a nullptr?

Ditto.

> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:416
> > +    _repaintCallback = 0;
> 
> We like nullptr for this in newer code.

Will change.

[Jer Noble]

Created attachment 212667 [details]
Patch

[Jer Noble]

Committed r156479: <http://trac.webkit.org/changeset/156479>