New issue
Advanced search Search tips

Issue 807639 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Cannot download file in Beta or Canary

Reported by hrlaws...@gmail.com, Jan 31 2018

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.119 Safari/537.36

Steps to reproduce the problem:
1. Download a file using blob
2. 
3. 

What is the expected behavior?
download file

What went wrong?
The webpage at blob:https://v360demo.xxxxxxxxxxxxxxxxxxxxx    might be temporarily down or it may have moved permanently to a new web address.
ERR_INVALID_RESPONSE

Did this work before? Yes 64

Does this work in other browsers? Yes

Chrome version: Version 65.0.3325.31 (Official Build) dev (64-bit)  Channel: beta
OS Version: 8
Flash Version: 

When using Chrome version 65 or 66 I cannot download a file using blob.  I get the error:
 

Comment 1 by hrlaws...@gmail.com, Jan 31 2018



This is an intermittent issue but mostly fails.
Labels: Needs-Triage-M65 Needs-Bisect
Labels: Needs-Triage-M64
Components: Internals>Services>Network Blink>Storage>FileSystem
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision ReleaseBlock-Stable Triaged-ET RegressedIn-65 M-65 FoundIn-66 Target-66 Target-65 FoundIn-65 OS-Linux OS-Mac Pri-1
Owner: jochen@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on Windows 10, mac 10.12.6 and Ubuntu 14.04 using chrome reported version #65.0.3325.31 and latest canary #66.0.3335.0. Issue is not seen in latest stable #64.0.3282.119.
Note: Use the url: https://www.google.com/url?q=https://jsfiddle.net/koldev/cW7W5/&sa=D&ust=1517552741539000&usg=AFQjCNHjMtpzyMs0sxyavarFGGQ_MFTqHA to test the issue.

Bisect Information:
=====================
Good build: 65.0.3309.0
Bad Build : 65.0.3310.0

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

From the above change log suspecting below change
Change-Id: I508d7abe1cba9b75d65132b0688984cdebfc6fd4
Reviewed-on: https://chromium-review.googlesource.com/758236

jochen@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.
Note: Adding stable blocker for M-65 as it is a recent regression. Please feel free to remove the same if not appropriate.

Thanks...!!
Cc: jsb...@chromium.org
thanks for the bug report.

The issue is that you revoke the blob URL before the download is ready. Those two operations are racing - I guess that's also the reason why it sometimes works and sometimes doesn't

jsbell@ can you chime in on the expected behavior here?
i.e. is it expected that revoking an blob url cancels any ongoing async operation?

Comment 7 by mek@chromium.org, Feb 1 2018

It is expected that this works. But known that it doesn't... Or at least that there are various race conditions around this. We do have at least one browser test for this case even (https://cs.chromium.org/chromium/src/content/test/data/download/download-attribute-blob.html?q=download-attribute-blob.html)

Comment 8 by mek@chromium.org, Feb 1 2018

And yes, using the navigation path for this probably made it worse, since before at least things were more or less appropriately ordered for the blob URL to be resolved before the revoke IPC gets handled. I'm working on fixing all of this as part of blob URL mojofication, but especially fixing the navigation codepath is rather hard/complicated.

Comment 9 by mek@chromium.org, Feb 1 2018

Cc: mek@chromium.org
(sorry for the repeated replies to myself)
What I'm essentially (trying to) do to fix this for real is resolve blob URLs as soon as possible on the blink side, and then passing a resulting URLLoaderFactory along with the blob URL to wherever eventually the blob URL needs to get fetch. This special factory will keep the URL alive (and will be requested over the same mojo pipe as the later URL revoke), and thus no more race conditions possible.

This is safe even for navigations since navigations to blob URLs are only allowed to same origin URLs, so even though the renderer can provide whatever content it wants for these URLs the browser can still check that the URL actually is same origin. The overall tracking bug for refactoring blob URL resolving/loading like this is issue 800901. But it's very unlikely that'll finish in time to turn on for M65, so wouldn't help directly for this bug.

Comment 10 by mek@chromium.org, Feb 1 2018

