Bug 41155 - In FrameLoader, m_URL is not set before calling client dispatchDidCommitLoad for cached pages
Summary: In FrameLoader, m_URL is not set before calling client dispatchDidCommitLoad ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Dinu Jacob
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-06-24 07:06 PDT by nokiabugz
Modified: 2010-09-20 13:51 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch with test case (3.67 KB, patch)
2010-08-09 13:25 PDT, Dinu Jacob
abarth: review-
Details | Formatted Diff | Diff
Patch file with changes (4.39 KB, patch)
2010-09-07 10:35 PDT, Dinu Jacob
no flags Details | Formatted Diff | Diff
Patch with change in local variable name (4.62 KB, patch)
2010-09-07 12:28 PDT, Dinu Jacob
no flags Details | Formatted Diff | Diff
Patch with spacing in comments fixed (4.62 KB, patch)
2010-09-08 06:38 PDT, Dinu Jacob
cfleizach: review-
cfleizach: commit-queue-
Details | Formatted Diff | Diff
Patch with spelling and grammar corrections (4.62 KB, patch)
2010-09-09 06:13 PDT, Dinu Jacob
no flags Details | Formatted Diff | Diff
Final patch to be committed (4.59 KB, patch)
2010-09-16 13:23 PDT, Dinu Jacob
no flags Details | Formatted Diff | Diff
Patch with unwanted comments removed (4.48 KB, patch)
2010-09-16 13:49 PDT, Dinu Jacob
no flags Details | Formatted Diff | Diff
Patch with unwanted comments removed (4.48 KB, patch)
2010-09-16 13:51 PDT, Dinu Jacob
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nokiabugz 2010-06-24 07:06:51 PDT
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.
Comment 1 nokiabugz 2010-06-24 07:30:47 PDT
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();
Comment 2 Dinu Jacob 2010-08-09 13:25:21 PDT
Created attachment 63924 [details]
Proposed patch with test case
Comment 3 Dinu Jacob 2010-08-09 13:30:46 PDT
reopening as it was closed by mistake..
Comment 4 Adam Barth 2010-08-14 23:32:27 PDT
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.
Comment 5 Adam Barth 2010-08-30 12:02:37 PDT
I think removing FrameLoader::m_URL either recently landed or is about to land.
Comment 6 Suresh Voruganti 2010-09-07 06:52:45 PDT
Any update on the issue? This is one of the issue that blocks Webkit 2.1 release
Comment 7 Dinu Jacob 2010-09-07 10:35:41 PDT
Created attachment 66737 [details]
Patch file with changes
Comment 8 WebKit Commit Bot 2010-09-07 10:37:13 PDT
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 9 WebKit Commit Bot 2010-09-07 10:39:21 PDT
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.
Comment 10 Dinu Jacob 2010-09-07 10:42:42 PDT
(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 11 Darin Adler 2010-09-07 11:08:18 PDT
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"?
Comment 12 Dinu Jacob 2010-09-07 12:28:12 PDT
Created attachment 66749 [details]
Patch with change in local variable name

Changed local variable ptitle to title as requested.
Comment 13 Adam Barth 2010-09-07 13:52:46 PDT
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 14 Dinu Jacob 2010-09-08 06:29:03 PDT
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.
Comment 15 Dinu Jacob 2010-09-08 06:38:10 PDT
Created attachment 66890 [details]
Patch with spacing in comments fixed
Comment 16 chris fleizach 2010-09-09 00:56:00 PDT
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

> +
Comment 17 Dinu Jacob 2010-09-09 06:13:08 PDT
Created attachment 67026 [details]
Patch with spelling and grammar corrections
Comment 18 Adam Barth 2010-09-09 06:28:14 PDT
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 19 Adam Barth 2010-09-09 06:29:14 PDT
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.
Comment 20 Dinu Jacob 2010-09-15 13:37:21 PDT
(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 ?
Comment 21 Adam Barth 2010-09-15 14:14:47 PDT
Thanks for coordinating with Nate.  Looking now.
Comment 22 Adam Barth 2010-09-15 14:20:28 PDT
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.
Comment 23 Dinu Jacob 2010-09-16 06:38:35 PDT
(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?
Comment 24 Adam Barth 2010-09-16 09:03:22 PDT
Sure
Comment 25 Dinu Jacob 2010-09-16 13:23:27 PDT
Created attachment 67832 [details]
Final patch to be committed

Patch based on latest revision to be committed after review.
Comment 26 Dinu Jacob 2010-09-16 13:24:59 PDT
(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 27 Darin Adler 2010-09-16 13:30:02 PDT
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.
Comment 28 Dinu Jacob 2010-09-16 13:33:50 PDT
(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 29 Adam Barth 2010-09-16 13:36:25 PDT
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.
Comment 30 Dinu Jacob 2010-09-16 13:38:34 PDT
(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
Comment 31 Dinu Jacob 2010-09-16 13:49:19 PDT
Created attachment 67838 [details]
Patch with unwanted comments removed
Comment 32 Dinu Jacob 2010-09-16 13:51:50 PDT
Created attachment 67839 [details]
Patch with unwanted comments removed
Comment 33 Adam Barth 2010-09-16 15:42:24 PDT
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.  :)
Comment 34 Dinu Jacob 2010-09-16 16:50:25 PDT
(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 35 Eric Seidel 2010-09-17 12:44:21 PDT
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 36 Eric Seidel 2010-09-18 03:21:11 PDT
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 37 Eric Seidel 2010-09-18 03:21:16 PDT
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.
Comment 38 Dinu Jacob 2010-09-20 10:23:41 PDT
(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.
Comment 39 Dinu Jacob 2010-09-20 10:29:02 PDT
(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 40 Eric Seidel 2010-09-20 13:03:09 PDT
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 41 WebKit Commit Bot 2010-09-20 13:20:39 PDT
Comment on attachment 67839 [details]
Patch with unwanted comments removed

Clearing flags on attachment: 67839

Committed r67878: <http://trac.webkit.org/changeset/67878>
Comment 42 WebKit Commit Bot 2010-09-20 13:20:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 WebKit Commit Bot 2010-09-20 13:23:28 PDT
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
Comment 44 Ademar Reis 2010-09-20 13:51:04 PDT
Revision r67878 cherry-picked into qtwebkit-2.1 with commit ab9219b <http://gitorious.org/webkit/qtwebkit/commit/ab9219b>