Summary: | FrameLoader::checkLoadCompleteForThisFrame uses wrong history item | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Charles Reis <creis> | ||||||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, beidson, brettw, commit-queue, eric, fishd, mihaip, thakis, webkit-ews, webkit.review.bot | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||
URL: | N/A | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Charles Reis
2010-11-01 17:56:27 PDT
Created attachment 72611 [details]
Patch
Attachment 72611 [details] did not build on qt: Build output: http://queues.webkit.org/results/4931005 Created attachment 72616 [details]
Patch
Comment on attachment 72616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72616&action=review r- for lack of a regression test. > ChangeLog:8 > + * ../../WebCore/loader/FrameLoader.cpp: "../../" is wrong. Please add an explanation of the fix to ChangeLog. This also seems a little weird as navigation in a subframe will be checking against the top-level back/forward item for the whole page... (In reply to comment #5) > This also seems a little weird as navigation in a subframe will be checking against the top-level back/forward item for the whole page... (I meant with regards to the patch) (In reply to comment #6) > (In reply to comment #5) > > This also seems a little weird as navigation in a subframe will be checking against the top-level back/forward item for the whole page... > > (I meant with regards to the patch) Subframe navigations won't hit this code, will they? "item" only gets assigned if the load is a back/forward and the frame is the mainFrame, based on the check on line 2352. Also, "item" is used to update the back/forward list on line 2383. It seems more likely to hit confusion about subframe vs mainframe if we assign something from history() to backForward() rather than from backForward() to backForward(). Although, looking again at line 2383, it looks like it has changed recently from goToItem to setCurrentItem, so it might be redundant to call setCurrentItem(currentItem()). I'll take another look at the buggy behavior to try to understand what should be happening. (In reply to comment #4) > (From update of attachment 72616 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72616&action=review > > r- for lack of a regression test. I'm in the process of writing a layout test, using layoutTestController.dumpBackForwardList() to check for the right state at the end. I haven't figured out how to get a layout test to hit this branch of FrameLoader, though. Is there a way to tell the browser to go back twice quickly without waiting for the first navigation to commit? Or to force the first back navigation to cancel? I'll keep looking, but any tips would be appreciated. Thanks, Charlie > > > ChangeLog:8 > > + * ../../WebCore/loader/FrameLoader.cpp: > > "../../" is wrong. Please add an explanation of the fix to ChangeLog. > Is there a way to tell the browser to go back twice quickly without waiting for the first navigation to
> commit? Or to force the first back navigation to cancel?
You may be able to do that in a separate window. LayoutTestController.setCanOpenWindows() enables window.open() in DumpRenderTree.
(In reply to comment #8) > > Is there a way to tell the browser to go back twice quickly without waiting for the first navigation to > > commit? Or to force the first back navigation to cancel? > > You may be able to do that in a separate window. LayoutTestController.setCanOpenWindows() enables window.open() in DumpRenderTree. Thanks. That's a good idea, but I can't seem to get it to cancel the first back navigation with the second one-- it always finishes the first navigation before attempting the second, even if there's a slow response from the server. For reference, I'm using the new window to tell the first window to go back twice, using some tricks with history.goBack() and setTimeout. In fact, I'm not even able to manually repro the bug in Chromium's test_shell (since it's not possible to hold down a "back" keyboard shortcut and clicking back repeatedly doesn't trigger it). It's definitely present in Chromium, though, since the navigation messages come in asynchronously from another process. As for the fix, it looks like we need to distinguish between two cases: If the navigation is just canceled using Stop or an equivalent, we're currently doing the right thing by going back to the last committed item. If the navigation is canceled because a new one is in progress, then we should not be going back to the last committed item. Ideally, we'd just leave the backForward() alone. (If that new navigation cancels, the other logic will kick in and send us back to the last committed item.) Perhaps a better fix would involve setting shouldReset to false if the backForward() current item is different than the URL being canceled (i.e., pdl->requestURL())? Created attachment 72886 [details]
Patch
(In reply to comment #10) > Created an attachment (id=72886) [details] > Patch I know this still needs a regression test, and I'm trying to get one working. I just wanted to get the updated patch up for feedback. Comment on attachment 72886 [details]
Patch
Patch looks OK; comparing URLs is not so great, and perhaps there is a better way to detect this case, but if not we can definitely live with this. Test is probably more important than the fix.
Comment on attachment 72886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72886&action=review > WebCore/loader/FrameLoader.cpp:2385 > + // Do not change the history if the backForward controller > + // is aleady loading a new URL. > + if (pdl->request().url() == page->backForward()->currentItem()->url()) { URL can't be the right thing to use here. What if all the pages in the back-forward list have the same URL? Wouldn't that case still have the corruption? Created attachment 73130 [details]
Patch
Comment on attachment 73130 [details] Patch (In reply to comment #13) > (From update of attachment 72886 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72886&action=review > > > WebCore/loader/FrameLoader.cpp:2385 > > + // Do not change the history if the backForward controller > > + // is aleady loading a new URL. > > + if (pdl->request().url() == page->backForward()->currentItem()->url()) { > > URL can't be the right thing to use here. What if all the pages in the back-forward list have the same URL? Wouldn't that case still have the corruption? Yeah, I'm still trying to figure this one out. Darin, do you have any ideas how to tell whether the DocumentLoader is loading a request that matches the backForward()'s HistoryItem or not? I'm having trouble finding another way to tell if we've gone on to a new back navigation, since the FrameLoader doesn't seem to have any other knowledge of it. Anyway, I've uploaded a patch with a layout test draft. It's not finished yet-- I still need to implement queueBackTwiceNavigation() in the various LayoutTestControllers, but it does let me successfully test for the bug. (It tells the browser to go back and then back again before the first one commits. JavaScript code doesn't seem able to do this.) (In reply to comment #15) > Anyway, I've uploaded a patch with a layout test draft. It's not finished yet-- I still need to implement queueBackTwiceNavigation() in the various LayoutTestControllers, but it does let me successfully test for the bug. (It tells the browser to go back and then back again before the first one commits. JavaScript code doesn't seem able to do this.) You should be able to do with without any LayoutTestController changes if you have a frame that uses a PHP script with a sleep() statement before any bytes are returned (I needed something like that for http://trac.webkit.org/changeset/71368, the inner-most frame is not supposed to go into the committed state before being navigated away from). (In reply to comment #15) > Yeah, I'm still trying to figure this one out. Darin, do you have any ideas how to tell whether the DocumentLoader is loading a request that matches the backForward()'s HistoryItem or not? Can you use item sequence numbers for this? See http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/WebCore/loader/HistoryController.cpp&q=itemsequencenumber&exact_package=chromium&l=560. (In reply to comment #16) > You should be able to do with without any LayoutTestController changes if you have a frame that uses a PHP script with a sleep() statement before any bytes are returned (I needed something like that for http://trac.webkit.org/changeset/71368, the inner-most frame is not supposed to go into the committed state before being navigated away from). Thanks for pointing this out, Mihai! I was able to get a test working without any new APIs using this technique. It's worth noting that it also helped me uncover another case (going back twice to the same history item, rather than to different history items), which affects what we need to do to fix it. (In reply to comment #17) > Can you use item sequence numbers for this? See http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/WebCore/loader/HistoryController.cpp&q=itemsequencenumber&exact_package=chromium&l=560. Yeah, I thought this was the right approach, although there's no way to get the itemSequenceNumber from the DocumentLoader or its ResourceRequest. As the second part of the test showed, though, going back twice to the same history item will result in the same itemSequenceNumber. That will make FrameLoader think we've hit Stop instead of Back, so it will still corrupt the backForward history. A better fix will let us uniquely match the ResourceRequest to the HistoryItem. I'm uploading a draft of one way to do that, adding a unique ID to ResourceRequest and assigning it to the HistoryItem while the request is in progress. I'm open to suggestions on how to improve it, though. Created attachment 73449 [details]
Patch
Comment on attachment 73449 [details] Patch A general note: any design that involves both HistoryItem and ResourceRequest seems suspicious. These are pretty distant layers to talk to each other. Requests already have identifiers, see e.g. FrameLoaderClient::assignIdentifierToInitialRequest(). Adding another "ID" would be terribly confusing. And of course m_id won't persist a Mac Objective-C API delegate call. See <http://trac.webkit.org/browser/trunk/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm#L387> - the request is re-created from scratch after a client has a chance to modify it. (In reply to comment #20) > (From update of attachment 73449 [details]) > A general note: any design that involves both HistoryItem and ResourceRequest seems suspicious. These are pretty distant layers to talk to each other. That's fair. Do you have a suggestion for how to tell whether the canceled request in pdl (which is a DocumentLoader) corresponds to page->backForward()->currentItem() (which is a HistoryItem)? Without that, we'll continue to corrupt HistoryItems if a new navigation has already begun. Or conversely, how to distinguish if the request was canceled due to a new navigation or not? Other options that I see: we could associate either the DocumentLoader or its NavigationAction with the HistoryItem. It might reasonable to add a HistoryItem field to NavigationAction, so that we can tell which HistoryItem triggered it. That'd fix this bug without touching ResourceRequest. > > Requests already have identifiers, see e.g. FrameLoaderClient::assignIdentifierToInitialRequest(). Adding another "ID" would be terribly confusing. Unfortunately, that identifier doesn't help-- there's no way to query which identifier is assigned to a given request, so we can't use it to compare requests. Additionally, the assignIdentifierToInitialRequest call currently does nothing in either Chrome (RenderView) or test_shell (WebViewDelegate). > > And of course m_id won't persist a Mac Objective-C API delegate call. See <http://trac.webkit.org/browser/trunk/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm#L387> - the request is re-created from scratch after a client has a chance to modify it. Glad you pointed that out. The NavigationAction approach should avoid that, by not depending on the ResourceRequest at all. Created attachment 73550 [details]
Patch
Any thoughts on the latest patch? Comment on attachment 73550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73550&action=review This change looks OK. I think we might want to refine the coding of it a bit. > WebCore/loader/FrameLoader.cpp:2385 > + if (!pdl->triggeringAction().isEmpty() > + && pdl->triggeringAction().historyItem() == page->backForward()->currentItem()) { Why do we need the isEmpty check? Presumably if the action is empty then the history item will be 0. Can currentItem be 0? I’d prefer to not have the isEmpty function. > WebCore/loader/FrameLoader.cpp:3207 > + action = NavigationAction(itemURL, loadType, false, 0, item); With all the overloads for creating a NavigationAction, for some reason we have to specify both false and 0 here. Maybe we should do different overloads instead? > WebCore/loader/FrameLoader.cpp:3235 > + action = NavigationAction(itemOriginalURL, loadType, false, 0, item); Same thing happens again here. I think we should consider whether we can do a different overload. > WebCore/loader/NavigationAction.h:48 > NavigationAction(); > NavigationAction(const KURL&, NavigationType); > + NavigationAction(const KURL&, NavigationType, PassRefPtr<HistoryItem>); > NavigationAction(const KURL&, FrameLoadType, bool isFormSubmission); > NavigationAction(const KURL&, NavigationType, PassRefPtr<Event>); > NavigationAction(const KURL&, FrameLoadType, bool isFormSubmission, PassRefPtr<Event>); > + NavigationAction(const KURL&, FrameLoadType, bool isFormSubmission, PassRefPtr<Event>, PassRefPtr<HistoryItem>); These are sorted in a strange order that makes it hard to read them. Also, I think we can use default argument values to cut down a bit on the overloading. Both PassRefPtr<Event> and PassRefPtr<HistoryItem> can have a default value of 0. Comment on attachment 73550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73550&action=review >> WebCore/loader/FrameLoader.cpp:2385 >> + && pdl->triggeringAction().historyItem() == page->backForward()->currentItem()) { > > Why do we need the isEmpty check? Presumably if the action is empty then the history item will be 0. Can currentItem be 0? > > I’d prefer to not have the isEmpty function. Sure, done. >> WebCore/loader/NavigationAction.h:48 >> + NavigationAction(const KURL&, FrameLoadType, bool isFormSubmission, PassRefPtr<Event>, PassRefPtr<HistoryItem>); > > These are sorted in a strange order that makes it hard to read them. Also, I think we can use default argument values to cut down a bit on the overloading. Both PassRefPtr<Event> and PassRefPtr<HistoryItem> can have a default value of 0. Good idea. I've simplified and reordered it a bit, avoiding the need to pass false or 0 at the call sites. Created attachment 74136 [details]
Patch
Comment on attachment 74136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74136&action=review > WebCore/loader/NavigationAction.h:45 > + NavigationAction(const KURL&, FrameLoadType, PassRefPtr<HistoryItem>); Could have an = 0 on this line. Created attachment 74142 [details]
Patch
Comment on attachment 74136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74136&action=review >> WebCore/loader/NavigationAction.h:45 >> + NavigationAction(const KURL&, FrameLoadType, PassRefPtr<HistoryItem>); > > Could have an = 0 on this line. Fixed. Thanks, Charlie Comment on attachment 74142 [details]
Patch
Putting this on hold for the moment-- I just noticed a layout test failing:
fast/dom/navigation-type-back-forward.html
I'll investigate.
Comment on attachment 74142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74142&action=review > WebCore/loader/FrameLoader.cpp:3234 > + action = NavigationAction(itemOriginalURL, loadType, item); This line caused the test failure. Interestingly, the compiler was treating item as the boolean parameter to the NavigationAction(KURL, FrameLoadType, bool, PassRefPtr<Event>) constructor, rather than using NavigationAction(KURL, FrameLoadType, PassRefPtr<HistoryItem>). Guess it thinks HistoryItem* is a closer match to bool than PassRefPtr<HistoryItem>. If I'm more explicit at the call site by passing in a RefPtr<HistoryItem>, the compiler complains that PassRefPtr<HistoryItem> and PassRefPtr<Event> are ambiguous (at least up on line 3209). It works if we switch to using HistoryItem* in NavigationAction, but I'm open to better suggestions. For example, we could make the PassRefPtr<Event> argument non-optional and pass in 0 at all the call sites, to avoid having item treated as a boolean in this case. Or we could reorder the arguments. Created attachment 74161 [details]
Patch
Comment on attachment 74161 [details]
Patch
Why is it safe to put a HistoryItem* in this object without using RefPtr? Can’t the history item be destroyed while the NavigationAction object is still around?
(In reply to comment #33) > (From update of attachment 74161 [details]) > Why is it safe to put a HistoryItem* in this object without using RefPtr? Can’t the history item be destroyed while the NavigationAction object is still around? I don't think it'll get destroyed before we access it in checkLoadCompleteForThisFrame, but it's probably not good to keep it that way. I'll upload a new patch reordering the arguments to eliminate the ambiguity. The last constructor will now be: NavigationAction(KURL, bool, FrameLoadType, PassRefPtr<Event> = 0) I can switch it to a non-optional Event argument and passing 0 at the call sites if you'd rather have that than reordered arguments. Created attachment 74305 [details]
Patch
Comment on attachment 74305 [details]
Patch
I wanted to review this patch, but I didn't understand it in enough detail. :(
Comment on attachment 74305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74305&action=review > WebCore/loader/FrameLoader.cpp:2380 > + if (pdl->triggeringAction().historyItem() == page->backForward()->currentItem()) { Can we make use HistoryController::currentItem() or perhaps HistoryController::provisionalItem()? It seems that in the back/forward navigation case, HistoryController::goToItem sets HistoryController::m_provisionalItem, and then when we commit the load, that gets promoted to HistoryController::m_currentItem. If another load comes in after that, then the BackForwardList's currentItem would change to correspond to the new provisional HistoryItem. I'm also a bit confused as to why this code only assigns |item| when the current frame is the main frame. This makes me wonder if calling BackForwardList::setCurrentItem here is really the right thing to do. If a navigation only occurs in a subframe, we'd still want to potentially update the BackForwardList. Comment on attachment 74305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74305&action=review >> WebCore/loader/FrameLoader.cpp:2380 >> + if (pdl->triggeringAction().historyItem() == page->backForward()->currentItem()) { > > Can we make use HistoryController::currentItem() or perhaps HistoryController::provisionalItem()? > It seems that in the back/forward navigation case, HistoryController::goToItem sets > HistoryController::m_provisionalItem, and then when we commit the load, that gets promoted to > HistoryController::m_currentItem. If another load comes in after that, then the BackForwardList's > currentItem would change to correspond to the new provisional HistoryItem. > > I'm also a bit confused as to why this code only assigns |item| when the current frame is the > main frame. This makes me wonder if calling BackForwardList::setCurrentItem here is really > the right thing to do. If a navigation only occurs in a subframe, we'd still want to > potentially update the BackForwardList. Yes, I think history()->provisionalItem() should be the same as backForward()->currentItem() here, and it's a bit clearer to understand that way. I've updated the comment as well to make this easier to follow. I'll upload that once the tests pass. I'm less sure about the frame issue. I'll take a look while the tests are running, but we may want to tackle that after this fix goes in. Created attachment 75044 [details]
Patch
(In reply to comment #38) > Yes, I think history()->provisionalItem() should be the same as backForward()->currentItem() here, and it's a bit clearer to understand that way. I've updated the comment as well to make this easier to follow. I'll upload that once the tests pass. The tests look good with provisionalItem(). I think it makes sense to go ahead with this part, since it resolves the crash in Chrome. > I'm less sure about the frame issue. I'll take a look while the tests are running, but we may want to tackle that after this fix goes in. There does appear to be an issue with frame navigations, where it can get confused if you cancel a slow loading frame. I'll file a bug for that. (In reply to comment #40) > There does appear to be an issue with frame navigations, where it can get confused if you cancel a slow loading frame. I'll file a bug for that. I've filed issue 50254 for the frame navigations problem, with an attached layout test. I haven't figured out what the fix for that one is yet. What's the status of this patch? Are you waiting for someone to set c+? Comment on attachment 75044 [details]
Patch
Sorry this patch has been sitting here with an r+ for awhile and that I'm just doing this, but reading my bugzilla CC email I noticed something I really dislike here.
I understand the motivation for reordering the arguments was the "null vs bool" ambiguity. A much better way to fix that besides reordering the arguments would be to take this opportunity to replace the bool with an enum.
Since it's been so long I'm going to change to r- in hopes this can be done before landing.
(In reply to comment #43) > (From update of attachment 75044 [details]) > Sorry this patch has been sitting here with an r+ for awhile and that I'm just doing this, but reading my bugzilla CC email I noticed something I really dislike here. > > I understand the motivation for reordering the arguments was the "null vs bool" ambiguity. A much better way to fix that besides reordering the arguments would be to take this opportunity to replace the bool with an enum. > > Since it's been so long I'm going to change to r- in hopes this can be done before landing. Ok, I'll take a look. I also tried to bring the patch up to date and discovered there's another problem. Some clients (e.g., DumpRenderTree but not test_shell or chromium) will re-use the same HistoryItem for repeated back navigations. That means if you click back twice, we can't distinguish between the two navigations and thus interpret it as a stop. Drat. Now that the fix for 50254 has landed and ensured we have provisional items in the history for all frames, I'd like to approach this another way. Rather than trying to determine whether a new navigation has begun and skipping the state reset, we should only do the state reset if we know that no navigation is in progress. We could detect that if history()->provisionalItem() was set to 0 after a stop. I'll see if I can get that approach to work. Created attachment 79337 [details]
Patch
(In reply to comment #44) > Now that the fix for 50254 has landed and ensured we have provisional items in the history for all frames, I'd like to approach this another way. Rather than trying to determine whether a new navigation has begun and skipping the state reset, we should only do the state reset if we know that no navigation is in progress. We could detect that if history()->provisionalItem() was set to 0 after a stop. > > I'll see if I can get that approach to work. I've just uploaded a new patch that clears the history's provisional item if a navigation is canceled, which is a nice invariant to have in itself. In this bug, it lets us avoid the state reset if a new navigation is in progress. I've slightly updated forward-and-cancel.html so that it acts as a regression test for the state reset logic (ensuring it happens if you click stop), and I'm still using back-twice-without-cancel.html to test that the state reset logic doesn't happen if you've started a new navigation. Comment on attachment 79337 [details] Patch Rejecting attachment 79337 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'bu..." exit_code: 2 Last 500 characters of output: d symbols: "__ZN7WebCore11FrameLoader14stopAllLoadersENS_14DatabasePolicyE", referenced from: -exported_symbol[s_list] command line option (maybe you meant: __ZN7WebCore11FrameLoader14stopAllLoadersENS_14DatabasePolicyENS_26ClearProvisionalItemPolicyE) ld: symbol(s) not found collect2: ld returned 1 exit status ** BUILD FAILED ** The following build commands failed: WebCore: Ld /Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal x86_64 (1 failure) Full output: http://queues.webkit.org/results/7540237 Created attachment 79629 [details]
Patch
Comment on attachment 79629 [details]
Patch
Here's an updated patch that changes the exported stopAllLoaders symbol in WebCore.exp.in.
Comment on attachment 79629 [details] Patch Rejecting attachment 79629 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'bu..." exit_code: 2 Last 500 characters of output: tp/tests/media ....... http/tests/messaging .. http/tests/mime ........ http/tests/misc ....................................................................................................... http/tests/multipart ..... http/tests/navigation ....... http/tests/navigation/back-twice-without-commit.html -> failed Exiting early after 1 failures. 21680 tests run. 457.09s total testing time 21679 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 13 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7627219 Created attachment 79655 [details]
Patch
Comment on attachment 79655 [details]
Patch
New patch that uses layoutTestController.clearBackForwardList() in both tests. (Mihai found that Chromium resets the navigation controller between tests without needing this, and suggests that we update the other ports to act this way as well.)
Comment on attachment 79655 [details] Patch Rejecting attachment 79655 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'bu..." exit_code: 2 Last 500 characters of output: ata ..... http/tests/media ....... http/tests/messaging .. http/tests/mime ........ http/tests/misc ....................................................................................................... http/tests/multipart ..... http/tests/navigation ....... http/tests/navigation/back-twice-without-commit.html -> crashed Exiting early after 1 failures. 21681 tests run. 738.20s total testing time 21680 test cases (99%) succeeded 1 test case (<1%) crashed 12 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7524224 Created attachment 79742 [details]
Patch
Comment on attachment 79742 [details]
Patch
The test in the previous patch crashed on commit and not in Chromium WebKit because it called queueBackNavigation with too large a value (which behaves fine in Chromium WebKit). I've updated back-twice-without-commit.html and tested it by hand on non-Chromium WebKit build on a Mac. This should hopefully do the trick.
Comment on attachment 79742 [details] Patch Clearing flags on attachment: 79742 Committed r76357: <http://trac.webkit.org/changeset/76357> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/76357 might have broken Qt Linux Release The following tests are not passing: http/tests/navigation/forward-and-cancel.html |