How come our Win10 Intel HD 630 bot didn't catch this regression?

https://ci.chromium.org/buildbot/chromium.gpu.fyi/Win10%20FYI%20Release%20%28Intel%20HD%20630%29/

Hmm, I think what's going on here is an unfortunate combination of behavior of XHR when downloading to a blob (which behind the scenes downloads to a file), and that CL of mine trying to prevent blink from reading files that have modified after blink gains access to them. The network code that writes the data to a file doesn't actually close the file until some arbitrary time after it has finished writing, which I guess could in some cases result in the blob backed by the file being created before the file is closed. Since on windows it is closing the file that updates the modified timestamp, that could result in some race conditions where the blob system sees the wrong timestamp.

The good news is that I'm rewriting the XHR side of this, and the new code shouldn't have the same problems. But that new code isn't going to be backported to M66, so that doesn't directly help for this case.

Reverting the fix for 710190 would of course unbreak this, which I guess I might end up doing, but that would be unfortunate as that was arguably a security bug...

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

commit 724fb40f190432710bc7d6ab31c1eba1b1fe69e9
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Tue Mar 27 21:53:14 2018

Revert "Make sure kFile BlobDataItems have a expected_modification_time."

This reverts commit 9b568e65a707b8a5abad91ddc464d158efdb95d9.

Reason for revert: this breaks XHR downloading to blobs, since the file that writes to might not be closed/updated until after the blob is created.

Original change's description:
> Make sure kFile BlobDataItems have a expected_modification_time.
> 
> Having expected_modification_time set guards us against files changing
> out from under us, and accidentally exposing those changes to the web.
> Without this change any files selected in <input> elements would be
> perpetually readable with whatever the file on disk changed to.
> 
> Bug: 710190
> Change-Id: I4b05ccfbe2d3c42116769d442bcec853973ff67c
> Reviewed-on: https://chromium-review.googlesource.com/920783
> Commit-Queue: Daniel Murphy <dmurph@chromium.org>
> Reviewed-by: Daniel Murphy <dmurph@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#538331}

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

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 710190,  826178 
Change-Id: I7f742f6d63d39a546c441203e5815abf5d0ba4b2
Reviewed-on: https://chromium-review.googlesource.com/981502
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546278}
[modify] https://crrev.com/724fb40f190432710bc7d6ab31c1eba1b1fe69e9/storage/browser/blob/blob_data_item.cc
[modify] https://crrev.com/724fb40f190432710bc7d6ab31c1eba1b1fe69e9/storage/browser/blob/blob_data_item.h
[modify] https://crrev.com/724fb40f190432710bc7d6ab31c1eba1b1fe69e9/storage/browser/blob/blob_storage_context.cc
[modify] https://crrev.com/724fb40f190432710bc7d6ab31c1eba1b1fe69e9/third_party/WebKit/LayoutTests/fast/filesystem/resources/file-writer-sync-write-overlapped.js
[delete] https://crrev.com/9126ccb11f468eb77b5188237c4fb809101e417e/third_party/WebKit/LayoutTests/http/tests/local/fileapi/file-changed-after-drop-expected.txt
[delete] https://crrev.com/9126ccb11f468eb77b5188237c4fb809101e417e/third_party/WebKit/LayoutTests/http/tests/local/fileapi/file-changed-after-drop.html
[delete] https://crrev.com/9126ccb11f468eb77b5188237c4fb809101e417e/third_party/WebKit/LayoutTests/http/tests/local/fileapi/script-tests/file-changed-after-drop.js

oops, of course still have to backport the revert to M66.

The bug cannot be reproduced if you run tests in all/conformance/textures/image_bitmap_from_blob separately. It can only be reproduced when we run these tests together.

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), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

How safe is this merge overall? Why is this critical for M66 vs waiting until M67?

It's a revert of a change that caused regressions in M66. Essentially the change had the unfortunate side effect of breaking XMLHttpRequest downloading to a blob. Merge should be perfectly safe since it just gets us back to the state the code had in M65.

Approving merge to M66. Branch:3359

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

commit a00e7f7ffb6cdc7bfc6d2779f19110e7681986a3
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Mar 30 22:44:55 2018

Revert "Make sure kFile BlobDataItems have a expected_modification_time."

This reverts commit 9b568e65a707b8a5abad91ddc464d158efdb95d9.

Reason for revert: this breaks XHR downloading to blobs, since the file that writes to might not be closed/updated until after the blob is created.

Original change's description:
> Make sure kFile BlobDataItems have a expected_modification_time.
>
> Having expected_modification_time set guards us against files changing
> out from under us, and accidentally exposing those changes to the web.
> Without this change any files selected in <input> elements would be
> perpetually readable with whatever the file on disk changed to.
>
> Bug: 710190
> Change-Id: I4b05ccfbe2d3c42116769d442bcec853973ff67c
> Reviewed-on: https://chromium-review.googlesource.com/920783
> Commit-Queue: Daniel Murphy <dmurph@chromium.org>
> Reviewed-by: Daniel Murphy <dmurph@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#538331}

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


(cherry picked from commit 724fb40f190432710bc7d6ab31c1eba1b1fe69e9)

Bug: 710190,  826178 
Change-Id: I7f742f6d63d39a546c441203e5815abf5d0ba4b2
Reviewed-on: https://chromium-review.googlesource.com/981502
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#546278}
Reviewed-on: https://chromium-review.googlesource.com/988956
Cr-Commit-Position: refs/branch-heads/3359@{#514}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/a00e7f7ffb6cdc7bfc6d2779f19110e7681986a3/storage/browser/blob/blob_data_item.cc
[modify] https://crrev.com/a00e7f7ffb6cdc7bfc6d2779f19110e7681986a3/storage/browser/blob/blob_data_item.h
[modify] https://crrev.com/a00e7f7ffb6cdc7bfc6d2779f19110e7681986a3/storage/browser/blob/blob_storage_context.cc
[modify] https://crrev.com/a00e7f7ffb6cdc7bfc6d2779f19110e7681986a3/third_party/WebKit/LayoutTests/fast/filesystem/resources/file-writer-sync-write-overlapped.js
[delete] https://crrev.com/c976556d63702d59736c380e645d9c9ea3199b3e/third_party/WebKit/LayoutTests/http/tests/local/fileapi/file-changed-after-drop-expected.txt
[delete] https://crrev.com/c976556d63702d59736c380e645d9c9ea3199b3e/third_party/WebKit/LayoutTests/http/tests/local/fileapi/file-changed-after-drop.html
[delete] https://crrev.com/c976556d63702d59736c380e645d9c9ea3199b3e/third_party/WebKit/LayoutTests/http/tests/local/fileapi/script-tests/file-changed-after-drop.js