Bug 37126

Summary: REGRESSION (r56223): http://www.playerpress.com/ shows up twice in the back/forward list.
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: Darin Fisher (:fishd, Google) <fishd>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, beidson, darin, eric, fishd, ian, manyoso, ossy, tonikitoo, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
testcase: frame.src assignment after appendChild
none
testcase: frame.contentWindow.location assignment after appendChild
none
v1 patch
beidson: review+
v2 patch: extra comment beidson: review+, fishd: commit-queue+

Description Brady Eidson 2010-04-05 16:33:43 PDT
REGRESSION (r56223): http://www.playerpress.com/ shows up twice in the back/forward list.

In fixing https://bugs.webkit.org/show_bug.cgi?id=9166, http://trac.webkit.org/changeset/56223 caused this change.

Simply navigating to http://www.playerpress.com/ causes it to show up in the back/forward list twice.  
Presumably some iframe on the page is setting its .src attribute.

Darin mentioned in the r56223 Changelog that this is to match HTML5's <iframe> spec, but the original bug was about iframe.src affecting global history (visited link coloring), not the session history.

CC'ing him to unravel what he thinks 4.2.8 actually says about this (reading that section on "process the iframe attributes" and how it interacts with setting .src makes my brain hurt).

Whether or not we decide HTML5 says this should be in the back list, I don't think it should be because no other browser does that!
Comment 1 Brady Eidson 2010-04-05 16:34:40 PDT
In radar as <rdar://problem/7825681>
Comment 2 Brady Eidson 2010-04-05 18:28:34 PDT
Another example is with NPR stories (which requires visiting an initial site to follow these steps):

1 - Visit google.com
2 - Visit http://www.npr.org/templates/story/story.php?storyId=125491180
3 - The back list contains only google.
4 - Go back.
5 - The forward list only contains http://www.npr.org/templates/story/story.php?storyId=125491180
6 - Go forward
7 - The back list contains http://www.google.com/ and a duplicate entry for http://www.npr.org/templates/story/story.php?storyId=125491180
8 - Click back
9 - ASSERT.  FrameLoader compares the current URL and the back item URL, which are both http://www.npr.org/templates/story/story.php?storyId=125491180, and determines this must be a same document navigation.  But it's not.
Comment 3 Brady Eidson 2010-04-05 18:32:14 PDT
LayoutTests/fast/loader/frame-src-change-added-to-history.html used the back/forward list to confirm the passing of the test, which means Darin thought session history was the intended affectee of the change.

