Implement redirect handling on MojoAsyncResourceHandler

Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for loading with mojo.

BUG=603396
Committed: https://crrev.com/7153dc4799a3cea581c8d125990dc84d12fef576
Cr-Commit-Position: refs/heads/master@{#432447}

[yhirano, 2016-11-01 11:28:41]

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

[commit-bot: I haz the power, 2016-11-01 11:28:59]

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

[yhirano, 2016-11-01 11:30:59]

Description was changed from

==========
Add redirect handling on MojoAsyncResourceHandler

This CL introduces the redirect handling for Loading with mojo.

BUG=603396
==========

to

==========
Add redirect handling on MojoAsyncResourceHandler

This CL introduces the redirect handling for Loading with mojo.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-01 11:41:16]

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

[commit-bot: I haz the power, 2016-11-01 11:41:17]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[yhirano, 2016-11-02 03:28:04]

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

[commit-bot: I haz the power, 2016-11-02 03:28:18]

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, 2016-11-02 04:05:22]

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

[commit-bot: I haz the power, 2016-11-02 04:05:23]

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...)

[yhirano, 2016-11-02 06:34:05]

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

[commit-bot: I haz the power, 2016-11-02 06:34:15]

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

[yhirano, 2016-11-02 06:45:24]

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

[commit-bot: I haz the power, 2016-11-02 06:45:40]

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

[yhirano, 2016-11-02 07:02:40]

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

[commit-bot: I haz the power, 2016-11-02 07:02:57]

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

[yhirano, 2016-11-02 07:04:40]

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

[commit-bot: I haz the power, 2016-11-02 07:05:01]

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, 2016-11-02 07:39:38]

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

[commit-bot: I haz the power, 2016-11-02 07:39:39]

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...)

[yhirano, 2016-11-04 07:14:03]

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

[commit-bot: I haz the power, 2016-11-04 07:14:20]

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

[yhirano, 2016-11-04 07:22:31]

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

[commit-bot: I haz the power, 2016-11-04 07:22:49]

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

[yhirano, 2016-11-04 07:35:53]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-11-04 07:35:57]

Patchset #1 (id:1) has been deleted

[yhirano, 2016-11-04 07:36:02]

Patchset #1 (id:40001) has been deleted

[yhirano, 2016-11-04 07:36:06]

Patchset #1 (id:60001) has been deleted

[yhirano, 2016-11-04 07:36:11]

Patchset #1 (id:80001) has been deleted

[yhirano, 2016-11-04 07:36:17]

Patchset #1 (id:100001) has been deleted

[yhirano, 2016-11-04 07:38:20]

Description was changed from

==========
Add redirect handling on MojoAsyncResourceHandler

This CL introduces the redirect handling for Loading with mojo.

BUG=603396
==========

to

==========
Add redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for Loading with mojo.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-04 08:09:27]

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

[commit-bot: I haz the power, 2016-11-04 08:09:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-11-09 04:17:41]

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

[commit-bot: I haz the power, 2016-11-09 04:18:02]

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, 2016-11-09 04:30:35]

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

[commit-bot: I haz the power, 2016-11-09 04:30:36]

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

[yhirano, 2016-11-09 05:14:27]

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

[commit-bot: I haz the power, 2016-11-09 05:14:35]

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, 2016-11-09 05:46:08]

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

[commit-bot: I haz the power, 2016-11-09 05:46:09]

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

[yhirano, 2016-11-09 05:53:40]

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

[commit-bot: I haz the power, 2016-11-09 05:53:52]

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

[yhirano, 2016-11-09 05:57:20]

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

[commit-bot: I haz the power, 2016-11-09 05:57:31]

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

[yhirano, 2016-11-09 05:59:49]

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

[commit-bot: I haz the power, 2016-11-09 06:00:02]

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

[yhirano, 2016-11-09 06:04:36]

Patchset #5 (id:200001) has been deleted

[yhirano, 2016-11-09 06:04:38]

Patchset #5 (id:180002) has been deleted

