Fail when uploading blob is non-existant

Fail when uploading blob is non-existant

BUG=692860
Review-Url: https://codereview.chromium.org/2829923004
Cr-Commit-Position: refs/heads/master@{#476858}
Committed: https://chromium.googlesource.com/chromium/src/+/a9e68ce94c149e7322faaa09fddb3aa2c1e0a2de

[dmurph, 2017-04-20 19:25:35]

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

[commit-bot: I haz the power, 2017-04-20 19:26:13]

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-04-20 19:59:00]

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

[commit-bot: I haz the power, 2017-04-20 19:59:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dmurph, 2017-04-20 22:23:40]

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

[commit-bot: I haz the power, 2017-04-20 22:24:10]

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-04-20 22:26:29]

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

[commit-bot: I haz the power, 2017-04-20 22:26:30]

Dry run: 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_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dmurph, 2017-05-31 00:49:52]

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

[commit-bot: I haz the power, 2017-05-31 00:50:34]

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-05-31 03:53:30]

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

[commit-bot: I haz the power, 2017-05-31 03:53:32]

Dry run: This issue passed the CQ dry run.

[dmurph, 2017-05-31 21:56:10]

dmurph@chromium.org changed reviewers:
+ horo@chromium.org

[dmurph, 2017-05-31 21:56:10]

Horo, can you PTAL?

[michaeln, 2017-05-31 22:41:19]

a drive by comment

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1180:
&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
Looks like ContinuePendingBeginRequest can be deferred till later, do we need to
AttachRequestBodyBlobDataHandles before making this async function call?

[horo, 2017-06-01 02:04:48]

lgtm

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1180:
&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
On 2017/05/31 22:41:19, michaeln wrote:
> Looks like ContinuePendingBeginRequest can be deferred till later, do we need
to
> AttachRequestBodyBlobDataHandles before making this async function call?

OnHttpHeaderReceived() in chrome_content_browser_client_extensions_part.cc is
the only interceptor.
And it is calling the callback synchronously. 

So even if we call AttachRequestBodyBlobDataHandles() before it, the issue will
not be fixed.

[kinuko, 2017-06-01 05:50:04]

Could we add a similar code in navigation_url_loader_network_service.cc (a new
alternative code path for BeginNavigationRequest when some flags are on)?

[dmurph, 2017-06-01 18:18:29]

Since I couldn't find an obvious way to fail the request, I created a bug for
the new path and assigned to Kinuko:
https://bugs.chromium.org/p/chromium/issues/detail?id=728728

Thanks for the review

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1180:
&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
On 2017/06/01 02:04:48, horo wrote:
> On 2017/05/31 22:41:19, michaeln wrote:
> > Looks like ContinuePendingBeginRequest can be deferred till later, do we
need
> to
> > AttachRequestBodyBlobDataHandles before making this async function call?
> 
> OnHttpHeaderReceived() in chrome_content_browser_client_extensions_part.cc is
> the only interceptor.
> And it is calling the callback synchronously. 
> 
> So even if we call AttachRequestBodyBlobDataHandles() before it, the issue
will
> not be fixed.

Acknowledged.

[dmurph, 2017-06-01 18:18:32]

Since I couldn't find an obvious way to fail the request, I created a bug for
the new path and assigned to Kinuko:
https://bugs.chromium.org/p/chromium/issues/detail?id=728728

Thanks for the review

[dmurph, 2017-06-01 18:18:42]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2017-06-01 18:19:11]

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-06-01 18:29:15]

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

[commit-bot: I haz the power, 2017-06-01 18:29:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmurph, 2017-06-01 18:47:26]

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

[dmurph, 2017-06-01 18:47:27]

Whoops, Kinuko, can you PTAL for
content/browser/loader/resource_dispatcher_host_impl.cc approval?

[michaeln, 2017-06-01 20:46:59]

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1180:
&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
On 2017/06/01 02:04:48, horo wrote:
> On 2017/05/31 22:41:19, michaeln wrote:
> > Looks like ContinuePendingBeginRequest can be deferred till later, do we
need
> to
> > AttachRequestBodyBlobDataHandles before making this async function call?
> 
> OnHttpHeaderReceived() in chrome_content_browser_client_extensions_part.cc is
> the only interceptor.
> And it is calling the callback synchronously. 
> 
> So even if we call AttachRequestBodyBlobDataHandles() before it, the issue
will
> not be fixed

Oh well, I was hoping that might be the source of (some of) the missing blobs,
guess not.

Since the interceptor interface provides for an async completion code path, it'd
probably be good to rearrange the order so that a new interceptor, or a change
to the existing interceptor, that does use the async completion doesn't tickle
the latent bug.

[kinuko, 2017-06-02 03:15:40]

