Bug 75445 - [Qt][WK2] QQuickWebView breaks when an empty url is loaded (debug mode)
Summary: [Qt][WK2] QQuickWebView breaks when an empty url is loaded (debug mode)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Critical
Assignee: Rafael Brandao
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2012-01-02 13:35 PST by Rafael Brandao
Modified: 2012-01-06 05:34 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.37 KB, patch)
2012-01-03 14:11 PST, Rafael Brandao
no flags Details | Formatted Diff | Diff
Patch (3.82 KB, patch)
2012-01-04 05:55 PST, Rafael Brandao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Brandao 2012-01-02 13:35:03 PST
I've tested it on Chrome and Safari, and both of them just ignore the request (once it is an empty url) and stays in the same page. If we don't handle this and request to load an empty url, an empty ResourceRequest is created, asserting on MainResourceLoader::willSendRequest. This behavior is well handled when we are using substitute data with an empty url as base url: the base url is changed to "about:blank", thus not crashing at all. It makes sense to ignore empty url requests, but it is weird that we crash when we get it.

Right now, QQuickWebView starts with an empty url and no content displayed. Once we load something with it, we can never return to the original state: no url and no content (this is how I've found this bug). This use case is very appropriate to create test cases, but I can't think of anything else where this would fit well.

A possible solution is to make QQuickWebView start with the url "about:blank" instead of a blank url, and ignore loads for empty urls as other browsers do (and add this to the documentation).

We could also change the way WebCore works handling it there, but I still couldn't find out why that assert is there and I suppose there's a good reason.

What you guys think?
Comment 1 Rafael Brandao 2012-01-03 04:25:22 PST
Ignoring the assert there, the url is loaded just like it was expected... it loads an empty page. The code on MainResourceLoader is very old, it is possible that the assert is no longer necessary but I'm still looking on it.

The comment preceding the assert on MainResourceLoader::willSendRequest is very confusing, it says the following code doesn't have any asserts for example. :-/
Comment 2 Rafael Brandao 2012-01-03 14:11:48 PST
Created attachment 120992 [details]
Patch
Comment 3 Rafael Brandao 2012-01-03 14:13:13 PST
(In reply to comment #2)
> Created an attachment (id=120992) [details]
> Patch

As we've discussed on IRC, it make sense to ignore an empty url when it is requested to be loaded.
Comment 4 Kenneth Rohde Christiansen 2012-01-03 14:15:47 PST
Comment on attachment 120992 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:757
>  void QQuickWebView::load(const QUrl& url)
>  {
> +    if (url.isEmpty())
> +        return;
> +
>      Q_D(QQuickWebView);
>      d->webPageProxy->loadURL(url.toString());
>  }

Should we emit load failed? a user might call load while doing another load which say reached 50% and with this change I believe it will just continue loading that page. Is this intended and can such a case be tested?
Comment 5 Rafael Brandao 2012-01-04 04:02:24 PST
(In reply to comment #4)
> (From update of attachment 120992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120992&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:757
> >  void QQuickWebView::load(const QUrl& url)
> >  {
> > +    if (url.isEmpty())
> > +        return;
> > +
> >      Q_D(QQuickWebView);
> >      d->webPageProxy->loadURL(url.toString());
> >  }
> 
> Should we emit load failed? a user might call load while doing another load which say reached 50% and with this change I believe it will just continue loading that page. Is this intended and can such a case be tested?

Maybe load fail would look like we actually tried to load it but we are just ignoring. I've looked how Chrome behaves and it keeps the old page loading, like nothing has happened (as we're doing). Safari reloads the same page when you use an empty url.
Comment 6 Kenneth Rohde Christiansen 2012-01-04 04:17:47 PST
anyway a test for that is needed.
Comment 7 Rafael Brandao 2012-01-04 05:55:33 PST
Created attachment 121103 [details]
Patch
Comment 8 WebKit Review Bot 2012-01-06 05:34:42 PST
Comment on attachment 121103 [details]
Patch

Clearing flags on attachment: 121103

Committed r104287: <http://trac.webkit.org/changeset/104287>
Comment 9 WebKit Review Bot 2012-01-06 05:34:47 PST
All reviewed patches have been landed.  Closing bug.