Bug 61328 – [Qt] QtWebkit never finishes loading sites when they are loaded after an initial QUrl fails to load.

• Component: WebKit Qt
• Status: RESOLVED FIXED

The problem is hard to explain. So let me give you an example as to where this problem would occur:

My user enters in a non-existant or badly-written URL, that has no valid host. They click the Go button, and a QWebView then parses the QString as a QUrl and uses the setUrl method to navigate there.
Upon the loadFinished signal activating the connected slot, the slot then see's, thanks to the passed boolean variable, that the attempted URL failed to load, and thus, it fallsback on perhaps a google search that contains the bad term.
The google search is started via the setUrl method inside the loadFinished signal's slot. It activates the loadStarted and loadProgress signals, but never gets to past ~50% load progress, and therefore never activates the loadFinished signal, and therefore never finishes loading.. causing the user to see a blank white page.


Attempting to go to any page through the loadFinished signal's slot appears to not work. As in the above example, even a simple URL like http://google.com passed to the setUrl method in loadFinished signal's slot appears to never finish loading.

The attached file is a testcase. Attach it to a simple project with a project file that contains "QT += core gui webkit" and compiles the app as a GUI binary. Clean the project, run qmake, and then build and run the application.
You will see a QTextEdit and a QPushButton side by side. Below it, you will see a QWebView. Enter your desired URL in to the QTextEdit and press the QPushButton(says "Go"). 
The QWebView will then load to the desired URL, with debugging occurring from the first time the user hits the QPushButton, all the way through all 3 loading signals [start, progress, finish] in the QWebView. The debugging occurs through qDebug(), so make sure you check the console.

To test for the stated bug, enter in a bad URL. I usually use:
http://google./com


You will see that the URL will fail to load, fallback on the google search, and never finish loading from there. It will stop loading at around 50-55% progress - for no apparent reason.

This is a major bug in my opinion that makes it impossible to do anything along the lines of a fallback search.

Created attachment 94525 [details]
The testcase.

This is the testcase. Make sure to attach it to a correct .pro file. See the description of this bug for more info.