oh, and one more thing, in https://chromium-review.googlesource.com/c/chromium/src/+/894843 I'm adding tests for some related cases, which at least cover some of the navigation code path. The tests there I'm unfortunately adding as Pass/Failure expectations though, since, like this bug, chrome has flaky race conditions around navigation to blob URLs.

Comment 11 by mek@chromium.org, Feb 1 2018

Actually, only now noticed that we *had* a test for this case, until jochen@ removed that part of the test in the CL that is blamed for breaking this... Even though the test explicitly mentioned that it was trying to test this case (and still mentions that...)
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3dd29b25f01eef7bf726aef971d554ed153a4f0

commit a3dd29b25f01eef7bf726aef971d554ed153a4f0
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Feb 02 19:21:44 2018

Bounce Blob URL revoking through the UI thread to give navigation a chance.

Navigation IPCs are handled on the UI thread so if a blob URL is revoked
immediately after navigating the revocation will almost always happen before
the navigation code has had a chance to post a task back to the IO thread to
actually perform the request. With this change revocation is hopefully
delayed just long enough to make navigation work again.

Bug:  807639 
Change-Id: I2afde86dd203c61cbb03cc02ac62ec21c7e7d704
Reviewed-on: https://chromium-review.googlesource.com/899666
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534119}
[modify] https://crrev.com/a3dd29b25f01eef7bf726aef971d554ed153a4f0/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/a3dd29b25f01eef7bf726aef971d554ed153a4f0/content/test/data/download/download-attribute-blob.html

Cc: jochen@chromium.org
Owner: mek@chromium.org
Status: Fixed (was: Assigned)
Thank you!

Comment 14 by mek@chromium.org, Feb 2 2018

We probably still want to backport the fix to M65, assuming nothing disastrous happens with it in Canary/Dev, right?
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-65; 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-65 label, otherwise remove Merge-TBD label. Thanks.
Right, I'll request merge next Monday
Project Member

Comment 17 by bugdroid1@chromium.org, Feb 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16c3e9aee2a702a8df966f6d6bcb30ffb9e89268

commit 16c3e9aee2a702a8df966f6d6bcb30ffb9e89268
Author: Thomas Anderson <thomasanderson@chromium.org>
Date: Sat Feb 03 01:24:12 2018

Revert "Bounce Blob URL revoking through the UI thread to give navigation a chance."

This reverts commit a3dd29b25f01eef7bf726aef971d554ed153a4f0.

Reason for revert: Causing DownloadContentTest.DownloadAttributeBlobURL to fail on Linux Tests, Linux Tests (dbg)(1), and Linux Tests (dbg)(1)(32).
https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests/66991
https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests%20%28dbg%29%281%29%2832%29/47823
https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests%20%28dbg%29%281%29/70044

Original change's description:
> Bounce Blob URL revoking through the UI thread to give navigation a chance.
> 
> Navigation IPCs are handled on the UI thread so if a blob URL is revoked
> immediately after navigating the revocation will almost always happen before
> the navigation code has had a chance to post a task back to the IO thread to
> actually perform the request. With this change revocation is hopefully
> delayed just long enough to make navigation work again.
> 
> Bug:  807639 
> Change-Id: I2afde86dd203c61cbb03cc02ac62ec21c7e7d704
> Reviewed-on: https://chromium-review.googlesource.com/899666
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534119}

TBR=mek@chromium.org,jochen@chromium.org

Change-Id: Ia460136d52f214d3d2914a8004ebe65c87e90bc3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  807639 
Reviewed-on: https://chromium-review.googlesource.com/900442
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534230}
[modify] https://crrev.com/16c3e9aee2a702a8df966f6d6bcb30ffb9e89268/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/16c3e9aee2a702a8df966f6d6bcb30ffb9e89268/content/test/data/download/download-attribute-blob.html

Project Member

Comment 18 by bugdroid1@chromium.org, Feb 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a5561649b9567c394ef1ff1619745b02ed7759d

