Bug 30573 - [Qt] Broken back/forward after using ErrorPageExtension to set error page
Summary: [Qt] Broken back/forward after using ErrorPageExtension to set error page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt
Depends on: 31158 31164 31509
Blocks: Qt46
  Show dependency treegraph
 
Reported: 2009-10-20 08:59 PDT by Antonio Gomes
Modified: 2009-11-16 07:48 PST (History)
7 users (show)

See Also:


Attachments
patch 0.1 (2.63 KB, patch)
2009-10-20 09:13 PDT, Antonio Gomes
abarth: review-
Details | Formatted Diff | Diff
patch 0.2 - on going patch (5.10 KB, patch)
2009-11-09 18:30 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 0.3 (9.39 KB, patch)
2009-11-14 15:14 PST, Antonio Gomes
abarth: review-
Details | Formatted Diff | Diff
(landed in r51038) patch 0.4 (10.49 KB, patch)
2009-11-15 09:03 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-10-20 08:59:38 PDT
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
Comment 1 Antonio Gomes 2009-10-20 09:13:15 PDT
Created attachment 41508 [details]
patch 0.1

use activeDocumentLoader() instead of m_provisionalDocumentLoader for the loader check in FrameLoader::checkLoadCompleteForThisFrame()
Comment 2 Antonio Gomes 2009-10-20 12:00:22 PDT
adam, do you have an opinion here?
Comment 3 Adam Barth 2009-10-20 12:17:02 PDT
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.
Comment 4 Antonio Gomes 2009-10-20 13:19:52 PDT
(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 =/
Comment 5 Kenneth Rohde Christiansen 2009-10-30 05:35:28 PDT
(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.
Comment 6 Antonio Gomes 2009-11-09 18:30:05 PST
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
Comment 7 Antonio Gomes 2009-11-13 07:06:09 PST
(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 ?
Comment 8 Antonio Gomes 2009-11-14 15:14:41 PST
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).
Comment 9 Antonio Gomes 2009-11-14 15:27:51 PST
(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.
Comment 10 Adam Barth 2009-11-14 16:02:14 PST
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.
Comment 11 Antonio Gomes 2009-11-15 09:03:25 PST
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.
Comment 12 Adam Barth 2009-11-15 09:56:37 PST
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.
Comment 13 Kenneth Rohde Christiansen 2009-11-16 05:54:03 PST
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?
Comment 14 Antti Koivisto 2009-11-16 07:15:42 PST
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.
Comment 15 Antonio Gomes 2009-11-16 07:47:42 PST
thx antti.

landed in r51038