Also, some information I left out:
I have tested this problem and it occurs in the same way on both QtWebkit 2.0.2[as delivered with Qt 4.7.3], and the latest version of QtWebkit 2.2[as seen on http://trac.webkit.org/wiki/QtWebKitRelease22].

Erm, I'm not sure how to say this...
I am going to modify a discussion I had [mainly with myself] just now, which will say everything you need to know:

In this bug report, I claimed to have tested it on both QtWebkit 2.0.2[default for Qt 4.7.3] and QtWebkit2.2[in beta, will be in next release of Qt]. I soon remembered I only compiled my testcase only with QtWebkit 2.0.2.
So I then went to my actual project [not this testcase], which uses QtWebkit 2.2. I changed a few things around to make it act like the testcase.
Now, although the testcase stopped loading the fallback URL 50% of the way through, my QtWebkit2.2 application loaded it perfectly when using my testcase's exact methods and such.
So I was like "Wow... I have been trying to fix this crash for several days...". I then changed my testcase compiler around to use QtWebkit2.2, as my [now working] application uses it.
I compiled my testcase and tested the bug. Now, instead of just failing to load halfway through, it crashed the entire application.

In other words:
Testcase doesn't crash when using QtWebkit 2.0.2 (although it does show a bug) --> Modify application, which uses QtWebkit 2.2, which crashed in the past, to use testcase methods --> Application works perfectly --> Modify testcase compiler to use same QtWebkit as my application [QtWebkit 2.0.2 to QtWebkit 2.2] -> Testcase gets WORSE, and crashes.

In OTHER words, I completely fixed my application using my testcase's methods. (0 bugs) I then tried to run my testcase with the same version of QtWebkit (which was 2.2) as my application, and the testcase now crashes.

If this is a bug, then... I am not really sure what is going on. I am quite confused myself. I hope to see some interesting comments from Webkit staff soon.

Ignore this whole comment please.

I wasn't thinking, and completely forgot that my original application used a workaround this bug, which is why it didn't crash. Testing my application (which uses QtWebkit 2.2) using the same methods as my testcase - without my workaround - causes my application to crash, which is the same result as when the testcase uses QtWebkit 2.2.
I am terribly sorry for the confusion.

(In reply to comment #3)
> Erm, I'm not sure how to say this...
> I am going to modify a discussion I had [mainly with myself] just now, which will say everything you need to know:
> 
> In this bug report, I claimed to have tested it on both QtWebkit 2.0.2[default for Qt 4.7.3] and QtWebkit2.2[in beta, will be in next release of Qt]. I soon remembered I only compiled my testcase only with QtWebkit 2.0.2.
> So I then went to my actual project [not this testcase], which uses QtWebkit 2.2. I changed a few things around to make it act like the testcase.
> Now, although the testcase stopped loading the fallback URL 50% of the way through, my QtWebkit2.2 application loaded it perfectly when using my testcase's exact methods and such.
> So I was like "Wow... I have been trying to fix this crash for several days...". I then changed my testcase compiler around to use QtWebkit2.2, as my [now working] application uses it.
> I compiled my testcase and tested the bug. Now, instead of just failing to load halfway through, it crashed the entire application.
> 
> In other words:
> Testcase doesn't crash when using QtWebkit 2.0.2 (although it does show a bug) --> Modify application, which uses QtWebkit 2.2, which crashed in the past, to use testcase methods --> Application works perfectly --> Modify testcase compiler to use same QtWebkit as my application [QtWebkit 2.0.2 to QtWebkit 2.2] -> Testcase gets WORSE, and crashes.
> 
> In OTHER words, I completely fixed my application using my testcase's methods. (0 bugs) I then tried to run my testcase with the same version of QtWebkit (which was 2.2) as my application, and the testcase now crashes.
> 
> If this is a bug, then... I am not really sure what is going on. I am quite confused myself. I hope to see some interesting comments from Webkit staff soon.

[Caio Marcelo de Oliveira Filho]

I could reproduce the problem in Linux too. I'm looking into this.

Marking it as P1 because it is a simple (and not unusual) use of the API that is crashing.

Importance changed from P1 Major to P1 Critical, as the application does crash when it hits this 'bug' in QtWebkit 2.2.

[Caio Marcelo de Oliveira Filho]

Created attachment 96184 [details]
Patch

Just some context:

- The core of the problem is that when FrameLoaderClient makes callbacks to user code, we must be sure that the FrameLoader is in a consistent state.

- I tried enqueueing the signals, but this led to other problems like: changing order of expected signals in DRT, and ultimately, the need of enqueueing setUrl()/load() calls as well to avoid loops due to setUrl() -> cancel load -> enqueue loadFinished(false) -> start load -> process loadFinished(false) -> setUrl() -> ...

- I could try to manipulate FrameLoader code to make sure that state will be consistent, but I think this would be always hard to keep, specially because other APIs doesn't use progress notification for this kind of signal. This new approach is used both by Mac and Gtk. I'm not really sure if the FrameLoader need to be ready for taking a new load() at every point in FrameLoaderClient. The progress notification were a gray area for me in that matter. I would be glad to hear opinions about this particular point.

- Now every frame will have its own loadStarted() and loadFinished(). We didn't emitted those for every frame before, only for the "originating frame", because the progress notification only happen in the "originating frame". QWebPage signals continue to be emitted only when the "originating frame" start/finish.

- I'm assuming that: the last frame to be loaded will be the "originating frame". This seems to match pieces of code I looked elsewhere.

- This commit will surface the issue reported in https://bugs.webkit.org/show_bug.cgi?id=28851. I prefer that we fix this other problem in a separate commit, since it might demand different testing. We can try to workaround at Qt level too, but I don't see why not use 28851 :-)

[Caio Marcelo de Oliveira Filho]

Benjamin, could you take a look at this one?

[Adam Barth]

Comment on attachment 96184 [details]
Patch

This looks fine w.r.t. FrameLoaderClient, as far as I can tell, but I'm sure I understand everything in full detail.

[Adam Barth]

> I'm sure I understand everything in full detail.

*unsure.  :)

[Robert Hogan]

Comment on attachment 96184 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1181
> +
> +    if (m_webFrame)
> +        emitLoadFinished(false);

I *think* dispatchDidFailLoad() will only fire when there is a network-level error. At the moment QtWebKit emits loadFinished(false) even when there are 4XX errors (which don't result in a didFail()). This patch will change that behaviour.

A related bug for this problem is https://bugs.webkit.org/show_bug.cgi?id=62852. There isn't any clear definition of how didFail() should be used, as sometimes it's invoked by WebCore and sometimes by WebKit. AFAICT the Mac and Chromium ports reserve it exclusively for transport errors.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 96184 [details]
Patch

The patch is wrong since it end up emitting more signals than necessary in the case of ErrorPageExtension.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #11)
> I *think* dispatchDidFailLoad() will only fire when there is a network-level error. At the moment QtWebKit emits loadFinished(false) even when there are 4XX errors (which don't result in a didFail()). This patch will change that behaviour.

Thanks for looking into this Robert. I tested the case for 404:

- 404 with contents will end up with a loadFinished(true).
- 404 without contents will ask for ErrorPageExtension, if it handles, then loadFinished(true), if the extension doesn't handle the error, loadFinished(false).

The current code relies on dispatchDidFailProvisionalLoad() and dispatchDidFailLoad() for getting the errors (only them fill m_loadError with something). The patch use the same functions to emit the error, so I think this will not cause a change on the behavior.

I'm working on autotests to make sure of that.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #12)
> (From update of attachment 96184 [details])
> The patch is wrong since it end up emitting more signals than necessary in the case of ErrorPageExtension.

More specifically, when I use the error page extension and it causes a new load, I shouldn't emit the signal again. I'm working to fix that.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #13)
> I'm working on autotests to make sure of that.

Robert, could you take a look at bug 63490? I've tried to test some more cases for loadFinished(), considering that existing implementation is the correct one.

[Caio Marcelo de Oliveira Filho]

Created attachment 101495 [details]
Patch

[Benjamin Poulain]

Luiz, Andreas, could you have a look?

[Andreas Kling]

Comment on attachment 101495 [details]
Patch

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

This is a pretty cool patch!

> Source/WebKit/qt/ChangeLog:36
> +        (WebCore::FrameLoaderClientQt::FrameLoaderClientQt): Initialize
> +        m_isOriginatingLoad().

Not sure what m_isOriginatingLoad() means here.

[Caio Marcelo de Oliveira Filho]

Committed r91855: <http://trac.webkit.org/changeset/91855>

[Ademar Reis]

Revision r91855 cherry-picked into qtwebkit-2.2 with commit 382deba <http://gitorious.org/webkit/qtwebkit/commit/382deba>