Summary: | Session history should skip over JS redirects | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||||||||||||||
Component: | History | Assignee: | Mihai Parparita <mihaip> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, anantha, ap, beidson, benm, commit-queue, darin, eric, fishd, ggaren, joenotcharles, mjs, mpComplete, pkasting, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
URL: | http://persistent.info/webkit/test-cases/redirects/page1.html | ||||||||||||||||||
Bug Depends on: | 43292 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Mihai Parparita
2010-07-22 17:42:42 PDT
Is this desired behavior supported by the HTML5 spec? If not, should we recommend that it be amended for this? Is this what other browsers, IE and Firefox, do? As best as I can tell, http://www.w3.org/TR/html5/history.html does not mention this behavior. It alludes to something similar in section 5.4.4 though: "In addition, a user agent could ignore calls to pushState() that are invoked on a timer, or from event listeners that are not triggered in response to a clear user action, or that are invoked in rapid succession." I tested Firefox 3.6 (Mac) and IE 8 (Windows 7), and both handle the hp.com case correctly, but not the persistent.info test case. They may special-case programatic fragment navigation (vs. whole page navigation). I'll see if I can come up with another test case, and see how they behave on the bugs that http://crrev.com/18373 was trying to fix (http://crbug.com/11896 and http://crbug.com/12820) Here's some more systematic test cases: persistent.info/webkit/test-cases/redirects/fragment.html?<timeout> persistent.info/webkit/test-cases/redirects/page.html?<timeout> Where <timeout> is the timeout in milliseconds before navigating to the destination (by setting window.location). In the case of fragment.html that's #foo, in the case of page.html that's destination.html. If the timeout param 0, the navigation is done onload (with no setTimeout call). The test was to visit google.com, then the page, wait for the redirect to complete, and press back. A result of "g.com" means that google.com was shown, while "page" means that the interstitial page was shown. Chrome WebKit Firefox IE 8 fragment onload g.com page g.com g.com 200ms g.com page page g.com 1000ms g.com page page g.com 5000ms g.com page page g.com 6000ms page page page g.com page onload g.com page g.com page 200ms g.com page page page 1000ms g.com page page page 5000ms page page page page 6000ms page page page page Observations: - No browser other than Chrome has time-dependent behavior. - All browsers (except for IE 8) handle fragment and page navigation the same - There is precedent for modifying WebKit to replace (session) history entries for JS redirects during onload. I don't know all the details, but I think that WebCore logic is to skip over redirects that happen within 1 second, regardless of whether they are caused by JS or not. Alexy: Can I get a pointer to where in the code that should be happening? Based on my tests (see comment 3) I'm not seeing that behavior, but perhaps I'm not testing the same thing (e.g. I haven't looked into <meta> refresh redirects). The words to search source code for are "lockBackForwardList" and "quickRedirect". As evidenced by FIXMEs around them, the logic is tangled, but it looks like it should be possible to explore with moderate effort. Specifically, the 1 second cutoff is implemented in RedirectScheduler::scheduleRedirect(): // We want a new history item if the refresh timeout is > 1 second. if (!m_redirect || delay <= m_redirect->delay()) schedule(new ScheduledRedirect(delay, url, true, delay <= 1, false)); But it may become impossible to understand the logic if the Chromium behavior is added to WebCore without refactoring what we have now. That's what I'm trying to avoid. I agree that Chromium behavior should not be ported wholesale here, and that a simplification may be in order. I was tracing the history of the "don't add a session history entry if the redirect delay is <= 1 second" logic, and it looks like it was refactored in http://trac.webkit.org/changeset/24353. Both that change and http://trac.webkit.org/changeset/24367 mention rdar://problem/5339292 as a bug that describes a cleanup of JS navigation logic to be more like the redirection one. Is there anything else relevant in that bug? Created attachment 62634 [details]
Patch
I've uploaded a strawman proposal that implements the a (hopefully) minimally invasive change: also lock the back forward list if the location change happens before the onload event and is not in response to a user gesture. The "user gesture" test was added to handle this test case: http://persistent.info/webkit/test-cases/redirects/delayed-onload.php. The initial page contents (up to the link) are flushed but the page doesn't finish loading for 10 seconds. Since the user has time to see the link and click on it, a new history entry should be generate for this page. With this patch, the table that I generated in comment 3 looks like this: Chrome WebKit Firefox IE 8 fragment onload g.com g.com g.com g.com 200ms g.com page page g.com 1000ms g.com page page g.com 5000ms g.com page page g.com 6000ms page page page g.com page onload g.com g.com g.com page 200ms g.com page page page 1000ms g.com page page page 5000ms page page page page 6000ms page page page page Additionally, pressing back from after going to http://ww.hp.com/ (which JS redirects to http://www.hp.com/#Product during onload) works as expected. At Darin's suggestion, I modified delayed-onload.php to also do a JS redirect inline (not in the onload handler). Here's the results of going to google.com, navigating to the page, waiting for the redirect, and then pressing back for different query values: Chrome WebKit Firefox IE -1 (no timeout, user clicks on link) page page page page 0 (navigate inline) g.com page g.com page 200 (navigate after 200ms) g.com page page page 6000 (navigate after 6000ms) g.com page page page (with my patch, the WebKit results would be page/g.com/g.com/g.com) Interestingly, Firefox still generates a session history entry for JS redirects in timeouts that occur before the page finishes loading (but based on the tests in comment 3, JS redirects during onload don't create one). delayed-onload.php delays the page itself finishing loading (so that both DOMContentLoaded and load occur in ~10 seconds). Here's another test, where the DOM should load immediately (firing DOMContentLoaded), but there's a referenced image that takes 10 seconds to load (firing load): http://persistent.info/webkit/test-cases/redirects/delayed-load.html Chrome WebKit Firefox 0 (navigate during DOMContentLoaded) g.com page page 200 (navigate after 200ms) g.com page page 6000 (navigate after 6000ms) page page page (IE 8 doesn't support DOMContentLoaded; with my patch the WebKit results would be g.com/g.com/g.com) Basically, it looks like Firefox doesn't create a new history entry if the JS redirect happens inline, or if it's during an onload (but not DOMContentLoaded) handler, whereas my patch doesn't create a new history entry for any JS redirects until onload has finished running. As a web developer, I find that simpler to understand, but if Darin (or anyone else) think that matching the Firefox behavior is better for web compat, I can do that too. Another test that Darin mentioned was to look at navigations caused by JS form submits, since there have been WebKit bugs with history generated by them in the past (http://trac.webkit.org/changeset/45606, http://trac.webkit.org/changeset/25410, https://webkit.org/b/32383) Chrome WebKit Firefox IE 0 (submit during onload) page page page page 200 (submit after 200ms) page page page page 6000 (submit after 6000ms) page page page page With my patch, the WebKit behavior is g.com/page/page. Given that all the other browsers always create a history entry, it seems prudent to not change WebKit's behavior here (i.e. don't trigger the check that I added to mustLockBackForwardList when it's called from scheduleFormSubmission). (In reply to comment #10) > Basically, it looks like Firefox doesn't create a new history entry if > the JS redirect happens inline, or if it's during an onload (but not > DOMContentLoaded) handler, whereas my patch doesn't create a new history > entry for any JS redirects until onload has finished running. As a web > developer, I find that simpler to understand, but if Darin (or anyone > else) think that matching the Firefox behavior is better for web compat, > I can do that too. Thanks for investigating this! Interesting, so it seems that Firefox is not taking user gesture into account. This may be something worth posting to whatwg about for discussion, but I'm happy to proceed with your patch. It seems reasonable and easy enough for web developers to understand. Adding Geoffrey Garen to the cc list, since I noticed that a lot of the LayoutTest failures that I'm seeing with this patch are actually tests that he added in http://trac.webkit.org/changeset/40331 and http://trac.webkit.org/changeset/40424. The expectation was that the tests would fail, but they now pass. I see that as a good thing (and will update the expectations), but I was wondering if Geoffrey had any pointers (e.g. other bugs that this patch might fix too). This sounds related to bugs 30224 (actually may be a duplicate) and 41670. (In reply to comment #14) > This sounds related to bugs 30224 (actually may be a duplicate) and 41670. Thanks for the pointers. This does fix bug 30224 and bug 41670 (in that the back button behavior is correct for both http://app.showtime-app.com and the mobile WIkipedia redirect). I can mark this one as a dupe of one of them, but I'd prefer to keep the IE/Firefox/Chrome/Safari tests that I did more visible than hidden behind a dupe link. Created attachment 62906 [details]
Patch v1
Attached complete patch with update expectations and a new test for the user gesture behavior for Darin to review. There are still some tests in http/tests/history that have a FAIL expectation, e.g. redirect-js-location-0-seconds. They navigate after onload (either 0 or 2 seconds), which seemed safer not to modify session history behavior for (though Chrome doesn't add session history entries for JS redirects up to 5 seconds after onload, no other browser does this). Comment on attachment 62906 [details] Patch v1 WebCore/loader/RedirectScheduler.cpp:251 + // See https://webkit.org/b/42861 for the original motivationf for this. nit: motivationf -> motivation LayoutTests/fast/history/gesture-before-onload.html:21 + document.getElementById('manual-explanation').style.display = ''; nit: indentation otherwise, r=me (nice work!) Created attachment 63071 [details]
Patch v2
(In reply to comment #18) > (From update of attachment 62906 [details]) > WebCore/loader/RedirectScheduler.cpp:251 > + // See https://webkit.org/b/42861 for the original motivationf for this. > nit: motivationf -> motivation Fixed. > LayoutTests/fast/history/gesture-before-onload.html:21 > + document.getElementById('manual-explanation').style.display = ''; > nit: indentation Fixed. Patch v2 should have both of those changes. Comment on attachment 63071 [details] Patch v2 Rejecting patch 63071 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20746 test cases. fast/history/gesture-before-onload.html -> failed Exiting early after 1 failures. 7973 tests run. 194.74s total testing time 7972 test cases (99%) succeeded 1 test case (<1%) had incorrect layout Full output: http://queues.webkit.org/results/3597608 Created attachment 63133 [details]
Patch
Updated patch fixes gesture-before-onload.html (to reset the back forward list before starting, since it otherwise only passed when run in isolation). (In reply to comment #23) > Updated patch fixes gesture-before-onload.html (to reset the back forward list before starting, since it otherwise only passed when run in isolation). sorry for not catching that during review. i always forget that! :-( Comment on attachment 63133 [details] Patch Clearing flags on attachment: 63133 Committed r64408: <http://trac.webkit.org/changeset/64408> All reviewed patches have been landed. Closing bug. Got rolled out because it was "causing failed tests on Chromium canaries due to wrong history item counts" (bug 43292). I'll have to try again with updated Chromium expectations. Tests that failed on the Chromium builder: http/tests/history/redirect-js-document-location-before-load.html = TEXT http/tests/history/redirect-js-location-assign-before-load.html = TEXT http/tests/history/redirect-js-location-before-load.html = TEXT http/tests/history/redirect-js-location-href-before-load.html = TEXT These are the tests that I updated the expectations of, since they now pass. The diff is: -PASS: History item count should be 1 and is. +FAIL: History item count should be 1 but instead is 2. (from http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&useWebKitCanary=true&tests=http%2Ftests%2Fhistory%2Fredirect-js-document-location-before-load.html) The strange part is that though history.length() reports 2, the actual dump of the back/forward list is correct (it only has one item), since I updated the expected output for that too. This may require a fix on the Chromium side, so it might be easiest to just update the Chromium test_expectations.txt to expected the text diff for now. *** Bug 17598 has been marked as a duplicate of this bug. *** > rdar://problem/5339292 as a bug that describes a cleanup of JS navigation logic > to be more like the redirection one. Is there anything else relevant in that bug? That bug is about further investigating ideas from bug 13400. Specifically, the ideas were: - always to add a history item unless the new load kicks off before the load event has fired; - or mimic our behavior for HTTP redirects, and add a new history item only if > 1 second has passed between loads. Created attachment 63283 [details]
Patch
New version of the patch should pass http/tests/history tests when run under Chromium's TestShell too. Short version: Check history.length instead of layoutTestController.webHistoryItemCount makes the same results be returned for both Chromium TestShell and DRT and it fixes previously-failing tests too. Long version: The difference (reported in comment 28) appeared to be that TestShell layoutTestController.webHistoryItemCount returns 1 more than the DRT version. The invocation in redirect-target.html actually says: layoutTestController.webHistoryItemCount + 1; // Add one to include the referring page, which loaded before we started recording history. I'm guessing this is because of the layoutTestController.keepWebHistory() call at the start of the tests. However, for TestShell, keepWebHistory is actually a no-op, since it always keeps history (from tools/test_shell/layout_test_controller.cc): void LayoutTestController::keepWebHistory(const CppArgumentList& args,CppVariant* result) { result->SetNull(); } Since we're not resetting history after the first page loads, it makes sense that Chromium gets one more item in the count. It looks like a bunch of these tests are already expected to fail for Chromium, based on chromium/test_expectations.txt // navigation controller tracks navigations as compared to how the Mac implementation does it. WONTFIX : http/tests/history/redirect-200-refresh-0-seconds.pl = FAIL WONTFIX : http/tests/history/redirect-js-location-replace-0-seconds.html = FAIL WONTFIX DEBUG : http/tests/history/redirect-js-location-replace-2-seconds.html = FAIL WONTFIX LINUX MAC RELEASE : http/tests/history/redirect-js-location-replace-2-seconds.html = FAIL WONTFIX SLOW WIN RELEASE : http/tests/history/redirect-js-location-replace-2-seconds.html = FAIL WONTFIX : http/tests/history/redirect-js-location-replace-before-load.html = FAIL WONTFIX : http/tests/history/redirect-meta-refresh-0-seconds.html = FAIL I could have added the 4 additional tests to that list too, but I was curious what gesture-before-onload.html (which I added) wasn't failing. It looks like the difference is that that calls layoutTestController.clearBackForwardList() and then queries history.length. That seems to be return the same result in both DRT and TestShell. I therefore modified all of the tests in http/tests/history to use that pattern too. The upshot is that the previously failing tests are now passing too (in addition to the ones that I modified in the initial version of this patch): Expected to fail, but passed: (5) http/tests/history/redirect-200-refresh-0-seconds.pl http/tests/history/redirect-js-location-replace-0-seconds.html http/tests/history/redirect-js-location-replace-2-seconds.html http/tests/history/redirect-js-location-replace-before-load.html http/tests/history/redirect-meta-refresh-0-seconds.html I'm going to hold off on modifying chromium/test_expectations.txt in this patch though, to make sure that the tests really do pass with all the builders. I also ended up modifying the redirect-30*.pl tests, they were setting the Location HTTP header but also including a <script> block with layoutTestController invocation, and that was leading to flakyness (it looked like we didn't always have time to execute the <script> tag before navigating away from the page). I modified them so that the <script> tag is now in a .html file, which then navigates to a Perl script that generates the 30x redirect to the redirect-target.html page, which seems to be work consistently. Comment on attachment 63283 [details] Patch This seems like a great change. > - bool lockBackForwardList = mustLockBackForwardList(m_frame) || (submission->state()->formSubmissionTrigger() == SubmittedByJavaScript && m_frame->tree()->parent()); > + bool lockBackForwardList = mustLockBackForwardList(m_frame, false) || (submission->state()->formSubmissionTrigger() == SubmittedByJavaScript && m_frame->tree()->parent()); I don't understand why form submission bypasses this. Do we have test cases that do form submission during the load event handling? Should those create back/forward list entries? If so, why? This "false" is completely mysterious, and shows why we normally use enum types for these sorts of arguments in WebKit. If we change this to be based on whether the user triggered the form submission, then we would no longer be passing a boolean constant in, and then the rationale for using an enum would go away. I also find the argument's name mysterious. The name is "must lock if during load", which seems too low level. I think we want a concept more like "directly triggered by user" or "triggered by script". A question the caller would know how to answer. Maybe we should just name it "wasUserGesture" to match the scheduleLocationChange function and reverse it's sense. The comment says "non-user navigation", so it seems you are thinking like I am, so lets have the argument name be consistent with this. I think we want to call this "back/forward list item" not "back forward list item" in the comment. Please consider an enum or a differently named boolean. Otherwise looks very good. (In reply to comment #33) > I don't understand why form submission bypasses this. Do we have test cases that do form submission during the load event handling? Should those create back/forward list entries? If so, why? FWIW, my original comments on bug 17598 were because I had a real-world page that did a form submission during loading, and I didn't want it treated as a separate history entry. (In reply to comment #33) > I don't understand why form submission bypasses this. Do we have test cases that do form submission during the load event handling? Should those create back/forward list entries? If so, why? Based on my testing in comment 11, it looked like all other browsers created a session history entry for form submissions on or before onload, so I was going for consistency with that. I don't mind making that consistent with the location change behavior, but based on http://trac.webkit.org/changeset/45606, http://trac.webkit.org/changeset/25410, https://webkit.org/b/32383 it looked this was a hairy area for web compat. (In reply to comment #34) > FWIW, my original comments on bug 17598 were because I had a real-world page that did a form submission during loading, and I didn't want it treated as a separate history entry. Peter, do you happen to remember the URL? (In reply to comment #35) > Based on my testing in comment 11, it looked like all other browsers created a session history entry for form submissions on or before onload, so I was going for consistency with that. Personally, I think other browsers are just wrong here. I'd rather do the sanest behavior and then tweak when we find it breaks something for something as esoteric as this. > (In reply to comment #34) > > FWIW, my original comments on bug 17598 were because I had a real-world page that did a form submission during loading, and I didn't want it treated as a separate history entry. > > Peter, do you happen to remember the URL? No. I think it was part of a Google intranet login system so I wouldn't be able to publicly link it anyway. Someone could probably make a testcase fairly easily, though. OK, I'll make form submits behave the same way as location changes (and add a test case). It looks like changing form submits during/before onload to not create a history entry breaks LayoutTests/http/tests/history/back-to-post.php. That was added to fix rdar://problem/6791439. Before I change it to make the form submit happen after onload, can any Apple engineer check that Radar bug and see if that goes against the sprit of the original fix? Created attachment 63400 [details]
Patch
New version of the patch now applies the same logic to form submits. It required some more test cases to be updated (e.g. another one of the http/tests/history tests now prints out PASS instead of FAIL) and ScheduledFormSubmission to invoke FrameLoader::clientRedirected too (which was a long-standing FIXME, so I felt reasonably confident doing that). It seems likely that this change does break the test case for Radar 6791439, but I’m sure it doesn’t have any effect on the bug. That bug was about going back to a page that was the result of a POST. So ideally we’d fix the test case to not depend on creating a history entry when doing a POST during an onload by adding the use of a timer as we did in other tests. (In reply to comment #41) > So ideally we’d fix the test case to not depend on creating a history entry when doing a POST during an onload by adding the use of a timer as we did in other tests. Great, that's what I ended up doing. Darin, would you have time to r+ this? Darin Fisher (the original reviewer, who gave me an r+ for the earlier version of the patch) is on vacation till next Wednesday. Comment on attachment 62906 [details] Patch v1 Cleared Darin Fisher's review+ from obsolete attachment 62906 [details] so that this bug does not appear in http://webkit.org/pending-commit. What's the status of this? I'm waiting anxiously for it to be reviewed and committed. (In reply to comment #44) > What's the status of this? I'm waiting anxiously for it to be reviewed and committed. Darin Fisher should be back tomorrow, so he should be able to review it then. Alternatively, let me know if you can think of another appropriate reviewer. Comment on attachment 63400 [details]
Patch
r=me
Created attachment 64375 [details]
Patch
New version of the patch that also updates /fast/dom/Geolocation/no-page-cache.html to do the navigation in the timeout (it was added a few hours ago http://trac.webkit.org/changeset/65325). Comment on attachment 63400 [details] Patch Rejecting patch 63400 from commit-queue. Unexpected failure when processing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 63400, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Last 500 characters of output: 3400&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=42861&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 63400 from bug 42861. NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Comment on attachment 64375 [details]
Patch
Spin again!
Comment on attachment 64375 [details] Patch Clearing flags on attachment: 64375 Committed r65340: <http://trac.webkit.org/changeset/65340> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/65340 might have broken GTK Linux 64-bit Debug (In reply to comment #53) > http://trac.webkit.org/changeset/65340 might have broken GTK Linux 64-bit Debug Filed bug 43998 about the GTK failure. *** Bug 14438 has been marked as a duplicate of this bug. *** *** Bug 22224 has been marked as a duplicate of this bug. *** |