Bug 95949 – [GTK] [WK2] Crash when navigating between pages

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sudarsana Nagineni (babu)]

Observed a crash in webkitURIResponseSetCertificateInfo when navigating between web pages.

#0  0x00007fbb166b0557 in webkitURIResponseSetCertificateInfo (response=0x0, wkCertificate=0xb6fd80) at ../../Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:288
288	    response->priv->resourceResponse.setSoupMessageCertificate(certificateInfo.certificate());
(gdb) bt
#0  0x00007fbb166b0557 in webkitURIResponseSetCertificateInfo (response=0x0, wkCertificate=0xb6fd80) at ../../Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:288
#1  0x00007fbb166be5c0 in setCertificateToMainResource (webView=0x974200) at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1108
#2  0x00007fbb166be78e in webkitWebViewLoadChanged (webView=0x974200, loadEvent=WEBKIT_LOAD_COMMITTED) at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1152
#3  0x00007fbb166a021d in didCommitLoadForFrame (page=0x9a13b0, frame=0xafe830, userData=0x0, clientInfo=0x974200) at ../../Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:68
#4  0x00007fbb16732a01 in WebKit::WebLoaderClient::didCommitLoadForFrame (this=0x9a13d0, page=0x9a13b0, frame=0xafe830, userData=0x0) at ../../Source/WebKit2/UIProcess/WebLoaderClient.cpp:72
#5  0x00007fbb16740770 in WebKit::WebPageProxy::didCommitLoadForFrame (this=0x9a13b0, frameID=1, mimeType="text/html", frameHasCustomRepresentation=false, certificateInfo=..., arguments=0x7fbaa4001250)
    at ../../Source/WebKit2/UIProcess/WebPageProxy.cpp:2029
#6  0x00007fbb1687a33c in CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long, WTF::String const&, bool, WebKit::PlatformCertificateInfo const&, CoreIPC::ArgumentDecoder*), unsigned long, WTF::String, bool, WebKit::PlatformCertificateInfo> (args=..., argumentDecoder=0x7fbaa4001250, object=0x9a13b0, function=
    (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const, unsigned long, const WTF::String &, bool, const WebKit::PlatformCertificateInfo &, CoreIPC::ArgumentDecoder *)) 0x7fbb167405b4 <WebKit::WebPageProxy::didCommitLoadForFrame(unsigned long, WTF::String const&, bool, WebKit::PlatformCertificateInfo const&, CoreIPC::ArgumentDecoder*)>) at ../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:253
#7  0x00007fbb168765e0 in CoreIPC::handleMessageVariadic<Messages::WebPageProxy::DidCommitLoadForFrame, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long, WTF::String const&, bool, WebKit::PlatformCertificateInfo const&, CoreIPC::ArgumentDecoder*)> (argumentDecoder=0x7fbaa4001250, object=0x9a13b0, function=
    (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const, unsigned long, const WTF::String &, bool, const WebKit::PlatformCertificateInfo &, CoreIPC::ArgumentDecoder *)) 0x7fbb167405b4 <WebKit::WebPageProxy::didCommitLoadForFrame(unsigned long, WTF::String const&, bool, WebKit::PlatformCertificateInfo const&, CoreIPC::ArgumentDecoder*)>) at ../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:332
#8  0x00007fbb168738dc in WebKit::WebPageProxy::didReceiveWebPageProxyMessage (this=0x9a13b0, messageID=..., arguments=0x7fbaa4001250) at DerivedSources/WebKit2/WebPageProxyMessageReceiver.cpp:271
#9  0x00007fbb1673f221 in WebKit::WebPageProxy::didReceiveMessage (this=0x9a13b0, connection=0xa6dba0, messageID=..., arguments=0x7fbaa4001250) at ../../Source/WebKit2/UIProcess/WebPageProxy.cpp:1788
#10 0x00007fbb16772c20 in WebKit::WebProcessProxy::didReceiveMessage (this=0x9964f0, connection=0xa6dba0, messageID=..., arguments=0x7fbaa4001250) at ../../Source/WebKit2/UIProcess/WebProcessProxy.cpp:430
#11 0x00007fbb1670916c in WebKit::WebConnectionToWebProcess::didReceiveMessage (this=0xa6db50, connection=0xa6dba0, messageID=..., arguments=0x7fbaa4001250)
    at ../../Source/WebKit2/UIProcess/WebConnectionToWebProcess.cpp:92
