Bug 30573 – [Qt] Broken back/forward after using ErrorPageExtension to set error page

• Component: Page Loading
• Status: RESOLVED FIXED

[Antonio Gomes]

Qt relies on FrameLoader::checkLoadCompleteForThisFrame() to load error pages.


backtrace:
   (...) a load error happens  (...)
1) FrameLoader::checkLoadCompleteForThisFrame()
2) FrameLoaderClientQt::dispatchDidFailProvisionalLoad
3) FrameLoaderClientQt::callErrorPageExtension
4) an "error page" is set via FrameLoader::load(request, substituteData, lockHistory)

note that we're yet in the middle of _1_ , however another load has happened (the error page has been loaded in the same loader object). so the previous m_provisionalDocumentLoader has been changed.

In details, see quoted code below:

void FrameLoader::checkLoadCompleteForThisFrame()
{
    RefPtr<DocumentLoader> pdl = m_provisionalDocumentLoader;
    if (!pdl)
        return;
                
    // If we've received any errors we may be stuck in the provisional state and actually complete.
    const ResourceError& error = pdl->mainDocumentError();
    if (error.isNull())
        return;

    bool shouldReset = true; <--- THIS IS IMPORTANT
    if (!(pdl->isLoadingInAPISense() && !pdl->isStopping())) {
        m_delegateIsHandlingProvisionalLoadError = true;
        m_client->dispatchDidFailProvisionalLoad(error);
        m_delegateIsHandlingProvisionalLoadError = false;
        (...)

        // READ THE COMMENT BELOW !!!!

        // Finish resetting the load state, but only if another load hasn't been started by the
        // delegate callback.
        if (pdl == m_provisionalDocumentLoader)
            clearProvisionalLoad();
        else if (m_provisionalDocumentLoader) {
            KURL unreachableURL = m_provisionalDocumentLoader->unreachableURL();
            if (!unreachableURL.isEmpty() && unreachableURL == pdl->request().url())
                shouldReset = false;
        }
(...)


in our qt specific case "another load" *has* happenned *but* the "else" statement just checks for 'm_provisionalDocumentLoader'. That breaks our back / forward action because it does get reset. 

My solution: we could check for the current active document loader, not only for the m_provisionalDocumentLoader, so using activeDocumentLoader()  ...

patch coming

[Antonio Gomes]

Created attachment 41508 [details]
patch 0.1

use activeDocumentLoader() instead of m_provisionalDocumentLoader for the loader check in FrameLoader::checkLoadCompleteForThisFrame()

[Antonio Gomes]

adam, do you have an opinion here?

[Adam Barth]

Comment on attachment 41508 [details]
patch 0.1

I'd have to think more about whether this patch is correct, but I don't want to accept any functional FrameLoader patches without tests.  We need more test coverage of this code.

[Antonio Gomes]

(In reply to comment #3)
> (From update of attachment 41508 [details])
> I'd have to think more about whether this patch is correct, but I don't want to
> accept any functional FrameLoader patches without tests.  We need more test
> coverage of this code.

adam, thx for replying.

i knew it is not a finished patch (mainly due to the lack of tests) but i requested r? and cc'ed you to get attention and opinios here (since you are the one caring more about FrameLoader ATM). so please if you could ignore the lack of test, i'm wondering if it is a reasonable change to be considered.

ps: i do not have a MAC and can not check if it breaks current tests =/

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 41508 [details] [details])
> > I'd have to think more about whether this patch is correct, but I don't want to
> > accept any functional FrameLoader patches without tests.  We need more test
> > coverage of this code.
> 
> adam, thx for replying.
> 
> i knew it is not a finished patch (mainly due to the lack of tests) but i
> requested r? and cc'ed you to get attention and opinios here (since you are the
> one caring more about FrameLoader ATM). so please if you could ignore the lack
> of test, i'm wondering if it is a reasonable change to be considered.
> 
> ps: i do not have a MAC and can not check if it breaks current tests =/

Why do you need a mac? You can test the test with Qt and the Windows version of WebKit by installing it in VMWare or so.

You could also upload the test and ask someone else to test it for you.

[Antonio Gomes]

Created attachment 42829 [details]
patch 0.2 - on going patch

patch implements a layout test for the change and qt unit test, however in is on top of another local patch that implements "error page" capabilities to QT's DRT.
uploading for the record

[Antonio Gomes]

(In reply to comment #3)
> (From update of attachment 41508 [details])
> I'd have to think more about whether this patch is correct, but I don't want to
> accept any functional FrameLoader patches without tests.  We need more test
> coverage of this code.

patch 0.2 adds a layout test candidate for this bugfix. however it relies on DRT to support "error pages" handle [1]. In [2] I tried to elaborate on why to implement error pages support to DRT via webkit-dev ML, but I am not 100% sure if it is a good think.

[1] http://pastebin.com/f698dd57c
[2] http://old.nabble.com/Error-handling-support-in-DRT.-td26239359.html

@abarth: how do you think we could proceed here ?

[Antonio Gomes]

Created attachment 43232 [details]
patch 0.3

Proposed fix + layout test + qt autotest

Patch applied on top of patch in bug 31509 (please see bug description there).

[Antonio Gomes]

(In reply to comment #8)
> Created an attachment (id=43232) [details]
> patch 0.3
> 
> Proposed fix + layout test + qt autotest
> 
> Patch applied on top of patch in bug 31509 (please see bug description there).

Patch 0.3 adds the new test into mac, gtk and win's Skipped lists, and follow up bugs will be filed for each of this DRTs to implement support for error pages.

[Adam Barth]

Comment on attachment 43232 [details]
patch 0.3

You didn't actually include the test in the patch.  Also, once you mark the test as Skipped on mac, you don't need to mark it as Skipped in mac-leopard, etc.

[Antonio Gomes]

Created attachment 43249 [details]
(landed in r51038) patch 0.4

(In reply to comment #10)
> (From update of attachment 43232 [details])
> You didn't actually include the test in the patch.

err, sorry about that, @abarth. done.

> Also, once you mark the
> test as Skipped on mac, you don't need to mark it as Skipped in mac-leopard,
> etc.

done.

[Adam Barth]

This looks ok to me, but I don't have a complete grasp of how frame loader works.  Hopefully someone more expert than me will review your patch.  If that doesn't happen in a few days, let me know and I'll try to review it myself.

[Kenneth Rohde Christiansen]

Would be good to get this reviewed as soon as possible as it is blocking our 4.6 release. Who do you think is able to review this patch?

[Antti Koivisto]

Comment on attachment 43249 [details]
(landed in r51038) patch 0.4

The change looks safe to me. activeDocumentLoader() == m_provisionalDocumentLoader here except in this Qt specific case so this shouldn't affect other platforms either.

r=me.

[Antonio Gomes]

thx antti.

landed in r51038