beforeunload event incorrectly fired when only hash part of url is changed
Reported by
factormy...@gmail.com,
Jun 26 2017
|
|||||||||||||||||||||||
Issue description
Chrome Version : 60.0.3112.40 (Official Build) beta (64-bit)
OS Version: Windows 10 v1703
URLs (if applicable) : see repro steps below
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
Firefox 54: OK
IE 11: OK
Edge 15.15063: OK
What steps will reproduce the problem?
1. Set up an event handler for "beforeunload"
eg, window.addEventListener("beforeunload", function(e) { console.error(e); });
2. Manually change the hash portion of the url in the browser & press enter
eg, append "#test" to the page url
3. Observe in the console that the "beforeunload' event was fired
What is the expected result?
The "beforeunload" event should NOT fire when ONLY the hash portion of the url is changed.
What happens instead of that?
The "beforeunload" event is fired unexpectedly.
Please provide any additional information below. Attach a screenshot if
possible.
Basically, it seems like spurious beforeunload events are now being fired incorrectly.
Note that it works as expected when running `window.location.hash = "#whatever"`, or clicking an a-tag with a hash url. There is some variance between that and manually changing the hash in the url, apparently.
Also, the hashchange event WILL be fired AFTER beforeunload in this case, which seems very clearly to demonstrate that something is wrong.
This is harmful for hash-routed SPAs that do (conditional) teardown in beforeunload, since now hash changes are unloading the app.
,
Jun 27 2017
This should be enough to reproduce:
<!DOCTYPE html>
<html>
<head>
<script type="text/javascript">
window.addEventListener("beforeunload", function(e) {
console.error(e);
});
window.addEventListener("hashchange", function(e) {
console.warn(e)
});
</script>
</head>
</html>
,
Jun 27 2017
Thank you for providing more feedback. Adding requester "kavvaru@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 29 2017
,
Jun 29 2017
Unable to reproduce the issue on windows 7,Ubuntu 14.04 using chrome version 60.0.3112.50 with the below steps 1.Opened the Html file with code provided. 2.Not observed any error or warning in console Please find the attached screen shot and confirm if anything missed here. Request you once try the issue on latest beta 60.0.3112.50 and update the thread if the issue still persists. Thanks,
,
Jun 30 2017
Please refer to the reproduction steps in the OP for how to replicate the bug
,
Jun 30 2017
Thank you for providing more feedback. Adding requester "kavvaru@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 3 2017
Unable to reproduce the issue on Win-10 using chrome beta version #60.0.3112.50 and latest canary #61.0.3147.0. Attached a screen cast for reference. Following are the steps followed to reproduce the issue. ------------ 1. Opened the html file with code provided in comment #2. 2. Manually changed the hash portion of the url in the browser i.e appended "#test" to the page url & pressed enter. 3. Observed in the console that the "HashChangeEvent" event was fired instead of "beforeunload' event as expected. factormystic@ - Could you please verify the screen cast and please let us know if anything missed from our side and also please check the issue on latest canary #61.0.3147.0 by creating a new profile without any apps and extensions and please let us know if the issue still persist or not. Thanks...!!
,
Jul 3 2017
Thank you for providing more feedback. Adding requester "krajshree@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 3 2017
I have also confirmed that it still reproduces in 60.0.3112.50
,
Jul 3 2017
Hmm, it's very inconsistent here. 99% of tries I can't reproduce it. I've tried finding the exact conditions but failed at both guesses (the two deleted comments above).
,
Jul 4 2017
,
Jul 4 2017
Unable to reproduce the issue on Win-7 and Win-10 using chrome reported version #60.0.3112.40 and chrome version #60.0.3112.50. Followed all the steps as mentioned in comment #8 and as per the .gif in comment #9. This time in an incognito window, but did not succeed in reproducing the issue. Could anyone from blink team please have a look into the issue. Thanks...!!
,
Jul 4 2017
,
Jul 6 2017
Hmm, we really would like a reliable repro step for us to be helpful. Changing the components to navigation as it smells like navigation issue.
,
Jul 11 2017
factormystic@ - Could you please respond as per comment #18. Thanks...!!
,
Jul 11 2017
I'm not sure what other diagnostics I can provide here. I have already provided a repro program and screen capture.
,
Jul 11 2017
Thank you for providing more feedback. Adding requester "krajshree@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 26 2017
Seeing this on 60.0.3112.78 now as well. Same repro as in the gif.
,
Aug 9 2017
@factormystic -- Tested on latest Chrome stable #60.0.3112.90 and Canary #62.0.3179.0 and unable to reproduce the issue. Could you please try updating your Chrome to latest stable version #60.0.3112.90 and see if you can still notice the issue. Let us know if the issue still persists. Thanks in advance.
,
Sep 19 2017
This is reliably reproducable in Chrome stable 61.0.3163.91 and Chrome Canary 63.0.3218.0.
,
Sep 19 2017
This is also reproducable with bookmarks containing hashes in the URLs using the test page in Comment 2: https://bugs.chromium.org/p/chromium/issues/detail?id=736785#c2
,
Sep 19 2017
I found a way to reproduce this reliably. Enable browser side navigation via chrome://flags/#browser-side-navigation or a command line switch --enable-browser-side-navigation A simplified test.html is attached: 1. open test.html 2. edit the URL in the addressbar: add #a and press Enter Expected: "hashchange fired" displayed in the page Observed: "FAIL: beforeunload fired", "hashchange fired" displayed in the page Browser side navigation aka PlzNavigate was disabled by default until recently. Now it's randomly enabled via field trials experiment. The OP must have enabled it manually or for some reason was always chosen by the field trial. The bug is present since PlzNavigate inception, apparently, somewhere around M40.
,
Sep 19 2017
,
Sep 19 2017
I can confirm that this is a PlzNavigate-specific issue - an ad-hoc browser test below fails at ToT, but passes if --disable-browser-side-navigation flag is passed.
IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
HashNavigationVsBeforeUnloadEvent) {
GURL main_url(embedded_test_server()->GetURL("/title1.html"));
GURL hash_url(embedded_test_server()->GetURL("/title1.html#hash"));
EXPECT_TRUE(NavigateToURL(shell(), main_url));
EXPECT_TRUE(ExecuteScript(
shell(),
R"( window.addEventListener("beforeunload", function(e) {
domAutomationController.send("beforeunload");
});
window.addEventListener("unload", function(e) {
domAutomationController.send("unload");
});
)"));
DOMMessageQueue message_queue;
std::vector<std::string> messages;
std::string message;
EXPECT_TRUE(NavigateToURL(shell(), hash_url));
while (message_queue.PopMessage(&message))
messages.push_back(message);
// Verify that none of "beforeunload", "unload" events fired.
EXPECT_THAT(messages, testing::IsEmpty());
}
FWIW, I see that with PlzNavigate beforeunload is dispatched by the browser from the following callstack:
#1 0x7f50615be453 content::RenderFrameHostImpl::DispatchBeforeUnload()
#2 0x7f50615a3862 content::NavigatorImpl::RequestNavigation()
#3 0x7f50615a2d2c content::NavigatorImpl::NavigateToEntry()
#4 0x7f50615a3a85 content::NavigatorImpl::NavigateToPendingEntry()
#5 0x7f5061590907 content::NavigationControllerImpl::NavigateToPendingEntryInternal()
#6 0x7f506158b014 content::NavigationControllerImpl::NavigateToPendingEntry()
#7 0x7f506158b427 content::NavigationControllerImpl::LoadEntry()
#8 0x7f506158c70b content::NavigationControllerImpl::LoadURLWithParams()
#9 0x000000b682b5 content::Shell::LoadURL()
#10 0x000000b15822 content::NavigateToURL()
I think the issue is with the way that NavigatorImpl::RequestNavigation calculates |should_dispatch_beforeunload| - it takes into account HISTORY_SAME_DOCUMENT but doesn't take into account SAME_DOCUMENT. FWIW, the issue seems to go away after the following changes:
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index b4e1ca4bdad8..ea6e799af30e 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -1138,9 +1138,6 @@ void NavigatorImpl::RequestNavigation(
// We don't want to dispatch a beforeunload handler if
// is_history_navigation_in_new_child is true. This indicates a newly created
// child frame which does not have a beforunload handler.
- bool should_dispatch_beforeunload =
- !is_same_document_history_load && !is_history_navigation_in_new_child &&
- frame_tree_node->current_frame_host()->ShouldDispatchBeforeUnload();
FrameMsg_Navigate_Type::Value navigation_type = GetNavigationType(
frame_tree_node->current_url(), // old_url
dest_url, // new_url
@@ -1148,6 +1145,10 @@ void NavigatorImpl::RequestNavigation(
entry, // entry
frame_entry, // frame_entry
is_same_document_history_load); // is_same_document_history_load
+ bool is_same_document = FrameMsg_Navigate_Type::IsSameDocument(navigation_type);
+ bool should_dispatch_beforeunload =
+ !is_same_document && !is_history_navigation_in_new_child &&
+ frame_tree_node->current_frame_host()->ShouldDispatchBeforeUnload();
std::unique_ptr<NavigationRequest> scoped_request =
NavigationRequest::CreateBrowserInitiated(
frame_tree_node, dest_url, dest_referrer, frame_entry, entry,
,
Sep 19 2017
WIP CL @ https://chromium-review.googlesource.com/c/chromium/src/+/673065
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d92ba9dc7a7a271c450c2bb48d44e9ff838c0422 commit d92ba9dc7a7a271c450c2bb48d44e9ff838c0422 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Sep 20 16:32:29 2017 Avoid firing beforeunload event for same document navigations. Bug: 736785 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I0af5431812de23e692b16fe23d677a71806fe26c Reviewed-on: https://chromium-review.googlesource.com/673065 Reviewed-by: Camille Lamy <clamy@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#503175} [modify] https://crrev.com/d92ba9dc7a7a271c450c2bb48d44e9ff838c0422/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/d92ba9dc7a7a271c450c2bb48d44e9ff838c0422/content/browser/frame_host/navigator_impl.cc
,
Sep 20 2017
I can confirm that that flag was set to "Default" previously, and that when set to "Disabled" the bug no longer occurs.
,
Sep 21 2017
Requesting merge to M62.
,
Sep 21 2017
factormystic@ - the fix from #c31 should now be present in the most recent canary - 63.0.3221.0
,
Sep 21 2017
The bug is marked as P3 or Feature. It should not be merged as M62 is in beta. Please contact the approriate milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 21 2017
I think the bug had wrong priority assigned to it. This bug was reported by an external user and so according to https://www.chromium.org/for-testers/bug-reporting-guidelines/triage-best-practices the bug should be at *least* P2 (since for P2 the guidelines say "Low/ No user impact"). I think the merge request should be reviewed by a human. - The fix from #c31 is relatively simple (slightly tweaking calculations within a single function / touching only a few lines of code). - The bug impacts end users (although I am not sure how exactly widespread the problem is) - Before the fix, the problem would consistently repro when PlzNavigate was enabled.
,
Sep 21 2017
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 25 2017
Thanks for the fix lukasza@ and providing more details. Approving merge to M62. Branch:3202
,
Sep 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eeab97173bc339f8eedd47f0cdf267bba41d135f commit eeab97173bc339f8eedd47f0cdf267bba41d135f Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Mon Sep 25 19:26:03 2017 Avoid firing beforeunload event for same document navigations. TBR=lukasza@chromium.org (cherry picked from commit d92ba9dc7a7a271c450c2bb48d44e9ff838c0422) Bug: 736785 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I0af5431812de23e692b16fe23d677a71806fe26c Reviewed-on: https://chromium-review.googlesource.com/673065 Reviewed-by: Camille Lamy <clamy@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#503175} Reviewed-on: https://chromium-review.googlesource.com/682650 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#434} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/eeab97173bc339f8eedd47f0cdf267bba41d135f/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/eeab97173bc339f8eedd47f0cdf267bba41d135f/content/browser/frame_host/navigator_impl.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by kavvaru@chromium.org
, Jun 27 2017Components: Blink>JavaScript
Labels: Needs-Feedback