WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
30573
[Qt] Broken back/forward after using ErrorPageExtension to set error page
https://bugs.webkit.org/show_bug.cgi?id=30573
Summary
[Qt] Broken back/forward after using ErrorPageExtension to set error page
Antonio Gomes
Reported
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Antonio Gomes
Comment 1
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()
Antonio Gomes
Comment 2
2009-10-20 12:00:22 PDT
adam, do you have an opinion here?
Adam Barth
Comment 3
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.
Antonio Gomes
Comment 4
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 =/
Kenneth Rohde Christiansen
Comment 5
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.
Antonio Gomes
Comment 6
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
Antonio Gomes
Comment 7
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 ?
Antonio Gomes
Comment 8
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).
Antonio Gomes
Comment 9
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.
Adam Barth
Comment 10
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.
Antonio Gomes
Comment 11
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.
Adam Barth
Comment 12
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.
Kenneth Rohde Christiansen
Comment 13
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?
Antti Koivisto
Comment 14
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.
Antonio Gomes
Comment 15
2009-11-16 07:47:42 PST
thx antti. landed in
r51038
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug