[WK2] Crash at at com.apple.WebKit2: WebKit::VoidCallback::invalidate + 46
Created attachment 212585 [details] Patch
<rdar://problem/13389393>
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
Comment on attachment 212585 [details] Patch Attachment 212585 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2166250
Created attachment 212599 [details] Patch
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
Comment on attachment 212599 [details] Patch Attachment 212599 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2281054
Created attachment 212635 [details] Patch
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.
(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.
Created attachment 212667 [details] Patch
Committed r156479: <http://trac.webkit.org/changeset/156479>