Cannot download from new anchor while changing history state.
Reported by
shakt...@gmail.com,
Mar 23 2018
|
|||||||||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.162 Safari/537.36
Steps to reproduce the problem:
1. Open the attached file history_save_bug.html in Chrome.
2. Click the "save" button.
What is the expected behavior?
A file called "untitled.txt" should be saved to your download folder. Chrome used to behave this way, and as far as I can tell, other browsers still do.
Or, if saving the file fails, a helpful error message would be appropriate.
What went wrong?
No file is saved, and no error message is displayed in the developer console. Changing the history state in the middle of trying to send a ClickEvent to an anchor to download a file causes the download to fail.
Did this work before? Yes I believe it worked in version 64.
Does this work in other browsers? Yes
Chrome version: 65.0.3325.162 Channel: stable
OS Version: OS X 10.13.2
Flash Version:
This script works if you remove the line:
history.back();
Alternatively, it *also* works if you use a setTimeout to execute this line:
anchor.dispatchEvent(new MouseEvent("click"));
Even if the timeout delay is zero.
In more complex tests, I noticed that the file would sometimes successfully save, but sometimes not, implying some kind of race condition.
It also fails to navigate to the data url if you comment out this line:
anchor.download = "untitled.txt";
However, if the anchor is being used to navigate to a url instead of download a file, it's less clear what the correct behavior is when combined with history.back(), since these two commands are conceptually opposed.
,
Mar 23 2018
Network triager here. There's no request sent to network stack, doesn't seem to be network related issue. Removing Network label, feel free to assign back if you believe this is inappropriate.
,
Mar 23 2018
It will be useful to bisect if this used to work so we have an idea when this behavior has regressed. It is not clear to me why the history API calls are needed, but it seems that the history.back() basically cancels the navigation to data: URL. I would expect the history.back() to work if done through setTimeout(), which will allow the data: URL navigation (which ends up resulting in the download) to be started without being cancelled.
,
Mar 26 2018
shaktool@ Thanks for the issue. Tested this issue on Mac OS 10.12.6, Windows 10 and Ubuntu 14.04 on the latest Canary 67.0.3379.0 and Stable 65.0.3325.181 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. Adding ReleaseBlock-Stable as this is a recent regression. Please feel free to remove it if it is not applicable. Thanks.
,
Mar 26 2018
I don't think this should be blocking any potential stable respins. I'd leave it up to jochen@ to make a call whether this is acceptable change in behavior or not, since I'm not as familiar with the reason it was changed.
,
Mar 26 2018
Thank you nasko@.
,
Mar 26 2018
Reproduce the issue on Chrome 66.0.3359.52/CrOS 10452.27.0 - Enguarde
,
Mar 29 2018
It appears we can revert the suspect CL cleanly. https://chromium-review.googlesource.com/#/c/chromium/src/+/985959/ I am not entirely clear if we want to revert it, but if we consider this to be a RBS regression, and the engineering team involved with the CL is all out of the office, we may have no better option.
,
Mar 29 2018
Well, at least the revert applies cleanly on ToT, this needs some work to figure out a backport.
,
Mar 29 2018
At this point if the patch owner returns on Monday I am holding off on landing the revert, we should try to have a fix for this early next week though, if we do another 65 stable release we would want this to be fixed in it.
,
Mar 29 2018
Not sure what makes you say it applies cleanly on ToT? If I try to rebase the CL you linked to on ToT I get several dozen merge conflicts that git can't figure out without help...
,
Mar 29 2018
Issue 820103 has been merged into this issue.
,
Mar 30 2018
The CL was what I got pressing the revert button in the Gerrit UI, that should only work if it applies cleanly. I was surprised it did, and even more that it would to master but not to the release branches closer to the landing point.
,
Mar 30 2018
Yeah, it doesn't seem like the Gerrit UI actually does any kind of rebasing on ToT. If you click on the git hash in that CL (https://chromium.googlesource.com/chromium/src/+/f7ee5f99842b861a68d0fec39c58d46f3f3c48d0) you can see that it actually just reverted it right on top of the original commit, so of course it applied cleanly there.
,
Apr 2 2018
Weird, my bad and apologies for the confusion. Jochen, have you had a chance to look at this today? I am looking at spinning another 65 stable this week and we would like to get a fix in asap.
,
Apr 2 2018
Just a heads up, M66 Stable cut is on April 12th, 10 days away. This issue is marked as RB-Stable for 66. Please make sure to address this issue prior to stable cut. Thanks!
,
Apr 3 2018
I don't think that we'll have a patch for 65, dropping the label accordingly
,
Apr 4 2018
,
Apr 9 2018
Reminder: Please note that M66 Stable is only 7 days away. This bug has been marked as ReleaseBlock Stable for M66. So please take a look and appropriately address this bug.
,
Apr 10 2018
that should be fixed now (for same origin downloads)
,
Apr 10 2018
[Auto-generated comment by a script] We noticed that this issue is targeted for M-66; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-66 label, otherwise remove Merge-TBD label. Thanks.
,
Apr 10 2018
,
Apr 10 2018
Partner team have verified the fix works as expected in R67 (OS-R67-10558.0.0), and have requested merge to R66. Is merge to R66 possible at this point?
,
Apr 11 2018
it's already merged to 66 |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by ellyjo...@chromium.org
, Mar 23 2018Labels: OS-Linux
Status: Untriaged (was: Unconfirmed)