[BlobStorage] Enforcing renderer constraints to prevent broken blobs

[BlobStorage] Enforcing renderer constraints to prevent broken blobs

R=pwnall, yhirano
BUG=688100
Review-Url: https://codereview.chromium.org/2717583003
Cr-Commit-Position: refs/heads/master@{#460867}
Committed: https://chromium.googlesource.com/chromium/src/+/d94af97739e56d4174dc320b16d36f6795f2622f

[dmurph, 2017-02-24 20:09:24]

Victor & Yutaka, can you PTAL?

Thanks!
Daniel

[pwnall, 2017-02-27 23:04:57]

On 2017/02/24 20:09:24, dmurph wrote:
> Victor & Yutaka, can you PTAL?
> 
> Thanks!
> Daniel

Why are the checks here CHECK instead of DCHECK? Is the issue serious enough to
crash the renderer? Wouldn't it be better if the functionality is degraded
instead?

[dmurph, 2017-02-27 23:33:42]

On 2017/02/27 23:04:57, pwnall wrote:
> On 2017/02/24 20:09:24, dmurph wrote:
> > Victor & Yutaka, can you PTAL?
> > 
> > Thanks!
> > Daniel
> 
> Why are the checks here CHECK instead of DCHECK? Is the issue serious enough
to
> crash the renderer? Wouldn't it be better if the functionality is degraded
> instead?

My reasoning was that it's currently impossible to do this just from JS, so this
should be happening through indirect usage of blobs in the renderer. The example
attached bug was one such case, but it would not have been caught by a DCHECK.
Having this Check-fail I think would close that feedback loop much faster. Maybe
this is too harsh though. I'm trying to avoid situations like the above bug.

[pwnall, 2017-02-28 02:40:37]

On 2017/02/27 23:33:42, dmurph wrote:
> On 2017/02/27 23:04:57, pwnall wrote:
> > On 2017/02/24 20:09:24, dmurph wrote:
> > > Victor & Yutaka, can you PTAL?
> > > 
> > > Thanks!
> > > Daniel
> > 
> > Why are the checks here CHECK instead of DCHECK? Is the issue serious enough
> to
> > crash the renderer? Wouldn't it be better if the functionality is degraded
> > instead?
> 
> My reasoning was that it's currently impossible to do this just from JS, so
this
> should be happening through indirect usage of blobs in the renderer. The
example
> attached bug was one such case, but it would not have been caught by a DCHECK.
> Having this Check-fail I think would close that feedback loop much faster.
Maybe
> this is too harsh though. I'm trying to avoid situations like the above bug.

This means the buggy code didn't have test coverage though, right? IIRC, most CQ
bots use a release-with-asserts build, so the bug would've been caught if the
code was properly tested.

[yhirano, 2017-02-28 09:22:35]

lgtm, thank you.

[dmurph, 2017-02-28 21:22:50]

yeah, so this would have made canary builds crash, which is unfortunately the
only way this would have been detected - and detected at the root cause, instead
of complex debugging. Do you think this is inappropriate? Or is there a way for
me to do this that is more appropriate? Maybe more extreme warnings?

[dmurph, 2017-02-28 23:05:41]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[dmurph, 2017-02-28 23:06:08]

On 2017/02/28 21:22:50, dmurph wrote:
> yeah, so this would have made canary builds crash, which is unfortunately the
> only way this would have been detected - and detected at the root cause,
instead
> of complex debugging. Do you think this is inappropriate? Or is there a way
for
> me to do this that is more appropriate? Maybe more extreme warnings?

As discussed offline with Victor, moved to dchecks instead of checks.

[commit-bot: I haz the power, 2017-02-28 23:06:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-02-28 23:37:03]

LGTM w/ nits.

https://codereview.chromium.org/2717583003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/blob/BlobData.cpp (right):

https://codereview.chromium.org/2717583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/blob/BlobData.cpp:114:
CHECK_EQ(m_fileComposition, FileCompositionStatus::NO_UNKNOWN_SIZE_FILES)
This CHECK should also be a DCHECK, but it's not introduced in this CL, so I'm
ok with the CHECK being changed in a different CL.

https://codereview.chromium.org/2717583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/blob/BlobData.cpp:128:
CHECK_EQ(m_fileComposition, FileCompositionStatus::NO_UNKNOWN_SIZE_FILES)
This CHECK should also be a DCHECK, same comment as above.

[commit-bot: I haz the power, 2017-03-01 00:12:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:12:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kinuko, 2017-03-01 00:26:21]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-03-01 00:26:21]