commit 8a5561649b9567c394ef1ff1619745b02ed7759d
Author: Reilly Grant <reillyg@chromium.org>
Date: Sat Feb 03 04:37:45 2018

Reland "Bounce Blob URL revoking through the UI thread to give navigation a chance."

This is a reland of a3dd29b25f01eef7bf726aef971d554ed153a4f0.

Original change's description:
> Bounce Blob URL revoking through the UI thread to give navigation a chance.
>
> Navigation IPCs are handled on the UI thread so if a blob URL is revoked
> immediately after navigating the revocation will almost always happen before
> the navigation code has had a chance to post a task back to the IO thread to
> actually perform the request. With this change revocation is hopefully
> delayed just long enough to make navigation work again.
>
> Bug:  807639 
> Change-Id: I2afde86dd203c61cbb03cc02ac62ec21c7e7d704
> Reviewed-on: https://chromium-review.googlesource.com/899666
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534119}

TBR=mek@chromium.org,jochen@chromium.org

Bug:  807639 , 808759 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I5033422d57ac7672e761045f7871c6aed29368b9
Reviewed-on: https://chromium-review.googlesource.com/900191
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534257}
[modify] https://crrev.com/8a5561649b9567c394ef1ff1619745b02ed7759d/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/8a5561649b9567c394ef1ff1619745b02ed7759d/content/test/data/download/download-attribute-blob.html
[modify] https://crrev.com/8a5561649b9567c394ef1ff1619745b02ed7759d/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Labels: Merge-Request-65
Project Member

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

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
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
Labels: TE-Verified-M66 TE-Verified-66.0.3340.0
Verified the fix on Mac 10.12.6, Win-10 and Ubuntu 14.04 using latest chrome version #66.0.3340.0 as per the comment #0 and #4.
Attaching screen cast for reference.
Observed that file got downloaded as expected.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
807639.mp4
956 KB View Download
Are you requesting a merge for cl listed at #18 to M65?
right, the CL was relanded by the sheriff after it turned out not to be the culprit
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 for CL listed at #18 to M65. Please merge ASAP so we can pick it up for tomorrow's last M65 dev release. Thank you.
Project Member

Comment 25 by bugdroid1@chromium.org, Feb 5 2018

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

commit 7b43f737ce7b421ca8717f604db045e7fbb4e7b3
Author: Jochen Eisinger <jochen@chromium.org>
Date: Mon Feb 05 17:05:22 2018

Reland "Bounce Blob URL revoking through the UI thread to give navigation a chance."

This is a reland of a3dd29b25f01eef7bf726aef971d554ed153a4f0.

Original change's description:
> Bounce Blob URL revoking through the UI thread to give navigation a chance.
>
> Navigation IPCs are handled on the UI thread so if a blob URL is revoked
> immediately after navigating the revocation will almost always happen before
> the navigation code has had a chance to post a task back to the IO thread to
> actually perform the request. With this change revocation is hopefully
> delayed just long enough to make navigation work again.
>
> Bug:  807639 
> Change-Id: I2afde86dd203c61cbb03cc02ac62ec21c7e7d704
> Reviewed-on: https://chromium-review.googlesource.com/899666
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#534119}

TBR=jochen@chromium.org, mek@chromium.org, reillyg@chromium.org

(cherry picked from commit 8a5561649b9567c394ef1ff1619745b02ed7759d)

Bug:  807639 , 808759 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I5033422d57ac7672e761045f7871c6aed29368b9
Reviewed-on: https://chromium-review.googlesource.com/900191
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#534257}
Reviewed-on: https://chromium-review.googlesource.com/902043
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#302}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/7b43f737ce7b421ca8717f604db045e7fbb4e7b3/content/browser/blob_storage/blob_dispatcher_host.cc
[modify] https://crrev.com/7b43f737ce7b421ca8717f604db045e7fbb4e7b3/content/test/data/download/download-attribute-blob.html
[modify] https://crrev.com/7b43f737ce7b421ca8717f604db045e7fbb4e7b3/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Labels: -Merge-TBD

Sign in to add a comment