[yhirano, 2016-11-09 06:04:45]

Patchset #4 (id:180001) has been deleted

[yhirano, 2016-11-09 06:04:47]

Patchset #3 (id:160001) has been deleted

[yhirano, 2016-11-09 06:04:53]

Patchset #2 (id:140001) has been deleted

[yhirano, 2016-11-09 06:04:57]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-11-09 06:39:01]

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

[commit-bot: I haz the power, 2016-11-09 06:39:02]

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

[yhirano, 2016-11-09 06:42:24]

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

[commit-bot: I haz the power, 2016-11-09 06:42:32]

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

[yhirano, 2016-11-09 07:41:57]

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

[commit-bot: I haz the power, 2016-11-09 07:42:11]

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

[yhirano, 2016-11-09 07:42:51]

Patchset #1 (id:230001) has been deleted

[yhirano, 2016-11-09 07:43:23]

Description was changed from

==========
Add redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for Loading with mojo.

BUG=603396
==========

to

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for Loading with mojo.

BUG=603396
==========

[yhirano, 2016-11-09 07:43:35]

yhirano@chromium.org changed reviewers:
+ tzik@chromium.org

[yhirano, 2016-11-09 07:44:12]

Description was changed from

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for Loading with mojo.

BUG=603396
==========

to

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for loading with mojo.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-09 09:20:48]

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

[commit-bot: I haz the power, 2016-11-09 09:20:49]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-11-10 05:58:55]

lgtm

[yhirano, 2016-11-10 06:17:59]

yhirano@chromium.org changed reviewers:
+ dcheng@chromium.org, horo@chromium.org, mmenke@chromium.org,
nasko@chromium.org

[yhirano, 2016-11-10 06:18:00]

+mmenke@ for content/browser/loader.
+horo@ for content/{browser, renderer}/service_worker.
+nasko@ for content not covered above.
+dcheng@ for mojom.

Thanks,

[dcheng, 2016-11-10 07:09:27]

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:268: DCHECK(did_defer_);
Is it safe to allow this to be called multiple times? This is called from the
renderer, and an exploited renderer could just invoke FollowRedirect() multiple
times.

(Basically, I'm wondering if this should be a DCHECK or if it's something we
need to check for and handle)

[horo, 2016-11-10 11:57:26]

https://codereview.chromium.org/2467833002/diff/270001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:289:
url_loader_ = nullptr;
We should provide more specific error message to client.
Please merge https://codereview.chromium.org/2485423004 to this cl.

[mmenke, 2016-11-10 21:42:41]

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:268: DCHECK(did_defer_);
On 2016/11/10 07:09:27, dcheng wrote:
> Is it safe to allow this to be called multiple times? This is called from the
> renderer, and an exploited renderer could just invoke FollowRedirect()
multiple
> times.
> 
> (Basically, I'm wondering if this should be a DCHECK or if it's something we
> need to check for and handle)

Good question...It looks like it's actually safe (We tear down the redirect and
try again), but that's just luck.  We should probably just kill the renderer if
this happens.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:394: void
MojoAsyncResourceHandler::OnWritable(MojoResult unused) {
nit:  I think the style is to not name unused variables?  Also should echo the
name in the header.  "MojoResult /*result*/" or "/*=result*/", or whatnot.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.h:90: void Resume();
This method no longer exists.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:1112:
ASSERT_TRUE(url_loader_client_.has_received_redirect());
Check that there was only one received redirect call?

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:1117:
EXPECT_EQ(1, resource_controller_.num_resume_calls());
Let's continue this request to completion.  Otherwise, we don't know if the
async resource handler's state machine continues to work after following the
redirect.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/test_url_loader_client.h (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/test_url_loader_client.h:40: bool has_received_redirect()
const { return has_received_redirect_; }
Can we count the number of redirects or something, make sure we only receive the
expected single redirect?

[yhirano, 2016-11-11 03:40:02]

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

[commit-bot: I haz the power, 2016-11-11 03:40:17]

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

[yhirano, 2016-11-11 03:41:36]

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

[commit-bot: I haz the power, 2016-11-11 03:42:09]

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, 2016-11-11 03:44:45]

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

[commit-bot: I haz the power, 2016-11-11 03:44:46]

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

[yhirano, 2016-11-11 03:54:15]

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

[commit-bot: I haz the power, 2016-11-11 03:54:29]

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, 2016-11-11 05:22:52]

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

