STEPS TO REPRODUCE: 1. Load any non-secure page (e.g. www.yahoo.com) 2. Load any secure page (e.g. www.gmail.com) 3. Select Back button. 4. Yahoo page will be loaded, observe the title bar. ACTUAL RESULTS: Secure icon is seen. EXPECTED RESULTS: Secure icon should not be seen.
Additional information from Dinu Jacob: The issue happens when we attempt to go back when caching is enabled. When we go back in history with the page cached, in WebCore::FrameLoader::transitionToCommitted, as 'cachePage' is a valid pointer, disptachDidCommitLoad is called (FrameLoader.cpp: 2546 ). At this point , m_URL of FrameLoader has not been updated yet. disptachDidCommitLoad calls client disptachDidCommitLoad which is FrameLoaderClientQt::dispatchDidCommitLoad(). This emits m_webFrame->urlChanged with the url of m_webFrame which is m_URL of FrameLoader ( d->frame->loader()->url() ). As m_URL of FrameLoader has not been updated with the new url, the signal is emitted with the old url. Secureuicontroller is using this url to determine whether to show/hide the secure icon. FrameLoader sets m_URL to the new value immediately after that in WebCore::FrameLoader::open (to cahcedFrame.url() ): #0 WebCore::FrameLoader::open (this=0xb25956a8, cachedFrame=@0xb22be244) at /nokia/bs_nmp/groups/s40_builds/viatches/qt-everywhere-opensource-src-4.6.2/src/3rdparty/webkit/WebCore/loader/FrameLoader.cpp:2638 #1 0xb724ed7f in WebCore::CachedFrame::open (this=0xb22be240) at /nokia/bs_nmp/groups/s40_builds/viatches/qt-everywhere-opensource-src-4.6.2/src/3rdparty/webkit/WebCore/history/CachedFrame.cpp:150 #2 0xb724f8ba in WebCore::CachedPage::restore (this=0xb1fc8d40, page=0xb43c16e0) at /nokia/bs_nmp/groups/s40_builds/viatches/qt-everywhere-opensource-src-4.6.2/src/3rdparty/webkit/WebCore/history/CachedPage.cpp:73 #3 0xb732ae28 in WebCore::FrameLoader::open (this=0xb25956a8, cachedPage=@0xb1fc8d40) at /nokia/bs_nmp/groups/s40_builds/viatches/qt-everywhere-opensource-src-4.6.2/src/3rdparty/webkit/WebCore/loader/FrameLoader.cpp:2631 #4 0xb732fff0 in WebCore::FrameLoader::commitProvisionalLoad (this=0xb25956a8, prpCachedPage=@0xbfffbb44) at /nokia/bs_nmp/groups/s40_builds/viatches/qt-everywhere-opensource-src-4.6.2/src/3rdparty/webkit/WebCore/loader/FrameLoader.cpp:2401 m_URL should be set to the new url before the call to disptachDidCommitLoad. In a normal load, m_URL is initially set to the new value in FrameLoader::didOpenURL called from FrameLoader::commitProvisionalLoad to the url value in m_provisionalDocumentLoader. It is set again after getting data in FrameLoader::begin (called from FrameLoader::receivedFirstData() and m_workingURL is passed into begin - this is also set in didOpenURL ). Even though transitionToCommitted is called in the normal case also, dispatchDidCommitLoad() is not called as cachedPage is not valid. Issue is in Webkit. Fix is: Before calling dispatchDidCommitLoad in the function FrameLoader::transitionToCommitted(PassRefPtr<CachedPage> cachedPage), need to set m_URL. m_URL = dl->url();
Created attachment 63924 [details] Proposed patch with test case
reopening as it was closed by mistake..
Comment on attachment 63924 [details] Proposed patch with test case We should remove FrameLoader::m_URL instead. It's redundant with Document::url and can only lead to bugs.
I think removing FrameLoader::m_URL either recently landed or is about to land.
Any update on the issue? This is one of the issue that blocks Webkit 2.1 release
Created attachment 66737 [details] Patch file with changes
Comment on attachment 66737 [details] Patch file with changes Rejecting patch 66737 from review queue. dinu.jacob@Nokia.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 66737 [details] Patch file with changes Rejecting patch 66737 from commit-queue. dinu.jacob@Nokia.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
(In reply to comment #7) > Created an attachment (id=66737) [details] > Patch file with changes Hi Adam, I haven't seen the patch to remove m_URL yet. While waiting for that, when I analyzed the bug with rest to that change, I found that since the frame document is set to the cached frame document only in FrameLoader::open(CachedFrameBase& cachedFrame), additional changes are required to avoid client from getting the incorrect url still. dispatchDidCommitLoad should be called only after FrameLoader::open is called (where all the initializations including setting the document and url is done). Can you please review the new changes? Dinu
Comment on attachment 66737 [details] Patch file with changes > + String ptitle = m_documentLoader->title(); > + if (!ptitle.isNull()) > + m_client->dispatchDidReceiveTitle(ptitle); What does "ptitle" mean? Can this local variable just be named "title"?
Created attachment 66749 [details] Patch with change in local variable name Changed local variable ptitle to title as requested.
Comment on attachment 66749 [details] Patch with change in local variable name View in context: https://bugs.webkit.org/attachment.cgi?id=66749&action=prettypatch > WebCore/ChangeLog:5 > + When loading a cached page,dispatchDidCommitLoad is called after FrameLoader::open so bad spacing > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1292 > +void tst_QWebPage::loadCachedPage() Is there a reason this can't be a LayoutTest?
Comment on attachment 66749 [details] Patch with change in local variable name View in context: https://bugs.webkit.org/attachment.cgi?id=66749&action=prettypatch > WebCore/ChangeLog:5 > + When loading a cached page,dispatchDidCommitLoad is called after FrameLoader::open so Will fix and add another patch > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1292 > +void tst_QWebPage::loadCachedPage() The frameloader values (like url etc) that client gets when dispatchDidCommitLoad is called are incorrect. It was best indicated by this test where the client is accessing it while in client's dispatchDidCommitLoad code.
Created attachment 66890 [details] Patch with spacing in comments fixed
Comment on attachment 66890 [details] Patch with spacing in comments fixed > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 66977) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,18 @@ > +2010-09-08 Jacob Dinu <dinu.jacob@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + When loading a cached page, dispatchDidCommitLoad is called after FrameLoader::open so > + that all initialzations are done before calling client dispatchDidCommitLoad to avoid spelling (initialzations) > + client from accessing incorrect data. grammar to avoid (the) client > +
Created attachment 67026 [details] Patch with spelling and grammar corrections
Comment on attachment 67026 [details] Patch with spelling and grammar corrections You need to click the patch checkbox for the tools to work properly.
Comment on attachment 67026 [details] Patch with spelling and grammar corrections This member is being removed in https://bugs.webkit.org/show_bug.cgi?id=41165. Let's wait and see if the problem still exists once that lands.
(In reply to comment #19) > (From update of attachment 67026 [details]) > This member is being removed in https://bugs.webkit.org/show_bug.cgi?id=41165. Let's wait and see if the problem still exists once that lands. Hi Adam, When I asked Nate on when his patch might be landed, he indicated that it was unlikely to happen in the next two weeks ( https://bugs.webkit.org/show_bug.cgi?id=41165#c12). As this bug is a blocker for our release, can you review my patch ?
Thanks for coordinating with Nate. Looking now.
Comment on attachment 67026 [details] Patch with spelling and grammar corrections I think this is ok. This code is complicated, especially the page cache part.
(In reply to comment #22) > (From update of attachment 67026 [details]) > I think this is ok. This code is complicated, especially the page cache part. Thanks Adam. Should I submit another patch based on the latest and request it to be committed?
Sure
Created attachment 67832 [details] Final patch to be committed Patch based on latest revision to be committed after review.
(In reply to comment #24) > Sure Adam, I have a attached a new patch based on the latest revision. Can you please approve and commit this one? Thanks, Dinu
Comment on attachment 67832 [details] Final patch to be committed View in context: https://bugs.webkit.org/attachment.cgi?id=67832&action=prettypatch > WebCore/ChangeLog:11 > + Only qt What does this cryptic change log line mean? The patch is a change to code that is shared by all platforms. Do you mean to say something like: “This change is harmless for all other platforms and helpful for Qt.”? > WebCore/loader/FrameLoader.cpp:1852 > + // For non-cached HTML pages, these methods are called in receivedFirstData(). This comment, which you moved here with the code, is now confusing. In the old code, it was referring to a line of code just two lines up where receivedFirstData is called. In this new code, it’s hard to understand what that means exactly.
(In reply to comment #27) > (From update of attachment 67832 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67832&action=prettypatch > > > WebCore/ChangeLog:11 > > + Only qt > > What does this cryptic change log line mean? The patch is a change to code that is shared by all platforms. Do you mean to say something like: “This change is harmless for all other platforms and helpful for Qt.”? > I replaced the line that asked for tests to indicate that tests were written specifically for Qt. Should I just remove this line? > > WebCore/loader/FrameLoader.cpp:1852 > > + // For non-cached HTML pages, these methods are called in receivedFirstData(). > > This comment, which you moved here with the code, is now confusing. In the old code, it was referring to a line of code just two lines up where receivedFirstData is called. In this new code, it’s hard to understand what that means exactly. I will remove this comment
Comment on attachment 67832 [details] Final patch to be committed View in context: https://bugs.webkit.org/attachment.cgi?id=67832&action=prettypatch >>> WebCore/ChangeLog:11 >>> + Only qt >> >> What does this cryptic change log line mean? The patch is a change to code that is shared by all platforms. Do you mean to say something like: “This change is harmless for all other platforms and helpful for Qt.”? > > I replaced the line that asked for tests to indicate that tests were written specifically for Qt. Should I just remove this line? Yes. >>> WebCore/loader/FrameLoader.cpp:1852 >>> + // For non-cached HTML pages, these methods are called in receivedFirstData(). >> >> This comment, which you moved here with the code, is now confusing. In the old code, it was referring to a line of code just two lines up where receivedFirstData is called. In this new code, it’s hard to understand what that means exactly. > > I will remove this comment Thanks.
(In reply to comment #29) > (From update of attachment 67832 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67832&action=prettypatch > > >>> WebCore/ChangeLog:11 > >>> + Only qt > >> > >> What does this cryptic change log line mean? The patch is a change to code that is shared by all platforms. Do you mean to say something like: “This change is harmless for all other platforms and helpful for Qt.”? > > > > I replaced the line that asked for tests to indicate that tests were written specifically for Qt. Should I just remove this line? > > Yes. > > >>> WebCore/loader/FrameLoader.cpp:1852 > >>> + // For non-cached HTML pages, these methods are called in receivedFirstData(). > >> > >> This comment, which you moved here with the code, is now confusing. In the old code, it was referring to a line of code just two lines up where receivedFirstData is called. In this new code, it’s hard to understand what that means exactly. > > > > I will remove this comment > > Thanks. (In reply to comment #29) > (From update of attachment 67832 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67832&action=prettypatch > > >>> WebCore/ChangeLog:11 > >>> + Only qt > >> > >> What does this cryptic change log line mean? The patch is a change to code that is shared by all platforms. Do you mean to say something like: “This change is harmless for all other platforms and helpful for Qt.”? > > > > I replaced the line that asked for tests to indicate that tests were written specifically for Qt. Should I just remove this line? > > Yes. > > >>> WebCore/loader/FrameLoader.cpp:1852 > >>> + // For non-cached HTML pages, these methods are called in receivedFirstData(). > >> > >> This comment, which you moved here with the code, is now confusing. In the old code, it was referring to a line of code just two lines up where receivedFirstData is called. In this new code, it’s hard to understand what that means exactly. > > > > I will remove this comment > > Thanks. Adam, For the next patch I upload, do I need to set both flags (review and commit ) to ? Thank you for all your help. Dinu
Created attachment 67838 [details] Patch with unwanted comments removed
Created attachment 67839 [details] Patch with unwanted comments removed
Comment on attachment 67839 [details] Patch with unwanted comments removed View in context: https://bugs.webkit.org/attachment.cgi?id=67839&action=prettypatch > WebCore/ChangeLog:9 > + When loading a cached page, dispatchDidCommitLoad is called after FrameLoader::open so > + that all initialzations are done before calling client dispatchDidCommitLoad to avoid > + client from accessing incorrect data. > + https://bugs.webkit.org/show_bug.cgi?id=41155 > + Ideally we'd mention the test here, but I don't think its worth re-spinning the patch. > WebCore/loader/FrameLoader.cpp:1857 > + // If we have a title let the WebView know about it. > + String title = m_documentLoader->title(); > + if (!title.isNull()) > + m_client->dispatchDidReceiveTitle(title); On the subject of useless comments. :)
(In reply to comment #33) > (From update of attachment 67839 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67839&action=prettypatch > > > WebCore/ChangeLog:9 > > + When loading a cached page, dispatchDidCommitLoad is called after FrameLoader::open so > > + that all initialzations are done before calling client dispatchDidCommitLoad to avoid > > + client from accessing incorrect data. > > + https://bugs.webkit.org/show_bug.cgi?id=41155 > > + > > Ideally we'd mention the test here, but I don't think its worth re-spinning the patch. I will remember this next time. > > > WebCore/loader/FrameLoader.cpp:1857 > > + // If we have a title let the WebView know about it. > > + String title = m_documentLoader->title(); > > + if (!title.isNull()) > > + m_client->dispatchDidReceiveTitle(title); > > On the subject of useless comments. :) Had same thoughts but left it alone as you hadn't mentioned it :)
Comment on attachment 67839 [details] Patch with unwanted comments removed This causes a build failure on mac. Sadly teh commit-queue hung before it could report it.
Comment on attachment 67026 [details] Patch with spelling and grammar corrections Cleared Adam Barth's review+ from obsolete attachment 67026 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 67832 [details] Final patch to be committed Cleared Adam Barth's review+ from obsolete attachment 67832 [details] so that this bug does not appear in http://webkit.org/pending-commit.
(In reply to comment #35) > (From update of attachment 67839 [details]) > This causes a build failure on mac. Sadly teh commit-queue hung before it could report it. I was able to build successfully on mac and found no new test failures either (my build was based on r64875). How can I now address this? Please advice.
(In reply to comment #38) > (In reply to comment #35) > > (From update of attachment 67839 [details] [details]) > > This causes a build failure on mac. Sadly teh commit-queue hung before it could report it. > > I was able to build successfully on mac and found no new test failures either (my build was based on r64875). How can I now address this? Please advice. Typo - based on r67845.
Comment on attachment 67839 [details] Patch with unwanted comments removed Let's just cq+ it again. It's my fault if the cq hangs. :)
Comment on attachment 67839 [details] Patch with unwanted comments removed Clearing flags on attachment: 67839 Committed r67878: <http://trac.webkit.org/changeset/67878>
All reviewed patches have been landed. Closing bug.
Comment on attachment 67839 [details] Patch with unwanted comments removed Rejecting patch 67839 from commit-queue. Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1 Last 500 characters of output: 1 succeeded at 1 with fuzz 3. patching file WebCore/loader/FrameLoader.cpp Hunk #1 FAILED at 1848. Hunk #2 FAILED at 1936. Hunk #3 FAILED at 2012. 3 out of 3 hunks FAILED -- saving rejects to file WebCore/loader/FrameLoader.cpp.rej patching file WebKit/qt/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp Hunk #1 FAILED at 97. Hunk #2 FAILED at 1289. 2 out of 2 hunks FAILED -- saving rejects to file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp.rej Full output: http://queues.webkit.org/results/4092014
Revision r67878 cherry-picked into qtwebkit-2.1 with commit ab9219b <http://gitorious.org/webkit/qtwebkit/commit/ab9219b>