New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 812293 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression



Sign in to add a comment

window.onbeforeunload is called when clicking download links

Project Member Reported by blois@google.com, Feb 14 2018

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.51 Safari/537.36

Steps to reproduce the problem:
1. Add a callback to window.onbeforeunload
2. Have an download link <a download="file.txt">
3. Click the download link

What is the expected behavior?
onbeforeunload is not called for download links.

What went wrong?
onbeforeunload is called for download links.

Did this work before? Yes 64.0.3282.140

Does this work in other browsers? Yes

Chrome version: 65.0.3325.51  Channel: beta
OS Version: 
Flash Version:
 
Labels: Needs-Bisect Needs-Triage-M65
Cc: clamy@chromium.org susan.boorgula@chromium.org
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision Triaged-ET RegressedIn-65 M-65 FoundIn-66 Target-66 Target-65 FoundIn-65 OS-Mac OS-Windows Pri-1
Owner: jochen@chromium.org
Status: Assigned (was: Unconfirmed)
blois@ Thanks for the issue.

Tested this issue on Ubuntu 14.04, Mac OS 10.12.6 and Windows 10 on the Beta 65.0.3325.73 and Canary 66.0.3347.0 and able to reproduce the issue by following the steps mentioned above.

Bisect Information:
===================
Good Build: 65.0.3309.0 (Revision - 526409)
Bad Build : 65.0.3310.0 (Revision - 526575)

On executing the per-revision bisect script, below is the Changelog URL:

https://chromium.googlesource.com/chromium/src/+log/cbce1149c9ae08297b816effbf456c02f97df8db..f2d2fe87028de36a489f7db3f5fb28da2e9d9b2b

From the above Changelog, suspecting the below change:
Reviewed-on:  https://chromium-review.googlesource.com/758236

jochen@ Please check and confirm if this issue is related to your change, else help us in assigning to the right owner.
CC'ing the reviewer of this issue clamy@, as this issue is a RBS issue and the author is OOO till the 18th of this month 

Adding ReleaseBlock-Stable as this is a recent regression. Please feel to remove the same if it is not applicable.

Thanks.

Comment 4 by jochen@chromium.org, Feb 18 2018

note that for general URLS, this is expected (also repros in Firefox). It seems that for blob URLs, there's a special case where it doesn't fire this event.

Comment 5 by jochen@chromium.org, Feb 20 2018

After talking to Anne, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1439498 - let's wait what they think

Comment 7 by jochen@chromium.org, Feb 26 2018

Status: Fixed (was: Assigned)

Comment 8 Deleted

Comment 9 by woxxom@gmail.com, Feb 27 2018

Will you merge it to Chrome 65?
 Issue 817009  has been merged into this issue.
Labels: ReleaseBlock-Stable
jochen@ Merged the  issue 817009  to this issue as it is similar.
Could you please merge the fix to M-65, as Stable is soon moving to M-65.

Hence adding ReleaseBlock-Stable. Please feel free to remove if this is not applicable.

Thanks..
//Adding to comment #11.

Able to reproduce this issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 on Beta 65.0.3325.88 and Dev 66.0.3355.0.
Issue seems to be fixed on Canary 66.0.3356.0 and Stable 64.0.3282.186.

Thanks..
Labels: Merge-Request-65
Project Member

Comment 14 by sheriffbot@chromium.org, Feb 28 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Before we approve merge to M65, could you pls confirm followings?

Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge?
Any other imp details to justify the merge.

Please note M65 is VERY close to Stable promotion so merge bar is very high. Thank you.

It's on today's canary. I think it's safe to merge, and has tests, however, given that the initial bug was only reported well into the beta period, I'm not sure how to quantify "baked".

I'm confident that the fix won't introduce crashers, nor will it make stuff worse than the reported bug.
Labels: OS-Android OS-Chrome OS-Fuchsia
Cc: cma...@chromium.org
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 branch based on comments #12, #16 and per offline chat with jochen@. 

+cmasso@ (Chrome on Android) TPM to keep her in loop.
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 28 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f8ea0c6058415c9d64a5a1e36f3d95b610ae2c8

commit 5f8ea0c6058415c9d64a5a1e36f3d95b610ae2c8
Author: Jochen Eisinger <jochen@chromium.org>
Date: Wed Feb 28 16:23:46 2018

Don't dispatch beforeunload events for downloads

BUG= 812293 
TBR=jochen@chromium.org

(cherry picked from commit cd05cee6edd1301f63d44306ff120820b6a4cfde)

Change-Id: I1375ecbdef13ea43ac350cdae6e550267166fadc
Reviewed-on: https://chromium-review.googlesource.com/934204
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#539097}
Reviewed-on: https://chromium-review.googlesource.com/941446
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#623}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/5f8ea0c6058415c9d64a5a1e36f3d95b610ae2c8/content/renderer/render_frame_impl.cc
[delete] https://crrev.com/a943a3711b1cbba1b07b0f16c279b2fddf515acb/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-area-element/area-download-click-expected.txt
[modify] https://crrev.com/5f8ea0c6058415c9d64a5a1e36f3d95b610ae2c8/third_party/WebKit/LayoutTests/external/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-404-expected.txt
[delete] https://crrev.com/a943a3711b1cbba1b07b0f16c279b2fddf515acb/third_party/WebKit/LayoutTests/external/wpt/html/semantics/text-level-semantics/the-a-element/a-download-click-expected.txt

Sign in to add a comment