[commit-bot: I haz the power, 2016-11-11 05:22:53]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-11 09:47:04]

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:268: DCHECK(did_defer_);
On 2016/11/10 21:42:40, mmenke wrote:
> On 2016/11/10 07:09:27, dcheng wrote:
> > Is it safe to allow this to be called multiple times? This is called from
the
> > renderer, and an exploited renderer could just invoke FollowRedirect()
> multiple
> > times.
> > 
> > (Basically, I'm wondering if this should be a DCHECK or if it's something we
> > need to check for and handle)
> 
> Good question...It looks like it's actually safe (We tear down the redirect
and
> try again), but that's just luck.  We should probably just kill the renderer
if
> this happens.

Done. It's now cancelling the request, I don't know what is the preferred manner
for the malformed request with mojo.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:394: void
MojoAsyncResourceHandler::OnWritable(MojoResult unused) {
On 2016/11/10 21:42:40, mmenke wrote:
> nit:  I think the style is to not name unused variables?  Also should echo the
> name in the header.  "MojoResult /*result*/" or "/*=result*/", or whatnot.

Done.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.h:90: void Resume();
On 2016/11/10 21:42:40, mmenke wrote:
> This method no longer exists.

Done.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:1112:
ASSERT_TRUE(url_loader_client_.has_received_redirect());
On 2016/11/10 21:42:41, mmenke wrote:
> Check that there was only one received redirect call?

Done in TestResourceClient.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:1117:
EXPECT_EQ(1, resource_controller_.num_resume_calls());
On 2016/11/10 21:42:41, mmenke wrote:
> Let's continue this request to completion.  Otherwise, we don't know if the
> async resource handler's state machine continues to work after following the
> redirect.

Done.

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
File content/browser/loader/test_url_loader_client.h (right):

https://codereview.chromium.org/2467833002/diff/270001/content/browser/loader...
content/browser/loader/test_url_loader_client.h:40: bool has_received_redirect()
const { return has_received_redirect_; }
On 2016/11/10 21:42:41, mmenke wrote:
> Can we count the number of redirects or something, make sure we only receive
the
> expected single redirect?

I added EXPECT_FALSE(has_received_redirect_) in the .cc file.

https://codereview.chromium.org/2467833002/diff/270001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2467833002/diff/270001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:289:
url_loader_ = nullptr;
On 2016/11/10 11:57:26, horo wrote:
> We should provide more specific error message to client.
> Please merge https://codereview.chromium.org/2485423004 to this cl.

Done.

[horo, 2016-11-11 14:53:05]

service_worker: lgtm

[mmenke, 2016-11-11 15:47:18]

https://codereview.chromium.org/2467833002/diff/330001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/330001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:274:
controller()->Cancel();
Seems like we should have a test for this.

Should we also have a test that we correctly allow two redirects?  Nothing
currently depends on the "did_defer_on_redirect_ = false" line, either.

[yhirano, 2016-11-14 07:30:42]

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

[commit-bot: I haz the power, 2016-11-14 07:30:47]

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, 2016-11-14 07:32:45]

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

[commit-bot: I haz the power, 2016-11-14 07:32:46]

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_...)

[yhirano, 2016-11-14 07:32:50]

Patchset #6 (id:350001) has been deleted

[yhirano, 2016-11-14 07:38:12]

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

[commit-bot: I haz the power, 2016-11-14 07:38:27]

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

[yhirano, 2016-11-14 07:40:50]

Patchset #6 (id:370001) has been deleted

[yhirano, 2016-11-14 07:41:09]

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

[commit-bot: I haz the power, 2016-11-14 07:41:17]

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, 2016-11-14 07:46:09]

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