lgtm

[dmurph, 2017-03-01 01:07:50]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 01:09:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-03-01 01:27:52]

Changed all checks to dchecks per comment. Done. Thanks!

[dmurph, 2017-03-01 01:27:56]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2017-03-01 01:28:02]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2017-03-01 01:28:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, kinuko@chromium.org, pwnall@chromium.org
Link to the patchset: https://codereview.chromium.org/2717583003/#ps60001
(title: "changed all to dcheck")

[commit-bot: I haz the power, 2017-03-01 01:28:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 02:44:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 02:44:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dmurph, 2017-03-01 03:28:58]

Good news - we found other broken callers. Will fix in patch and update soon.

[dmurph, 2017-03-04 01:04:33]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 01:04:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 02:27:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 02:27:27]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-03-06 21:13:45]

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/File.cpp:88: if (metadata.length ==
BlobDataItem::toEndOfFile) {
Tests?

[dmurph, 2017-03-06 23:43:29]

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/File.cpp:88: if (metadata.length ==
BlobDataItem::toEndOfFile) {
On 2017/03/06 21:13:45, pwnall wrote:
> Tests?

Tested by one or more of:
FileTest.fileSystemFileWithNativeSnapshot
V8ScriptValueSerializerTest.RoundTripFileNativeSnapshot
FileListTest.pathsForUserVisibleFiles
WebDragDataTest.items

[pwnall, 2017-03-06 23:45:12]

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/File.cpp:88: if (metadata.length ==
BlobDataItem::toEndOfFile) {
On 2017/03/06 23:43:29, dmurph wrote:
> On 2017/03/06 21:13:45, pwnall wrote:
> > Tests?
> 
> Tested by one or more of:
> FileTest.fileSystemFileWithNativeSnapshot
> V8ScriptValueSerializerTest.RoundTripFileNativeSnapshot
> FileListTest.pathsForUserVisibleFiles
> WebDragDataTest.items

Do we know for sure that we have coverage of both branches?

[dmurph, 2017-03-29 23:19:24]

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/2717583003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fileapi/File.cpp:88: if (metadata.length ==
BlobDataItem::toEndOfFile) {
On 2017/03/06 23:45:12, pwnall wrote:
> On 2017/03/06 23:43:29, dmurph wrote:
> > On 2017/03/06 21:13:45, pwnall wrote:
> > > Tests?
> > 
> > Tested by one or more of:
> > FileTest.fileSystemFileWithNativeSnapshot
> > V8ScriptValueSerializerTest.RoundTripFileNativeSnapshot
> > FileListTest.pathsForUserVisibleFiles
> > WebDragDataTest.items
> 
> Do we know for sure that we have coverage of both branches?

Checked. Added one test for case where we have the file length in metadata.

[dmurph, 2017-03-29 23:19:38]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2017-03-29 23:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yhirano@chromium.org, pwnall@chromium.org
Link to the patchset: https://codereview.chromium.org/2717583003/#ps100001
(title: "added test")

[commit-bot: I haz the power, 2017-03-29 23:20:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 01:23:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 01:23:18]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[dmurph, 2017-03-30 20:41:31]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:42:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:49:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490906491914330,
"parent_rev": "e6b2fb315a37379020c437f9e28d7b921e273410", "commit_rev":
"d94af97739e56d4174dc320b16d36f6795f2622f"}

[commit-bot: I haz the power, 2017-03-30 20:50:32]

Description was changed from

==========
[BlobStorage] Enforcing renderer constraints to prevent broken blobs

R=pwnall, yhirano
BUG=688100
==========

to

==========
[BlobStorage] Enforcing renderer constraints to prevent broken blobs

R=pwnall, yhirano
BUG=688100

Review-Url: https://codereview.chromium.org/2717583003
Cr-Commit-Position: refs/heads/master@{#460867}
Committed:
https://chromium.googlesource.com/chromium/src/+/d94af97739e56d4174dc320b16d3...
==========

[commit-bot: I haz the power, 2017-03-30 20:50:33]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d94af97739e56d4174dc320b16d3...