[Webkit-unassigned] [Bug 142375] New: [GTK] UI process crashes if webkit_web_view_get_tls_info is called before internal load-committed event

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=142375

            Bug ID: 142375
           Summary: [GTK] UI process crashes if
                    webkit_web_view_get_tls_info is called before internal
                    load-committed event
    Classification: Unclassified
           Product: WebKit
           Version: 528+ (Nightly build)
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: WebKit Gtk
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: mcatanzaro at igalia.com

webkit_web_view_get_tls_info does not check if mainFrame->certificateInfo() is null before using it. It will always be null before WebFrameProxy::didCommitLoad is called, so we always crash in that case. We never noticed because Epiphany is probably the only user of this function, and it only calls it after receiving the WebKitGTK+ LOAD_COMMITTED event, since there is no point in calling this function before that. Now, I think it is user error to call this function before LOAD_COMMITTED, but we should return FALSE and maybe use g_warning() rather than crash.

So that is easy to fix. But, here is the fun part: it is possible that, if we have delayed the emission of LOAD_COMMITTED, WebFrameProxy::didStartProvisionalLoad gets called for the *next* load before we emit LOAD_COMMITTED for the previous load. In that case, the backtrace (crash reported downstream) looks like this:

 #0 webkit_web_view_get_tls_info at /lib64/libwebkit2gtk-4.0.so.37
 #1 load_changed_cb
 #6 webkitWebViewEmitLoadChanged(_WebKitWebView*, WebKitLoadEvent, bool) at /lib64/libwebkit2gtk-4.0.so.37
 #7 webkitWebViewEmitDelayedLoadEvents(_WebKitWebView*) at /lib64/libwebkit2gtk-4.0.so.37
 #8 webkitWebViewLoadChanged(_WebKitWebView*, WebKitLoadEvent) at /lib64/libwebkit2gtk-4.0.so.37
 #9 WebKit::WebPageProxy::didStartProvisionalLoadForFrame(unsigned long, unsigned long, WTF::String const&, WTF::String const&, WebKit::UserData const&) at /lib64/libwebkit2gtk-4.0.so.37
 #10 void IPC::handleMessage<Messages::WebPageProxy::DidStartProvisionalLoadForFrame, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long, unsigned long, WTF::String const&, WTF::String const&, WebKit::UserData const&)>(IPC::MessageDecoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long, unsigned long, WTF::String const&, WTF::String const&, WebKit::UserData const&)) at /lib64/libwebkit2gtk-4.0.so.37

So LOAD_COMMITTED gets emitted in response to the internal load started event, and Ephy crashes because it has used webkit_web_view_get_tls_info before the internal load committed event! Now, the crash is easy to fix, but is it really wise to emit LOAD_COMMITTED in this case? It is being done intentionally, judging by the comment "Finish a possible previous load waiting for main resource" in webkitWebViewLoadChanged, but I bet this will cause other unexpected, impossible to reproduce bugs. For instance, a random example: with this crash fixed, Epiphany would next call webkit_web_view_get_uri() and get the URI for the new page that's loading, not the page that was committed. Then it will get the next load started event without ever returning to the main loop (which I suppose a reasonable application might expect to happen). On the other hand, a reasonable application is more likely to expect to always receive a LOAD_COMMITTED for each LOAD_STARTED, so I guess we can't just skip it.

Eh, I will just fix the crash, but this is something to think about.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150306/1f21ef53/attachment-0002.html>