I really don't think this is right.  I'm going to dig in to 4.2.8 and see if I can find out if it really specifies this.
Comment 4 Brady Eidson 2010-04-05 19:20:50 PDT
(In reply to comment #3)
> LayoutTests/fast/loader/frame-src-change-added-to-history.html used the
> back/forward list to confirm the passing of the test, which means Darin thought
> session history was the intended affectee of the change.
> 
> I really don't think this is right.  I'm going to dig in to 4.2.8 and see if I
> can find out if it really specifies this.

The layout test does show a difference in behavior between Safari 4.0.5 and Opera/Firefox, and brings ToT WebKit inline with what they do.  IE8 completely breaks with the test, making the Opera and Firefox compatibility much more interesting.

But r56223 caused some other change to break playerpress.com, npr articles, and possibly others.  Neither the PP.com or NPR.org bugs reproduce in Firefox or Opera.
Comment 5 Brady Eidson 2010-04-05 19:25:57 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > LayoutTests/fast/loader/frame-src-change-added-to-history.html used the
> > back/forward list to confirm the passing of the test, which means Darin thought
> > session history was the intended affectee of the change.
> > 
> > I really don't think this is right.  I'm going to dig in to 4.2.8 and see if I
> > can find out if it really specifies this.
> 
> The layout test does show a difference in behavior between Safari 4.0.5 and
> Opera/Firefox, and brings ToT WebKit inline with what they do.  IE8 completely
> breaks with the test, making the Opera and Firefox compatibility much more
> interesting.
> 
> But r56223 caused some other change to break playerpress.com, npr articles, and
> possibly others.  Neither the PP.com or NPR.org bugs reproduce in Firefox or
> Opera.

Correction - The PP.com bug and NPR article bug both reproduce in Opera.

Interesting...
Comment 6 Brady Eidson 2010-04-05 19:32:06 PDT
IE8 doesn't misbehave with either site.

But I don't know if that's important/relevant to this discussion because it does weird things on related tests.

Still looking for someone who has the IE9 beta to try (Still might not be relevant) 

If the spec really does say this is correct behavior (and my reading of it suggests it does), then it's a split with most of the marketshare right now.  Is it better?
Comment 7 Brady Eidson 2010-04-05 19:37:04 PDT
I'm not sure what I think about this one anymore.
Comment 8 Brady Eidson 2010-04-07 08:36:14 PDT
The change made a test case that works in other browsers now pass in WebKit. Notably, IE8 doesn't pass the test.

But the change also caused a real-world compatibility problem at real websites - a problem that IE8 and Firefox both don't have.

Unless someone can point out real-world compatibility gained by this change, and demonstrate that it is better than the compatibility lost by this change, I'm strongly leaning towards reverting the change until it can be done again, fixing the original bug while not introducing this new regression.
Comment 9 Darin Fisher (:fishd, Google) 2010-04-07 08:43:23 PDT
Yes, my objective in fixing bug 9166 was to affect session history as well as global history.  Although I don't have strong opinions about whether or not it affects global history.

There are sites like www.transact-online.co.uk that were fixed by r56223.  See http://code.google.com/p/chromium/issues/detail?id=8011#c3 for details about how to reproduce the problem.

Brady, do you have a reduction for this issue yet?  I'm going to start working on that shortly.
Comment 10 Darin Fisher (:fishd, Google) 2010-04-07 08:45:41 PDT
I cannot reproduce the issue in #2 using Chrome, but I can reproduce the issue with www.playerpress.com.
Comment 11 Brady Eidson 2010-04-07 08:55:58 PDT
(In reply to comment #9) 
> Brady, do you have a reduction for this issue yet?  I'm going to start working
> on that shortly.

I'll start reducing playerpress.com now.

(In reply to comment #10)
> I cannot reproduce the issue in #2 using Chrome, but I can reproduce the issue
> with www.playerpress.com.

That's very curious.
Comment 12 Darin Fisher (:fishd, Google) 2010-04-07 09:59:26 PDT
In the case of http://www.playerpress.com/, what I observe is page navigation completes, and then sometime later an iframe is navigated to /xd_receiver.html.  Sometimes that navigation occurs before the main page navigation completes, which prevents the bug from being visible.  However, that is not the common case for me.

This is a hidden iframe that appears to be integral to facebook connect.  It looks like it is used to communicate from facebook back to www.playerpress.com.

I need to look further to see how the iframe is constructed.
Comment 13 Darin Fisher (:fishd, Google) 2010-04-07 10:28:13 PDT
More details:

Page loading has ended, and an iframe that points to /xd_receiver.html#foo is navigated via "src" assignment to /xd_receiver.html#bar.  Because of r56223, this is counted as a new navigation.

I'm still not sure how to reduce this properly because given a simple test based on what I described above, both WebKit and Firefox agree on behavior.  So, there must be some other subtlety that I'm not yet seeing.
Comment 14 Darin Fisher (:fishd, Google) 2010-04-07 10:53:40 PDT
Please ignore comment #13.  I was wrong.  It is not changing the reference fragment.  It is navigating from about:blank.

Here's the facebook code that creates the hidden iframe:

    _createHiddenIFrame: function (b) {
        if (FB.FBDebug.logLevel > 4) FB.FBDebug.writeLine('Create iframe ' + b + ' in ' + document.URL);
        var a;
        a = document.createElement('iframe');
        a.className = 'FB_RECEIVER_DOM';
        if (!this._iframeCreated && FBIntern.AppInfo.get_singleton().get_hostInfo().get_hostName() === FBIntern.HostName.IE) {
            a.src = 'javascript:false';
            this._iframeCreated = true;
        }
        if (FBIntern.AppInfo.get_singleton().get_hostInfo().get_hostName() === FBIntern.HostName.IE) {
            a.src = b;
            a = FB.HiddenContainer.get().appendChild(a);
        } else {
            a = FB.HiddenContainer.get().appendChild(a);
            a.src = b;
        }
        return a;
    },

I think a possible fix for us, and perhaps what Firefox is doing, is to keep history locked when navigating from about:blank.
Comment 15 Brady Eidson 2010-04-07 10:55:17 PDT
(In reply to comment #14)
> Please ignore comment #13.  I was wrong.  It is not changing the reference
> fragment.  It is navigating from about:blank.
> ...
> I think a possible fix for us, and perhaps what Firefox is doing, is to keep
> history locked when navigating from about:blank.

I think that sounds pretty reasonable.

If it works, we should consider getting that specifically into HTML5 (if it's not already there in some roundabout way that we both missed)
Comment 16 Darin Fisher (:fishd, Google) 2010-04-07 11:32:52 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Please ignore comment #13.  I was wrong.  It is not changing the reference
> > fragment.  It is navigating from about:blank.
> > ...
> > I think a possible fix for us, and perhaps what Firefox is doing, is to keep
> > history locked when navigating from about:blank.
> 
> I think that sounds pretty reasonable.
> 
> If it works, we should consider getting that specifically into HTML5 (if it's
> not already there in some roundabout way that we both missed)

I think the solution may need to be a bit more subtle, which is why I haven't posted a patch yet.  What I think we really need to know is 1) are we navigating away from about:blank and 2) has the iframe been navigated anywhere else?

That is, if the iframe is newly constructed and has only loaded about:blank, then I think the next navigation should have lockHistory/lockBackForwardHistory set to true.

I am actually struggling a bit to see how best to implement that condition in WebCore.  FrameLoader has a committedFirstRealDocumentLoad() method that looks like it should be applicable here, but sadly that is set to true as a byproduct of FrameLoader::requestFrame("about:blank").  That may actually be a bug in itself.  Or, perhaps we should have a requestFrameWithEmptyDocument method to make things more clear.
Comment 17 Darin Fisher (:fishd, Google) 2010-04-07 23:28:01 PDT
Created attachment 52834 [details]
testcase: frame.src assignment after appendChild
Comment 18 Darin Fisher (:fishd, Google) 2010-04-07 23:29:18 PDT
Created attachment 52835 [details]
testcase: frame.contentWindow.location assignment after appendChild
Comment 19 Darin Fisher (:fishd, Google) 2010-04-13 15:50:50 PDT
Created attachment 53291 [details]
v1 patch
Comment 20 Brady Eidson 2010-04-13 16:04:27 PDT
Comment on attachment 53291 [details]
v1 patch

At this point the variable name "m_quickRedirectComing" is silly  :)

Do you see us adding more logic to currentItemShouldBeReplaced() in the future?  The name slightly bothers me because it doesn't obviously match the conditions being tested.

Are we sure we shouldn't adopt this new behavior in the (problematic but still quite real) cases where we have no m_currentItem?

r+ with a good answer to the m_currentItem question.
Comment 21 Darin Fisher (:fishd, Google) 2010-04-13 16:21:19 PDT
(In reply to comment #20)
> (From update of attachment 53291 [details])
> At this point the variable name "m_quickRedirectComing" is silly  :)

Probably true.  I would like to clean that up.

m_navigateWithReplacementEnabled comes to mind, but I will defer such cleanup to a separate cleanup-only patch.


> Do you see us adding more logic to currentItemShouldBeReplaced() in the future?
>  The name slightly bothers me because it doesn't obviously match the conditions
> being tested.

I don't intend to add more cases to currentItemShouldBeReplaced.  Locally, I have added a clarifying comment that quotes the HTML5 spec.

It was hard to come up with a good name for this.  The name is meant to convey that this is about determining if "navigation" should happen with "replacement enabled" to borrow terms from the HTML5 spec.


> Are we sure we shouldn't adopt this new behavior in the (problematic but still
> quite real) cases where we have no m_currentItem?

I am also concerned about the cases where m_currentItem may be null.  I have it on my list to eliminate cases where m_currentItem may be null.  In fact, I think the m_documentLoader null check in FrameLoader::clientRedirected should really be a null check of history()->currentItem(), but that change breaks a layout test.  I suggest this change because it should only make sense to perform a navigation with replacement enabled if there is a current history item to replace!
Comment 22 Darin Fisher (:fishd, Google) 2010-04-13 16:23:13 PDT
Brady, please also note that my goal here is to be as incremental as possible.  I expect to iterate on this clientRedirected function and related code in the near term.
Comment 23 Darin Fisher (:fishd, Google) 2010-04-13 16:26:13 PDT
Created attachment 53296 [details]
v2 patch: extra comment
Comment 24 Brady Eidson 2010-04-13 16:26:36 PDT
(In reply to comment #22)
> Brady, please also note that my goal here is to be as incremental as possible. 
> I expect to iterate on this clientRedirected function and related code in the
> near term.

Oh, totally.  I was just probing the future, making sure such incremental plans were in place :)
Comment 25 Brady Eidson 2010-04-13 16:27:24 PDT
Comment on attachment 53296 [details]
v2 patch: extra comment

I feel a little better about this one, even.

r+
Comment 26 Darin Fisher (:fishd, Google) 2010-04-13 21:52:56 PDT
Landed as http://trac.webkit.org/changeset/57558
Comment 27 Eric Seidel (no email) 2010-04-13 22:29:02 PDT
Looks to have broken tiger.  http://build.webkit.org/results/Tiger%20Intel%20Release/r57558%20(10787)/results.html

Sorry the cq was down this afternoon during the hackfest.  It's back now.
Comment 28 WebKit Review Bot 2010-04-13 22:46:27 PDT
http://trac.webkit.org/changeset/57558 might have broken Tiger Intel Release
Comment 29 Eric Seidel (no email) 2010-04-13 22:48:17 PDT
Qt looks affected too:
http://build.webkit.org/results/Qt%20Linux%20Release/r57558%20(10053)/results.html
Comment 30 Csaba Osztrogon√°c 2010-04-13 23:09:54 PDT
(In reply to comment #29)
> Qt looks affected too:
> http://build.webkit.org/results/Qt%20Linux%20Release/r57558%20(10053)/results.html

It is a known issue, and skipped by http://trac.webkit.org/changeset/57560 ( https://bugs.webkit.org/show_bug.cgi?id=36392 ). Thx for it, Darin.