Bug 48812 – FrameLoader::checkLoadCompleteForThisFrame uses wrong history item

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Charles Reis]

FrameLoader::checkLoadCompleteForThisFrame is called when a navigation is canceled, and it instructs the back/forward history to go to the current item from the history, rather than staying at the canceled navigation's history item.  The method is getting the current item using history()->currentItem(), and then assigning it using page->backForward()->setCurrentItem(item.get()).

However, there are cases when history()'s current item and backForward()'s current item are different.  For example, holding down the "back" keyboard shortcut in Chromium will start several navigations, some of which get canceled before committing.  (See http://code.google.com/p/chromium/issues/detail?id=58082 for details.)  In this case, backForward()->currentHistoryItem() is updated for every attempted navigation, but history()->currentItem() is not.  By trying to set the current item to history()->currentItem(), FrameLoader::checkLoadCompleteForThisFrame is causing us to jump to an outdated entry, rather than the one that's in progress.  This is leading to corruption of the back/forward history in Chromium.

Since the method is trying to update the back/forward history, it should be getting the current history item from there to make sure it matches the current state.

[Charles Reis]

Created attachment 72611 [details]
Patch

[Early Warning System Bot]

Attachment 72611 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4931005

[Charles Reis]

Created attachment 72616 [details]
Patch

[Alexey Proskuryakov]

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.

[Brady Eidson]

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...

[Brady Eidson]

(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)

[Charles Reis]

(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.

[Alexey Proskuryakov]

> 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.

[Charles Reis]

(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())?

[Charles Reis]

Created attachment 72886 [details]
Patch

[Charles Reis]

(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.

[Darin Adler]

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.

[Adam Barth]

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?

[Charles Reis]

Created attachment 73130 [details]
Patch

[Charles Reis]

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.)

[Mihai Parparita]

(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).

[Mihai Parparita]

(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.

[Charles Reis]

(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.

[Charles Reis]

Created attachment 73449 [details]
Patch

[Alexey Proskuryakov]

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.

[Charles Reis]

(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.

[Charles Reis]

Created attachment 73550 [details]
Patch

[Charles Reis]

Any thoughts on the latest patch?

[Darin Adler]

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.

[Charles Reis]

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.

[Charles Reis]

Created attachment 74136 [details]
Patch

[Darin Adler]

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.

[Charles Reis]

Created attachment 74142 [details]
Patch

[Charles Reis]

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

[Charles Reis]

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.

[Charles Reis]

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.

[Charles Reis]

Created attachment 74161 [details]
Patch

[Darin Adler]

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?

[Charles Reis]

(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.

[Charles Reis]

Created attachment 74305 [details]
Patch

[Adam Barth]

Comment on attachment 74305 [details]
Patch

I wanted to review this patch, but I didn't understand it in enough detail.  :(

[Darin Fisher (:fishd, Google)]

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.

[Charles Reis]

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.

[Charles Reis]

Created attachment 75044 [details]
Patch

[Charles Reis]

(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.

[Charles Reis]

(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.

[Nico Weber]

What's the status of this patch? Are you waiting for someone to set c+?

[Brady Eidson]

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.

[Charles Reis]

(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.

[Charles Reis]

Created attachment 79337 [details]
Patch

[Charles Reis]

(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.

[WebKit Commit Bot]

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

[Charles Reis]

Created attachment 79629 [details]
Patch

[Charles Reis]

Comment on attachment 79629 [details]
Patch

Here's an updated patch that changes the exported stopAllLoaders symbol in WebCore.exp.in.

[WebKit Commit Bot]

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

[Charles Reis]

Created attachment 79655 [details]
Patch

[Charles Reis]

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.)

[WebKit Commit Bot]

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

[Charles Reis]

Created attachment 79742 [details]
Patch

[Charles Reis]

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.

[WebKit Commit Bot]

Comment on attachment 79742 [details]
Patch

Clearing flags on attachment: 79742

Committed r76357: <http://trac.webkit.org/changeset/76357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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