Bug 146988 – REGRESSION(r186088): Crash under WebKit::WebPageProxy::didFailLoadForFrame

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

REGRESSION(r186088): Crash under WebKit::WebPageProxy::didFailLoadForFrame 

For WKView apps:
This WebPageProxy methods calls m_loaderClient->didFailLoadWithErrorForFrame(), and inside that callback the embedding app can cause the WKView to go away.
When a WKView goes away, it's PageClientImpl gets destroyed also.
But then we call into m_pageClient, which was just destroyed...

So we'll add a refView and derefView method to PageClient, add a RefPtr-style object to manage those, and use it in any method where a loaderClient method is called but is not the last thing to be called in the method.

[Brady Eidson]

Created attachment 256878 [details]
Patch v1

[Brady Eidson]

Created attachment 256879 [details]
Patch v2 - Alternate approach came up with Tim while reviewing

[Tim Horton]

Comment on attachment 256878 [details]
Patch v1

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

Plz fix iOS.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:277
> +class PageClientViewRefPtr {

Protector

[Brady Eidson]

Created attachment 256880 [details]
Patch v3

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

Attachment 256880 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:744:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:746:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:751:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:753:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:189:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

For GTK the implementation of those functions would be:

void PageClientImpl::refView()
{
  g_object_ref(m_viewWidget);
}

void PageClientImpl::derefView()
{
  g_object_unref(m_viewWidget);
}

For EFL, I will CC Gyuyoung, but the EFL bot will be happy if they're implemented in WebViewEfl.

[Brady Eidson]

https://trac.webkit.org/changeset/186887

[Anders Carlsson]

Comment on attachment 256880 [details]
Patch v3

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

> Source/WebKit2/UIProcess/mac/PageClientImpl.mm:848
> +void PageClientImpl::refView()
> +{
> +    [m_wkView retain];
> +}
> +
> +void PageClientImpl::derefView()
> +{
> +    [m_wkView release];
> +}
> +

This is wrong. It should use CFRetain/CFRelease or it will break under GC.

[Brady Eidson]

(In reply to comment #9)
> Comment on attachment 256880 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256880&action=review
> 
> > Source/WebKit2/UIProcess/mac/PageClientImpl.mm:848
> > +void PageClientImpl::refView()
> > +{
> > +    [m_wkView retain];
> > +}
> > +
> > +void PageClientImpl::derefView()
> > +{
> > +    [m_wkView release];
> > +}
> > +
> 
> This is wrong. It should use CFRetain/CFRelease or it will break under GC.

Fixed in https://trac.webkit.org/changeset/186897