lgtm

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1180:
&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
On 2017/06/01 20:46:59, michaeln wrote:
> On 2017/06/01 02:04:48, horo wrote:
> > On 2017/05/31 22:41:19, michaeln wrote:
> > > Looks like ContinuePendingBeginRequest can be deferred till later, do we
> need
> > to
> > > AttachRequestBodyBlobDataHandles before making this async function call?
> > 
> > OnHttpHeaderReceived() in chrome_content_browser_client_extensions_part.cc
is
> > the only interceptor.
> > And it is calling the callback synchronously. 
> > 
> > So even if we call AttachRequestBodyBlobDataHandles() before it, the issue
> will
> > not be fixed
> 
> Oh well, I was hoping that might be the source of (some of) the missing blobs,
> guess not.
> 
> Since the interceptor interface provides for an async completion code path,
it'd
> probably be good to rearrange the order so that a new interceptor, or a change
> to the existing interceptor, that does use the async completion doesn't tickle
> the latent bug.

Moving AttachRequestBodyBlobDataHandle earlier also makes sense, dmurph: mind
changing the order a bit?  Doing it in a follow-up is also fine.

[dmurph, 2017-06-02 19:34:45]

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

[commit-bot: I haz the power, 2017-06-02 19:35:18]

Dry run: CQ is trying da patch.

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

[dmurph, 2017-06-02 19:38:07]

Done. Hopefully this doesn't break anything - it shouldn't it's just moving this
higher up the chain.

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1180:
&ResourceDispatcherHostImpl::ContinuePendingBeginRequest,
On 2017/06/02 03:15:40, kinuko wrote:
> On 2017/06/01 20:46:59, michaeln wrote:
> > On 2017/06/01 02:04:48, horo wrote:
> > > On 2017/05/31 22:41:19, michaeln wrote:
> > > > Looks like ContinuePendingBeginRequest can be deferred till later, do we
> > need
> > > to
> > > > AttachRequestBodyBlobDataHandles before making this async function call?
> > > 
> > > OnHttpHeaderReceived() in chrome_content_browser_client_extensions_part.cc
> is
> > > the only interceptor.
> > > And it is calling the callback synchronously. 
> > > 
> > > So even if we call AttachRequestBodyBlobDataHandles() before it, the issue
> > will
> > > not be fixed
> > 
> > Oh well, I was hoping that might be the source of (some of) the missing
blobs,
> > guess not.
> > 
> > Since the interceptor interface provides for an async completion code path,
> it'd
> > probably be good to rearrange the order so that a new interceptor, or a
change
> > to the existing interceptor, that does use the async completion doesn't
tickle
> > the latent bug.
> 
> Moving AttachRequestBodyBlobDataHandle earlier also makes sense, dmurph: mind
> changing the order a bit?  Doing it in a follow-up is also fine.

Done.

[dmurph, 2017-06-02 19:49:38]

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

[commit-bot: I haz the power, 2017-06-02 19:50:03]

Dry run: CQ is trying da patch.

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

[dmurph, 2017-06-02 19:53:19]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2017-06-02 19:53:24]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2017-06-02 19:53:24]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2829923004/#ps80001
(title: "reworked to be exactly the same, logically")

[commit-bot: I haz the power, 2017-06-02 19:54:01]

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-06-02 20:14:49]

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

[commit-bot: I haz the power, 2017-06-02 20:14:50]

Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmurph, 2017-06-02 21:18:52]

On 2017/06/02 20:14:50, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Moving that seemed to make everything crash - maybe somehow the blob storage
context is only available after a certain point? I'd rather not make functional
changes in this code, as I'm unfamiliar with this pipeline. Reverting the order
change.

[michaeln, 2017-06-02 21:36:36]

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

[michaeln, 2017-06-02 21:36:37]

https://codereview.chromium.org/2829923004/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (left):

https://codereview.chromium.org/2829923004/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1304:
GetBlobStorageContext(requester_info->blob_storage_context());
you could retain the local variable and avoid the extra param to the
ContinuePendingBeginRequest method

https://codereview.chromium.org/2829923004/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2829923004/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1214: headers,
std::move(mojo_request), std::move(url_loader_client), nullptr,
it's crashing because of this nullptr

[dmurph, 2017-06-02 23:00:34]

found the bug, doing what michael suggested.

https://codereview.chromium.org/2829923004/diff/80001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (left):

https://codereview.chromium.org/2829923004/diff/80001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1304:
GetBlobStorageContext(requester_info->blob_storage_context());
On 2017/06/02 21:36:37, michaeln wrote:
> you could retain the local variable and avoid the extra param to the
> ContinuePendingBeginRequest method

Done.

[dmurph, 2017-06-02 23:01:06]

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

[dmurph, 2017-06-02 23:01:14]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2017-06-02 23:01:18]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2017-06-02 23:01:19]

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

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

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-06-03 02:12:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496444478604590,
"parent_rev": "1862926fcdeb5bd96594c55e674947b9e04e111e", "commit_rev":
"a9e68ce94c149e7322faaa09fddb3aa2c1e0a2de"}

[commit-bot: I haz the power, 2017-06-03 02:13:08]

Description was changed from

==========
Fail when uploading blob is non-existant

BUG=692860
==========

to

==========
Fail when uploading blob is non-existant

BUG=692860

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

[commit-bot: I haz the power, 2017-06-03 02:13:10]

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