#12 0x00007fbb1661588b in CoreIPC::Connection::dispatchMessage (this=0xa6dba0, message=...) at ../../Source/WebKit2/Platform/CoreIPC/Connection.cpp:691
#13 0x00007fbb16615a29 in CoreIPC::Connection::dispatchOneMessage (this=0xa6dba0) at ../../Source/WebKit2/Platform/CoreIPC/Connection.cpp:717
#14 0x00007fbb1661fbe6 in WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator() (this=0x7fbaa40012c0, c=0xa6dba0) at ../../Source/WTF/wtf/Functional.h:174
#15 0x00007fbb1661f9ec in WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()() (this=0x7fbaa40012b0)
    at ../../Source/WTF/wtf/Functional.h:406
#16 0x00007fbb16624a2e in WTF::Function<void ()>::operator()() const (this=0x7fffa9008840) at ../../Source/WTF/wtf/Functional.h:614
#17 0x00007fbb171f7711 in WebCore::RunLoop::performWork (this=0x965e40) at ../../Source/WebCore/platform/RunLoop.cpp:87
#18 0x00007fbb17c694ae in WebCore::RunLoop::queueWork (runLoop=0x965e40) at ../../Source/WebCore/platform/gtk/RunLoopGtk.cpp:102
#19 0x00007fbb151f1c9a in g_main_dispatch (context=0x86d440) at /build/buildd/glib2.0-2.32.1/./glib/gmain.c:2515
#20 g_main_context_dispatch (context=0x86d440) at /build/buildd/glib2.0-2.32.1/./glib/gmain.c:3052
#21 0x00007fbb151f2060 in g_main_context_iterate (dispatch=1, block=<optimized out>, context=0x86d440, self=<optimized out>) at /build/buildd/glib2.0-2.32.1/./glib/gmain.c:3123
#22 g_main_context_iterate (context=0x86d440, block=<optimized out>, dispatch=1, self=<optimized out>) at /build/buildd/glib2.0-2.32.1/./glib/gmain.c:3060
#23 0x00007fbb151f245a in g_main_loop_run (loop=0xa93740) at /build/buildd/glib2.0-2.32.1/./glib/gmain.c:3317
#24 0x00007fbb15bb725d in gtk_main () at /build/buildd/gtk+3.0-3.4.1/./gtk/gtkmain.c:1165
#25 0x000000000040b650 in main (argc=1, argv=0x7fffa9008ae8) at ../../Tools/MiniBrowser/gtk/main.c:233
(gdb) p response
$1 = (WebKitURIResponse *) 0x0
(gdb)

[Sudarsana Nagineni (babu)]

Created attachment 162461 [details]
Patch

