Bug 42861 - Session history should skip over JS redirects
: Session history should skip over JS redirects
Status: RESOLVED FIXED
: WebKit
History
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://persistent.info/webkit/test-ca...
:
: 43292
:
  Show dependency treegraph
 
Reported: 2010-07-22 17:42 PST by
Modified: 2011-06-30 18:28 PST (History)


Attachments
Patch (3.81 KB, patch)
2010-07-26 18:07 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch v1 (31.14 KB, patch)
2010-07-28 18:08 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (31.15 KB, patch)
2010-07-30 10:09 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.16 KB, patch)
2010-07-30 18:15 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (58.68 KB, patch)
2010-08-02 17:54 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (80.67 KB, patch)
2010-08-03 18:29 PST, Mihai Parparita
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (81.59 KB, patch)
2010-08-13 14:33 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-22 17:42:42 PST
Chrome currently has some logic to not add session history entries for pages that generate "machine" redirects (e.g. if foo.com has a window.location.href = '/page.html' JS snippet onload on soon after) - see http://crrev.com/18373. This logic shouldn't live in Chrome code (in navigation_controller.cc, etc.), it should instead be promoted to WebKit code.

Bug that this would fix (reproduced as of WebKit r63854):
1. Visit http://www.apple.com/
2. Visit http://www.hp.com/
3. Wait for JS redirect to http://www.hp.com/#Product (should happen <1 second after hp.com loads)
4. Press back

Expected result:
1. End up at http://www.apple.com/

Actual result:
1. End up at a 404 page on hp.com

Reduced test case at:

http://persistent.info/webkit/test-cases/redirects/page1.html

After waiting for the two redirects (ending up on page 3), pressing back should take you to page 1, not page 2.

