Bug 13400 – REGRESSION (r20809-20814): No back entry created for navigations created by assigning to document.location

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Darin Fisher (:fishd, Google)]

No back entry created for navigations created by assigning to document.location

Tested using rev 20934

Test case coming up...

[Darin Fisher (:fishd, Google)]

Created attachment 14083 [details]
test case

Load this page and click the "go to next page", which navigates back to itself w/ modifications to the query string to result in a unique URL.  Notice that no back entry is created for these navigations!

[Darin Fisher (:fishd, Google)]

Doh, unfortunately you will need to download this testcase to reproduce the bug.  Bugzilla's query params interfere.

[Darin Fisher (:fishd, Google)]

Created attachment 14084 [details]
better test case

This version of the test case should work when hosted on bugzilla.

[Darin Fisher (:fishd, Google)]

This regression was introduced between r20809 (pass) and r20814 (fail).

[Darin Fisher (:fishd, Google)]

Probably caused by:
http://trac.webkit.org/projects/webkit/changeset/20813

[Nikolas Bertheau]

Created attachment 14107 [details]
A further simplified attachment

document.location doesn't exist, but the document part can safely be ignored.
location is an object, not a variable. Normally, an assignment like location=URL should produce an error.

[Nikolas Bertheau]

(In addition to comment #6)
Okay, I must admit that document.location does exist and location=... is an appropriate alternative to location.href=...
All four buttons should work like the first two.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (In addition to comment #6)
> Okay, I must admit that document.location does exist and location=... is an
> appropriate alternative to location.href=...
> All four buttons should work like the first two.

What do other browsers (like MSIE 6/7 and Firefox 2.0.x) do with this test?

[Matt Lilek]

(In reply to comment #8)
> 
> What do other browsers (like MSIE 6/7 and Firefox 2.0.x) do with this test?
> 

Firefox 2 and IE 7 on Vista both redirect to webkit.org for all 4 buttons.

[Matt Lilek]

Err....ignore me.

[Nikolas Bertheau]

(In reply to comment #8)
> What do other browsers (like MSIE 6/7 and Firefox 2.0.x) do with this test?

Gecko:
http://developer.mozilla.org/en/docs/DOM:document.location
http://developer.mozilla.org/en/docs/DOM:window.location
(i.e., "you can also assign to this property to load another URL")

[Darin Adler]

<rdar://problem/5153025>

[Matt Perry]

Created attachment 14617 [details]
window.open test case

I believe this bug also occurs when doing a window.open in an onload handler.  The initial navigation is not stored in session history.  Note that if you do window.open in an onclick handler (in the attached test case, click on the text), the nav is stored.  I believe this is due to the difference in handling of userGestured vs non-userGestured loads (the latter is done as FrameLoadTypeInternal).

[Matt Perry]

Created attachment 15068 [details]
simple patch

Looks like the problem with setting document.location is just that the userGesture flag wasn't properly tracked.

[Darin Adler]

Comment on attachment 15068 [details]
simple patch

We need a regression test for this.

[Matt Perry]

Created attachment 15072 [details]
patch with regression test

Here's the patch above, but with a regression test this time.

[Maciej Stachowiak]

Comment on attachment 15072 [details]
patch with regression test

Good catch, but I don't think this is quite the right fix. A location assignment should add a new history item even if off a timer (at least if it's not within a short time period of the original page load. See modified-testcase.html.

In Firefox, it appears that assigning document.location in the normal way *always* creates a new history item. See even-immediate-assignment.html. Given this, I think the right thing is probably to always pass true as the userGesture argument, but you may want to test other similar cases and add them as LayoutTests if you think of other corner cases.

Otherwise this patch looks great!

[Maciej Stachowiak]

Created attachment 15236 [details]
a test that fails with this patch.

[Maciej Stachowiak]

Created attachment 15237 [details]
another difference between Firefox 2 and this patch

[Matt Perry]

Interesting.  Firefox appears to add a history item in every case EXCEPT when document.location is directly assigned in an onload handler.  If it occurs through a timeout or user gestures, a history item is added.  My guess is that a direct assignment in onload is treated as a client redirect and not added to history.

We can get close to this behavior if we just force userGesture = true here (that is, always pass false for lockHistory and true for userGesture).  But, in that case, we'd still add a history item for a direct assignment in onload.  Unfortunately, if we do that, we end up failing/hitting asserts in several layout tests.

I'll have to look at this more.

[Matt Perry]

Actually, I take that back.  I'm hitting asserts and various errors when I run the layout tests, but (a) none of the tests fail, and (b) it happens without this patch applied.  Maybe something else about my checkout is hosed.

[Matt Perry]

Created attachment 15260 [details]
simple patch with slew of tests

As Maciej suggested, I went with just forcing userGesture=true.

This does introduce a difference with Firefox: doing something like onload="document.location = bla" does not create a back/forward item in Firefox, but will for us.  I'm not sure whether that's a problem.  It seems like overall this is an improvement (this brings us closer to Firefox in other ways).

[Maciej Stachowiak]

Comment on attachment 15260 [details]
simple patch with slew of tests

Awesome, I love the tests, could you please file a bug on the remaining small difference from Firefox behavior? I do think we want such cases handled as a client redirect. It would also be good to check if inline scripts that don't set a timer and happen before onload have this behavior, I suspect they do.

r=me

[Mark Rowe (bdash)]

Comment on attachment 15260 [details]
simple patch with slew of tests

You're missing a ChangeLog for your WebCore changes, and your layout test results have hard-coded paths which are specific to your machine.  I believe navigation tests need to be HTTP tests (http/tests/navigation) so that they will have consistent paths across machines.

[Matt Perry]

Created attachment 15278 [details]
fix above patch

Same as above patch, but
- moved the tests to http/tests/navigation
- included changes to WebCore/ChangeLog
as bdash suggested.

[Adam Roben (:aroben)]

Comment on attachment 15278 [details]
fix above patch

+    bool userGesture = true;
+    frame->loader()->scheduleLocationChange(str, activeFrame->loader()->outgoingReferrer(), !userGesture, userGesture);

There's no need for the userGesture variable anymore. Can you remove it?

[Matt Perry]

Created attachment 15301 [details]
small tweak to above patch

Nuked the userGesture var.

[Adam Roben (:aroben)]

Comment on attachment 15301 [details]
small tweak to above patch

r=me

[Mark Rowe (bdash)]

Landed in r24004.

[Maxime BRITTO]

*** Bug 14147 has been marked as a duplicate of this bug. ***