NavigationController might not be able to track navigation correctly in a special case |
|||||||
Issue descriptionIn the middle of following codereview, I noticed unexpected behaviors. https://codereview.chromium.org/2174293002/ Here is an example log on performing reload at twitter.com. Four items are pending_entry's unique ID, DidCommitProvisionalLoad_Params.nav_entry_id, navigation type, and DidCommitProvisionalLoad_Params.gesture. 3, 0, [EXISTING_PAGE], [GestureUnknown] 5, 3, [EXISTING_PAGE], [GestureAuto ] *, 0, [AUTO_SUBFRAME], [GestureUnknown] *, 0, [AUTO_SUBFRAME], [GestureUnknown] *, 0, [AUTO_SUBFRAME], [GestureUnknown] *, 0, [AUTO_SUBFRAME], [GestureUnknown] *, 0, [AUTO_SUBFRAME], [GestureUnknown] *, 0, [EXISTING_PAGE], [GestureUnknown] Usually, we see the first NavigationControllerImpl::RenderDidNavigate() call with the same ID of nav_entry_id with pending_entry's unique ID. But in this case, we see unexpected renderer initiated navigation before the first navigation's completion. I may misunderstand something, but let me file this bug to track my investigation.
,
Sep 14 2016
Note: when DevTools is open, nav_entry_id fir the first callback is always 0. This might be another problem, but I do not investigate this another case at this point.
,
Sep 14 2016
DevTools Network tab is refreshed twice on reloading twitter.com. But this may not because of client redirect, but unload event. The first Network log: - twitter.com (triggered by reload) - jot (triggered inside unload event handler to do 'POST') The second Network log: - twitter.com (many resource requests follow) I'm not sure I understand DevTools behavior correctly, but the log refresh might happen to remove 'jot' entry that is triggered by previous document, and the second request for the twitter.com is the same one with one in the first log.
,
Sep 14 2016
Inside Document::dispatchBeforeUnloadEvent() for the page that is going to be reloaded, DidCommitProvisionalLoad() is called before DidStartProvisionalLoad() for the reload happens.
,
Sep 15 2016
,
Sep 15 2016
replaceState is called on beforeload event handler, and that triggers another same page navigation in the dispatchBeforeUnloadEvent().
,
Sep 15 2016
OK, now I have a minimized test that can repro the case.
<html><head><script>
window.addEventListener("beforeunload", function (event) {
history.replaceState({}, "title", location.href);
});
</script></head></html>
,
Sep 15 2016
Note: When I run the sample code at #8 with a different URL as the third argument of replaceState, Firefox navigates to the specified URL on reload. That looks a Firefox's bug. Chrome just sets the URL in the omnibox, but reload takes the right original URL. This sounds working correctly as the spec expects. Anyway, I'm failing to track UI-initiated reloads at NavigationController. Here is what happens at twitter.com. 1. browser initiates a reload navigation A. 2. on beforeunload event handler, JavaScript calls replaceState(). This should not trigger an actual navigation, but just set an URL to the current history state. Since this is a renderer-initiated navigation-like event, only DidCommitProvisionalLoad() is called. 3. Navigation A is started, and DidStartProvisionalLoad() is called. 4. Navigation A is committed, and DidCommitProvisionalLoad() is called. If we do not have 2, we can associate 1, 3, and 4 as a one for the first reload. But with the step 2, we dispose pending_entry at the step 2, and can not associate 1 and 3. From viewpoint of the spec; https://html.spec.whatwg.org/multipage/browsers.html#dom-history-replacestate We can simply ignore replaceState() as the algorithm step 3 allows us as optional. > Optionally, abort these steps. (For example, the user agent might disallow calls to these methods 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.)
,
Sep 19 2016
Thanks for tracking this down to replaceState during beforeunload. I don't think ignoring replaceState is going to be the right solution. That's likely going to break whatever Twitter is trying to accomplish with it. I wonder if we want to leave the pending entry in place in this case? We'll need to be very careful about that to avoid leaving a pending entry in place when we shouldn't, but maybe there's some way to discover that the intended navigation is still going to happen in this case. The code is in RendererDidNavigateToExistingPage: // The entry we found in the list might be pending if the user hit // back/forward/reload. This load should commit it (since it's already in the // list, we can just discard the pending pointer). We should also discard the // pending entry if it corresponds to a different navigation, since that one // is now likely canceled. If it is not canceled, we will treat it as a new // navigation when it arrives, which is also ok. // // Note that we need to use the "internal" version since we don't want to // actually change any other state, just kill the pointer. DiscardNonCommittedEntriesInternal(); I'm mainly worried about the bad implications of leaving around a pending entry in other cases, though. How important is it to fix this case for the reload tracking?
,
Sep 20 2016
I'm extremely nervous to keep the pending entry, and somehow track that A is still ongoing. Hopefully the changes to match the entry up with the nav_handle id prevent spoofing attacks here though...
,
Sep 20 2016
Agreed that URL spoofs are the risk if we get it wrong and leave a pending entry around in any cases that the navigation isn't going to commit anymore. Nasko and I were chatting about this a bit, and we think it *may* be safe in the following case: - Pending NavEntry is browser-initiated. - Commit is same-page. - Commit has no user gesture. - Commit is renderer-initiated. If all of these are true, the existing browser-initiated navigation should not be canceled by the commit, and we can leave the pending entry around for it. The page wouldn't be able to do any more spoofing than it could without the replaceState (just by altering its appearance). If we go this route, we should have a browser test that confirms that neither same-process nor cross-process navigations get canceled by such a renderer-initiated, same-page commit, and that we clear the pending NavEntry in other cases. I'm somewhat inclined to go down this path (despite the risks), because there are other features that try to store things on the pending entry and risk having them clobbered by sites like Twitter that extensively use pushState/replaceState. toyoshim@: Would this help with your case? Do you want to try writing a CL for it?
,
Nov 22 2016
Seems like we understand root cause and a possible solution (thanks creis). I'll go ahead and mark this as available for now. toyoshim feel free to pick this up if you'd like.
,
Nov 22 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 9 2018
I think we still want this fixed in the long run, so removing the Hotlist-Recharge-Cold label.
,
Apr 13 2018
Loading rotation triager here: toyoshim@ would you make the call? |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by toyoshim@chromium.org
, Sep 14 2016