Proposed fix is similar to http://crrev.com/18373: Don't add a session history entry if the navigation happens without a user gesture (click or key press) within the first 5 seconds after the page loads.
------- Comment #1 From 2010-07-22 20:18:21 PST -------
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?
------- Comment #2 From 2010-07-23 11:59:25 PST -------
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)
------- Comment #3 From 2010-07-23 14:03:49 PST -------
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.
------- Comment #4 From 2010-07-24 17:24:46 PST -------
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.
------- Comment #5 From 2010-07-24 21:36:36 PST -------
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).
------- Comment #6 From 2010-07-24 22:39:44 PST -------
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.
------- Comment #7 From 2010-07-26 17:32:22 PST -------
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?
------- Comment #8 From 2010-07-26 18:07:33 PST -------
Created an attachment (id=62634) [details]
Patch
------- Comment #9 From 2010-07-26 18:17:04 PST -------
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.
------- Comment #10 From 2010-07-27 14:10:23 PST -------
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.
------- Comment #11 From 2010-07-27 14:29:17 PST -------
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).
------- Comment #12 From 2010-07-27 14:41:02 PST -------
(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.
------- Comment #13 From 2010-07-27 17:33:35 PST -------
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).
------- Comment #14 From 2010-07-28 08:32:31 PST -------
This sounds related to bugs 30224 (actually may be a duplicate) and 41670.
------- Comment #15 From 2010-07-28 10:50:23 PST -------
(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.
------- Comment #16 From 2010-07-28 18:08:41 PST -------
Created an attachment (id=62906) [details]
Patch v1
------- Comment #17 From 2010-07-28 18:11:53 PST -------
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 #18 From 2010-07-30 08:20:38 PST -------
(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

LayoutTests/fast/history/gesture-before-onload.html:21
 +      document.getElementById('manual-explanation').style.display = '';
nit: indentation

otherwise, r=me (nice work!)
------- Comment #19 From 2010-07-30 10:09:38 PST -------
Created an attachment (id=63071) [details]
Patch v2
------- Comment #20 From 2010-07-30 10:10:15 PST -------
(In reply to comment #18)
> (From update of attachment 62906 [details] [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 #21 From 2010-07-30 17:49:49 PST -------
(From update of attachment 63071 [details])
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
------- Comment #22 From 2010-07-30 18:15:08 PST -------
Created an attachment (id=63133) [details]
Patch
------- Comment #23 From 2010-07-30 18:16:02 PST -------
Updated patch fixes gesture-before-onload.html (to reset the back forward list before starting, since it otherwise only passed when run in isolation).
------- Comment #24 From 2010-07-30 20:09:02 PST -------
(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 #25 From 2010-07-30 22:39:36 PST -------
(From update of attachment 63133 [details])
Clearing flags on attachment: 63133

Committed r64408: <http://trac.webkit.org/changeset/64408>
------- Comment #26 From 2010-07-30 22:39:43 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #27 From 2010-07-31 08:41:33 PST -------
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.
------- Comment #28 From 2010-07-31 09:45:29 PST -------
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.
------- Comment #29 From 2010-07-31 15:08:17 PST -------
*** Bug 17598 has been marked as a duplicate of this bug. ***
------- Comment #30 From 2010-08-02 02:10:22 PST -------
> 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.
------- Comment #31 From 2010-08-02 17:54:47 PST -------
Created an attachment (id=63283) [details]
Patch
------- Comment #32 From 2010-08-02 17:55:58 PST -------
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 #33 From 2010-08-03 12:43:08 PST -------
(From update of attachment 63283 [details])
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.
------- Comment #34 From 2010-08-03 12:49:48 PST -------
(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.
------- Comment #35 From 2010-08-03 12:58:24 PST -------
(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?
------- Comment #36 From 2010-08-03 13:06:00 PST -------
(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.
------- Comment #37 From 2010-08-03 13:07:54 PST -------
OK, I'll make form submits behave the same way as location changes (and add a test case).
------- Comment #38 From 2010-08-03 16:01:15 PST -------
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?
------- Comment #39 From 2010-08-03 18:29:36 PST -------
Created an attachment (id=63400) [details]
Patch
------- Comment #40 From 2010-08-03 18:32:58 PST -------
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).
------- Comment #41 From 2010-08-03 23:09:49 PST -------
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.
------- Comment #42 From 2010-08-04 08:20:37 PST -------
(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 #43 From 2010-08-04 10:25:49 PST -------
(From update of attachment 62906 [details])
Cleared Darin Fisher's review+ from obsolete attachment 62906 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #44 From 2010-08-09 09:56:15 PST -------
What's the status of this?  I'm waiting anxiously for it to be reviewed and committed.
------- Comment #45 From 2010-08-12 22:47:36 PST -------
(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 #46 From 2010-08-13 12:58:24 PST -------
(From update of attachment 63400 [details])
r=me
------- Comment #47 From 2010-08-13 14:33:27 PST -------
Created an attachment (id=64375) [details]
Patch
------- Comment #48 From 2010-08-13 14:34:35 PST -------
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 #49 From 2010-08-13 14:52:26 PST -------
(From update of attachment 63400 [details])
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 #50 From 2010-08-13 16:09:41 PST -------
(From update of attachment 64375 [details])
Spin again!
------- Comment #51 From 2010-08-13 16:36:23 PST -------
(From update of attachment 64375 [details])
Clearing flags on attachment: 64375

Committed r65340: <http://trac.webkit.org/changeset/65340>
------- Comment #52 From 2010-08-13 16:36:32 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #53 From 2010-08-13 17:16:25 PST -------
http://trac.webkit.org/changeset/65340 might have broken GTK Linux 64-bit Debug
------- Comment #54 From 2010-08-13 17:48:53 PST -------
(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.
------- Comment #55 From 2011-04-06 13:43:12 PST -------
*** Bug 14438 has been marked as a duplicate of this bug. ***
------- Comment #56 From 2011-06-30 18:28:59 PST -------
*** Bug 22224 has been marked as a duplicate of this bug. ***