[WebKit Review 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

[Carlos Garcia Campos]

Is it possible that when the load has been commited and we have a main resource, the resource hasn't received a response yet? Is the crash easily reproducible or does it randomly happens?

[Sudarsana Nagineni (babu)]

(In reply to comment #3)
> Is it possible that when the load has been commited and we have a main resource, the resource hasn't received a response yet?
By looking at the stack trace and code, I can confirm that the load has been committed and we have a main resource, but the response is still missing.

>Is the crash easily reproducible or does it randomly happens?
yes, it is reproducible, but not always. 

Here are the steps:
1. Open MiniBrowser and load http://www.youtube.com
2. Click on any video link.
3. Navigate back/forward several times while pages are loading.

[Carlos Garcia Campos]

Created attachment 162478 [details]
A different patch

I've checked the problem and it's related to bug https://bugs.webkit.org/show_bug.cgi?id=91478. For pages loaded from the history cache, the parameter isMainResource is always true, so all subresources are considered the main one. When the loca committed signal is emitted, the mainResource variable doesn't point to the main resource, but to the last subresources that is being loaded in this moment, and it can happen that the response hasn't been received yet for such subresource. So, we can simply assume that the main resource of the main frame is the main resource of the web view, and that when the load has been committed the main resource has already received a response.

[Martin Robinson]

(In reply to comment #5)

> I've checked the problem and it's related to bug https://bugs.webkit.org/show_bug.cgi?id=91478. For pages loaded from the history cache, the parameter isMainResource is always true, so all subresources are considered the main one.

Certainly this must be a bug in the platform-independent code?

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > I've checked the problem and it's related to bug https://bugs.webkit.org/show_bug.cgi?id=91478. For pages loaded from the history cache, the parameter isMainResource is always true, so all subresources are considered the main one.
> 
> Certainly this must be a bug in the platform-independent code?

No, see bug #91478, we were incorrectly assuming that the resource loaded for the main frame that is provisionally loaded is the main resource of the web view. WebKit1 has the same issue, but in WebKit1 get_main_resource() calls the loader to get the main resource, so it doesn't need to assume anything.

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > 
> > > I've checked the problem and it's related to bug https://bugs.webkit.org/show_bug.cgi?id=91478. For pages loaded from the history cache, the parameter isMainResource is always true, so all subresources are considered the main one.
> > 
> > Certainly this must be a bug in the platform-independent code?
> 
> No, see bug #91478, we were incorrectly assuming that the resource loaded for the main frame that is provisionally loaded is the main resource of the web view. WebKit1 has the same issue, but in WebKit1 get_main_resource() calls the loader to get the main resource, so it doesn't need to assume anything.

If my understanding of what you are saying is correct, "the parameters isMainResource is always true" for pages loaded from the history cache. That means that many resources will be created with isMainResource set to true, so they won't be correctly instantiated (see webkit_web_resource_get_data).  So it seems to fix this bug you we must fix the value of isMainResource passed.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > 
> > > > I've checked the problem and it's related to bug https://bugs.webkit.org/show_bug.cgi?id=91478. For pages loaded from the history cache, the parameter isMainResource is always true, so all subresources are considered the main one.
> > > 
> > > Certainly this must be a bug in the platform-independent code?
> > 
> > No, see bug #91478, we were incorrectly assuming that the resource loaded for the main frame that is provisionally loaded is the main resource of the web view. WebKit1 has the same issue, but in WebKit1 get_main_resource() calls the loader to get the main resource, so it doesn't need to assume anything.
> 
> If my understanding of what you are saying is correct, "the parameters isMainResource is always true" for pages loaded from the history cache. That means that many resources will be created with isMainResource set to true, so they won't be correctly instantiated (see webkit_web_resource_get_data).  So it seems to fix this bug you we must fix the value of isMainResource passed.

isMainResource is actually pageIsProvisionallyLoading, so maybe we should simply change the name of the variable (isMainResource) because it's not always true. That's the assumption that was wrong. But we still need to check if it's the first resource or not.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > 
> > > > > I've checked the problem and it's related to bug https://bugs.webkit.org/show_bug.cgi?id=91478. For pages loaded from the history cache, the parameter isMainResource is always true, so all subresources are considered the main one.
> > > > 
> > > > Certainly this must be a bug in the platform-independent code?
> > > 
> > > No, see bug #91478, we were incorrectly assuming that the resource loaded for the main frame that is provisionally loaded is the main resource of the web view. WebKit1 has the same issue, but in WebKit1 get_main_resource() calls the loader to get the main resource, so it doesn't need to assume anything.
> > 
> > If my understanding of what you are saying is correct, "the parameters isMainResource is always true" for pages loaded from the history cache. That means that many resources will be created with isMainResource set to true, so they won't be correctly instantiated (see webkit_web_resource_get_data).  So it seems to fix this bug you we must fix the value of isMainResource passed.
> 
> isMainResource is actually pageIsProvisionallyLoading, so maybe we should simply change the name of the variable (isMainResource) because it's not always true. That's the assumption that was wrong. But we still need to check if it's the first resource or not.

btw, the reason why it's not always true is because pages loaded from the history cache don't go through the provisional state, they transition directly to committed, because they are already in the cache.

[Martin Robinson]

(In reply to comment #9)

> isMainResource is actually pageIsProvisionallyLoading, so maybe we should simply change the name of the variable (isMainResource) because it's not always true. That's the assumption that was wrong. But we still need to check if it's the first resource or not.

It looks like there is no surefire way that I can easily see to detect when a resource is the main resource, so this sounds like a good approach. I think we also need to correct the value passed to webkitWebResourceCreate, as isMainResource is incorrect.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #9)
> 
> > isMainResource is actually pageIsProvisionallyLoading, so maybe we should simply change the name of the variable (isMainResource) because it's not always true. That's the assumption that was wrong. But we still need to check if it's the first resource or not.
> 
> It looks like there is no surefire way that I can easily see to detect when a resource is the main resource, so this sounds like a good approach. I think we also need to correct the value passed to webkitWebResourceCreate, as isMainResource is incorrect.

Yes, that was my conclusion too, what we know for sure is that we need the main resource loaded to load subresources, so the first resource we load for the main frame has to be the main one. I'll change isMainResource to pageIsProvisionallyLoading before landing if patch gets approved.

[Carlos Garcia Campos]

Created attachment 162525 [details]
Updated patch

Remove the isMainResource parameter because it's now unused and pass the correct value to resource constructor.

[Carlos Garcia Campos]

Comment on attachment 162461 [details]
Patch

Thanks for the patch, we decided to follow a different approach to fix the source of the problem.

[Carlos Garcia Campos]

Committed r127750: <http://trac.webkit.org/changeset/127750>

[Sudarsana Nagineni (babu)]

(In reply to comment #14)
> Thanks for the patch, we decided to follow a different approach to fix the source of the problem.

Thanks for the proper fix, Carlos.