Bug 41155 – In FrameLoader, m_URL is not set before calling client dispatchDidCommitLoad for cached pages

• Component: New Bugs
• Status: RESOLVED FIXED

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();

[Dinu Jacob]

Created attachment 63924 [details]
Proposed patch with test case

[Dinu Jacob]

reopening as it was closed by mistake..

[Adam Barth]

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.

[Adam Barth]

I think removing FrameLoader::m_URL either recently landed or is about to land.

[Suresh Voruganti]

Any update on the issue? This is one of the issue that blocks Webkit 2.1 release

[Dinu Jacob]

Created attachment 66737 [details]
Patch file with changes

[WebKit Commit Bot]

Comment on attachment 66737 [details]
Patch file with changes

Rejecting patch 66737 from review queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 66737 [details]
Patch file with changes

Rejecting patch 66737 from commit-queue.

[e-mail] 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.

[Dinu Jacob]

(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

[Darin Adler]

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"?

[Dinu Jacob]

Created attachment 66749 [details]
Patch with change in local variable name

Changed local variable ptitle to title as requested.

[Adam Barth]

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?

[Dinu Jacob]

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.

[Dinu Jacob]

Created attachment 66890 [details]
Patch with spacing in comments fixed

[chris fleizach]

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 @@
> [phone]  Jacob Dinu  <[e-mail]>
> +
> +        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

> +

[Dinu Jacob]

Created attachment 67026 [details]
Patch with spelling and grammar corrections

[Adam Barth]

Comment on attachment 67026 [details]
Patch with spelling and grammar corrections

You need to click the patch checkbox for the tools to work properly.

[Adam Barth]

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.

[Dinu Jacob]

(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 ?

[Adam Barth]

Thanks for coordinating with Nate.  Looking now.

[Adam Barth]

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.

[Dinu Jacob]

(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?

[Adam Barth]

Sure

[Dinu Jacob]

Created attachment 67832 [details]
Final patch to be committed

Patch based on latest revision to be committed after review.

[Dinu Jacob]

(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

[Darin Adler]

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.

[Dinu Jacob]

(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

[Adam Barth]

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.

[Dinu Jacob]

(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

[Dinu Jacob]

Created attachment 67838 [details]
Patch with unwanted comments removed

[Dinu Jacob]

Created attachment 67839 [details]
Patch with unwanted comments removed

[Adam Barth]

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.  :)

[Dinu Jacob]

(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 :)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Dinu Jacob]

(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.

[Dinu Jacob]

(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.

[Eric Seidel (no email)]

Comment on attachment 67839 [details]
Patch with unwanted comments removed

Let's just cq+ it again.  It's my fault if the cq hangs. :)

[WebKit Commit Bot]

Comment on attachment 67839 [details]
Patch with unwanted comments removed

Clearing flags on attachment: 67839

Committed r67878: <http://trac.webkit.org/changeset/67878>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

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

[Ademar Reis]

Revision r67878 cherry-picked into qtwebkit-2.1 with commit ab9219b <http://gitorious.org/webkit/qtwebkit/commit/ab9219b>