Bug 136692 – [GTK] Race condition with WebKitWebView:is-loading after starting page load

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

After starting a page load with, say, webkit_web_view_load_uri or webkit_web_view_go_back, the is-loading property of the web view would ideally be TRUE.  Instead, it is FALSE, and will be set to TRUE soon afterwards. This is unexpected and a practical problem for Epiphany [1].

If we use a PageLoadState::Observer, we can be notified of the new load before these functions return, without the need to manually set the is-loading property in each of the several web view functions that start a load.

It doesn't look like we can completely replace the WebKitLoaderClient with a PageLoadState::Observer, so this is a minimal patch that only changes how we set the is-loading property. We might want to change more.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=735577#c15

[Michael Catanzaro]

Created attachment 237878 [details]
Patch

Unfortunately, this either requires the WebKitWebViewBase to assume it is also a WebKitWebView, or else for the WebKitPageLoadObserver to make the same assumption. This seems undesirable, even if it is a pretty safe bet.

[Carlos Garcia Campos]

Comment on attachment 237878 [details]
Patch

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

I also started with this, I have a wip patch in my local repo, but it's blocked by other differences between our API and the internal one regarding the active URL.

> Source/WebKit2/PlatformGTK.cmake:169
> +    UIProcess/API/gtk/WebKitPageLoadObserver.cpp
> +    UIProcess/API/gtk/WebKitPageLoadObserver.h

I'm not sure we need a new file for this, I would add the class in WebKitWebView.cpp, since it's a private class an only used by WebKitWebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:40
> -    webkitWebViewLoadChanged(WEBKIT_WEB_VIEW(clientInfo), WEBKIT_LOAD_STARTED);
> +    ASSERT_UNUSED(clientInfo, webkit_web_view_is_loading(WEBKIT_WEB_VIEW(clientInfo)));

I don't think this is correct, we should really call webkitWebViewLoadChanged here.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:24
> +#include <WebCore/platform/NotImplemented.h>

This should be #include <WebCore/NotImplemented.h> but I don't think we need this here.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:30
> +    , m_pageLoadState(pageLoadState)

I would leave the caller use PageLoadState to add/remove the observer

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:43
> +PassRefPtr<WebKitPageLoadObserver> WebKitPageLoadObserver::create(WebKitWebView& webView, PageLoadState& pageLoadState)
> +{
> +    return adoptRef(new WebKitPageLoadObserver(webView, pageLoadState));
> +}

I don't this this class nedds to be ref counted, it will not be shared, it should be created by the web view in the constructor and deleted on dispose.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:47
> +    notImplemented();

Don't need to use notImplemented in this case, I think, since it's not something that will not work because this is not implemented. We can use the will* methods to freeze the gobject property notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:53
> +    if (m_pageLoadState.isLoading())
> +        webkitWebViewLoadChanged(&m_webView, WEBKIT_LOAD_STARTED);

I don't think this is correct, we should only emit load-started when the frame has transition to provisional state, and this is happening before. I agree on changing the is-loading before as well as making sure that get_uri() right after load_uri() doesn't return null but the requested uri, but for that we should also implement the didChangeActiveURL.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.h:29
> +class WebKitPageLoadObserver : public RefCounted<WebKitPageLoadObserver>, public PageLoadState::Observer {

This class could be marked as final, and again I don't think it needs to be refcounted.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.h:62
> +    WebKitWebView& m_webView;

I don't use references for GObjects, use a pointer instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:102
> +    RefPtr<WebKitPageLoadObserver> pageLoadObserver;

I don't think this should be in WebKitWebViewBase, but in WebKitWebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1011
> +    priv->pageLoadObserver = WebKitPageLoadObserver::create(*WEBKIT_WEB_VIEW(webkitWebViewBase), priv->pageProxy->pageLoadState());

It's not correct to cast the webkitWebViewBase as WebKitWebView here.

[Michael Catanzaro]

(In reply to comment #2)
> (From update of attachment 237878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237878&action=review
> 
> I also started with this, I have a wip patch in my local repo, but it's blocked by other differences between our API and the internal one regarding the active URL.

Do you want me to keep working on this then?

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:102
> > +    RefPtr<WebKitPageLoadObserver> pageLoadObserver;
> 
> I don't think this should be in WebKitWebViewBase, but in WebKitWebView.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1011
> > +    priv->pageLoadObserver = WebKitPageLoadObserver::create(*WEBKIT_WEB_VIEW(webkitWebViewBase), priv->pageProxy->pageLoadState());
> 
> It's not correct to cast the webkitWebViewBase as WebKitWebView here.

The problem is that I'm not sure when else it is safe to add the page load observer, except when the WebKitWebViewBase creates a new page?

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 237878 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=237878&action=review
> > 
> > I also started with this, I have a wip patch in my local repo, but it's blocked by other differences between our API and the internal one regarding the active URL.
> 
> Do you want me to keep working on this then?

I already have a patch, but we need to fix other things before, so it's better to wait for now.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:102
> > > +    RefPtr<WebKitPageLoadObserver> pageLoadObserver;
> > 
> > I don't think this should be in WebKitWebViewBase, but in WebKitWebView.
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1011
> > > +    priv->pageLoadObserver = WebKitPageLoadObserver::create(*WEBKIT_WEB_VIEW(webkitWebViewBase), priv->pageProxy->pageLoadState());
> > 
> > It's not correct to cast the webkitWebViewBase as WebKitWebView here.
> 
> The problem is that I'm not sure when else it is safe to add the page load observer, except when the WebKitWebViewBase creates a new page?

In the web view constructor

[Carlos Garcia Campos]

Created attachment 238303 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 238303 [details]
Patch

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

LGTM, thanks!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1590
> +    if (isDelayedEvent) {
> +        if (loadEvent == WEBKIT_LOAD_COMMITTED)
> +            webView->priv->waitingForMainResource = false;
> +        else if (loadEvent == WEBKIT_LOAD_FINISHED) {
> +            // Manually set is-loading again in case a new load was started.
> +            webkitWebViewSetIsLoading(webView, getPage(webView)->pageLoadState().isLoading());
> +        }
> +    }

Is there a reason not to do these checks in the if/else if block above? Do they need to come after the load changed signal emission?

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 238303 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238303&action=review
> 
> LGTM, thanks!
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1590
> > +    if (isDelayedEvent) {
> > +        if (loadEvent == WEBKIT_LOAD_COMMITTED)
> > +            webView->priv->waitingForMainResource = false;
> > +        else if (loadEvent == WEBKIT_LOAD_FINISHED) {
> > +            // Manually set is-loading again in case a new load was started.
> > +            webkitWebViewSetIsLoading(webView, getPage(webView)->pageLoadState().isLoading());
> > +        }
> > +    }
> 
> Is there a reason not to do these checks in the if/else if block above? Do they need to come after the load changed signal emission?

Exactly, for example we guarantee that is-loading is false when load-changed is emitted with FINISHED.

[Carlos Garcia Campos]

Committed r174497: <http://trac.webkit.org/changeset/174497>