Bug 37126 – REGRESSION (r56223): http://www.playerpress.com/ shows up twice in the back/forward list.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

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!

[Brady Eidson]

In radar as <rdar://problem/7825681>

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

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?

[Brady Eidson]

I'm not sure what I think about this one anymore.

[Brady Eidson]

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.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

I cannot reproduce the issue in #2 using Chrome, but I can reproduce the issue with www.playerpress.com.

[Brady Eidson]

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

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Brady Eidson]

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

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Created attachment 52834 [details]
testcase: frame.src assignment after appendChild

[Darin Fisher (:fishd, Google)]

Created attachment 52835 [details]
testcase: frame.contentWindow.location assignment after appendChild

[Darin Fisher (:fishd, Google)]

Created attachment 53291 [details]
v1 patch

[Brady Eidson]

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.

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 53296 [details]
v2 patch: extra comment

[Brady Eidson]

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

[Brady Eidson]

Comment on attachment 53296 [details]
v2 patch: extra comment

I feel a little better about this one, even.

r+

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/57558

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57558 might have broken Tiger Intel Release

[Eric Seidel (no email)]

Qt looks affected too:
http://build.webkit.org/results/Qt%20Linux%20Release/r57558%20(10053)/results.html

[Csaba Osztrogonác]

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