Bug 13400 - REGRESSION (r20809-20814): No back entry created for navigations created by assigning to document.location
Summary: REGRESSION (r20809-20814): No back entry created for navigations created by a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
: 14147 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-04-18 18:02 PDT by Darin Fisher (:fishd, Google)
Modified: 2007-07-06 04:56 PDT (History)
5 users (show)

See Also:


Attachments
test case (263 bytes, text/html)
2007-04-18 18:04 PDT, Darin Fisher (:fishd, Google)
no flags Details
better test case (980 bytes, text/html)
2007-04-18 18:45 PDT, Darin Fisher (:fishd, Google)
no flags Details
A further simplified attachment (1.27 KB, text/html)
2007-04-20 03:26 PDT, Nikolas Bertheau
no flags Details
window.open test case (206 bytes, text/html)
2007-05-18 18:39 PDT, Matt Perry
no flags Details
simple patch (1.33 KB, patch)
2007-06-15 15:59 PDT, Matt Perry
no flags Details | Formatted Diff | Diff
patch with regression test (4.05 KB, patch)
2007-06-15 16:44 PDT, Matt Perry
mjs: review-
Details | Formatted Diff | Diff
a test that fails with this patch. (1013 bytes, text/html)
2007-06-25 20:06 PDT, Maciej Stachowiak
no flags Details
another difference between Firefox 2 and this patch (382 bytes, text/html)
2007-06-25 20:07 PDT, Maciej Stachowiak
no flags Details
simple patch with slew of tests (9.20 KB, patch)
2007-06-26 16:26 PDT, Matt Perry
mrowe: review-
Details | Formatted Diff | Diff
fix above patch (9.94 KB, patch)
2007-06-27 14:47 PDT, Matt Perry
aroben: review-
Details | Formatted Diff | Diff
small tweak to above patch (9.90 KB, patch)
2007-06-28 17:40 PDT, Matt Perry
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2007-04-18 18:02:28 PDT
No back entry created for navigations created by assigning to document.location

Tested using rev 20934

Test case coming up...
Comment 1 Darin Fisher (:fishd, Google) 2007-04-18 18:04:41 PDT
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!
Comment 2 Darin Fisher (:fishd, Google) 2007-04-18 18:08:03 PDT
Doh, unfortunately you will need to download this testcase to reproduce the bug.  Bugzilla's query params interfere.
Comment 3 Darin Fisher (:fishd, Google) 2007-04-18 18:45:02 PDT
Created attachment 14084 [details]
better test case

This version of the test case should work when hosted on bugzilla.
Comment 4 Darin Fisher (:fishd, Google) 2007-04-19 15:56:55 PDT
This regression was introduced between r20809 (pass) and r20814 (fail).
Comment 5 Darin Fisher (:fishd, Google) 2007-04-19 15:59:32 PDT
Probably caused by:
http://trac.webkit.org/projects/webkit/changeset/20813
Comment 6 Nikolas Bertheau 2007-04-20 03:26:03 PDT
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.
Comment 7 Nikolas Bertheau 2007-04-20 06:25:55 PDT
(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.
Comment 8 David Kilzer (:ddkilzer) 2007-04-20 12:34:19 PDT
(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?

Comment 9 Matt Lilek 2007-04-20 12:41:03 PDT
(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.
Comment 10 Matt Lilek 2007-04-20 12:42:41 PDT
Err....ignore me.
Comment 11 Nikolas Bertheau 2007-04-20 14:33:46 PDT
(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")
Comment 12 Darin Adler 2007-04-23 08:39:30 PDT
<rdar://problem/5153025>
Comment 13 Matt Perry 2007-05-18 18:39:36 PDT
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).
Comment 14 Matt Perry 2007-06-15 15:59:01 PDT
Created attachment 15068 [details]
simple patch

Looks like the problem with setting document.location is just that the userGesture flag wasn't properly tracked.
Comment 15 Darin Adler 2007-06-15 16:07:04 PDT
Comment on attachment 15068 [details]
simple patch

We need a regression test for this.
Comment 16 Matt Perry 2007-06-15 16:44:13 PDT
Created attachment 15072 [details]
patch with regression test

Here's the patch above, but with a regression test this time.
Comment 17 Maciej Stachowiak 2007-06-25 20:06:19 PDT
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!
Comment 18 Maciej Stachowiak 2007-06-25 20:06:53 PDT
Created attachment 15236 [details]
a test that fails with this patch.
Comment 19 Maciej Stachowiak 2007-06-25 20:07:39 PDT
Created attachment 15237 [details]
another difference between Firefox 2 and this patch
Comment 20 Matt Perry 2007-06-26 12:41:30 PDT
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.
Comment 21 Matt Perry 2007-06-26 12:58:15 PDT
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.
Comment 22 Matt Perry 2007-06-26 16:26:07 PDT
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).
Comment 23 Maciej Stachowiak 2007-06-26 21:34:23 PDT
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
Comment 24 Mark Rowe (bdash) 2007-06-26 22:41:00 PDT
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.
Comment 25 Matt Perry 2007-06-27 14:47:52 PDT
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.
Comment 26 Adam Roben (:aroben) 2007-06-27 21:00:45 PDT
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?
Comment 27 Matt Perry 2007-06-28 17:40:27 PDT
Created attachment 15301 [details]
small tweak to above patch

Nuked the userGesture var.
Comment 28 Adam Roben (:aroben) 2007-07-02 19:08:22 PDT
Comment on attachment 15301 [details]
small tweak to above patch

r=me
Comment 29 Mark Rowe (bdash) 2007-07-04 23:10:33 PDT
Landed in r24004.
Comment 30 Maxime BRITTO 2007-07-06 04:56:55 PDT
*** Bug 14147 has been marked as a duplicate of this bug. ***