[commit-bot: I haz the power, 2016-11-14 07:46:10]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[yhirano, 2016-11-14 07:47:46]

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

[commit-bot: I haz the power, 2016-11-14 07:47:59]

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

[yhirano, 2016-11-14 08:56:05]

https://codereview.chromium.org/2467833002/diff/330001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/330001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:274:
controller()->Cancel();
On 2016/11/11 15:47:17, mmenke wrote:
> Seems like we should have a test for this.
> 
> Should we also have a test that we correctly allow two redirects?  Nothing
> currently depends on the "did_defer_on_redirect_ = false" line, either.

Done.

[commit-bot: I haz the power, 2016-11-14 09:51:47]

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

[commit-bot: I haz the power, 2016-11-14 09:51:49]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-11-14 15:36:14]

content/browser/loader LGTM

[yhirano, 2016-11-14 16:09:58]

yhirano@chromium.org changed reviewers:
+ avi@chromium.org

[yhirano, 2016-11-14 16:09:59]

+avi@ as a content OWNER.

[Avi (use Gerrit), 2016-11-14 16:13:18]

lgtm

stampity stamp

[dcheng, 2016-11-15 07:41:41]

https://codereview.chromium.org/2467833002/diff/410001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/410001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:278:
DCHECK(!did_defer_on_writing_);
So in general, loading is a large state machine. If the renderer gets out of
sync, I think we should probably be calling mojo::ReportBadMessage(...) to kill
it, right? Are there reasons where we'd want to continue?

(Also, I'm not sure if we're consistently doing this now; do you know what
happens for legacy IPC?)

[yhirano, 2016-11-15 08:04:22]

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

[commit-bot: I haz the power, 2016-11-15 08:04:39]

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

[yhirano, 2016-11-15 08:08:24]

https://codereview.chromium.org/2467833002/diff/410001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2467833002/diff/410001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler.cc:278:
DCHECK(!did_defer_on_writing_);
On 2016/11/15 07:41:41, dcheng wrote:
> So in general, loading is a large state machine. If the renderer gets out of
> sync, I think we should probably be calling mojo::ReportBadMessage(...) to
kill
> it, right? Are there reasons where we'd want to continue?
> 
> (Also, I'm not sure if we're consistently doing this now; do you know what
> happens for legacy IPC?)

This dcheck is not affected by a malformed renderer. L272-L275 is the section
for the issue, and PS8 calls mojo::ReportBadMessage.

[dcheng, 2016-11-15 08:19:13]

mojo lgtm

cc +rockot, because I'm curious if there's a better way to detect a bad message
for unit testing

[yhirano, 2016-11-15 09:40:36]

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

[commit-bot: I haz the power, 2016-11-15 09:40:49]

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, 2016-11-15 09:45:29]

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

[commit-bot: I haz the power, 2016-11-15 09:45:30]

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

[yhirano, 2016-11-16 09:00:40]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-16 09:00:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org,
horo@chromium.org, avi@chromium.org, mmenke@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2467833002/#ps450001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-16 09:01:00]

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, 2016-11-16 09:43:18]

Description was changed from

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for loading with mojo.

BUG=603396
==========

to

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for loading with mojo.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-16 09:43:20]

Committed patchset #9 (id:450001)

[commit-bot: I haz the power, 2016-11-16 09:46:58]

Description was changed from

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for loading with mojo.

BUG=603396
==========

to

==========
Implement redirect handling on MojoAsyncResourceHandler

This CL introduces redirect handling for loading with mojo.

BUG=603396

Committed: https://crrev.com/7153dc4799a3cea581c8d125990dc84d12fef576
Cr-Commit-Position: refs/heads/master@{#432447}
==========

[commit-bot: I haz the power, 2016-11-16 09:47:00]

Patchset 9 (id:??) landed as
https://crrev.com/7153dc4799a3cea581c8d125990dc84d12fef576
Cr-Commit-Position: refs/heads/master@{#432447}