Fixes for the failing URLLoaderFactoryImplTest content_unittests with PlzNavigate enabled

Fixes for the failing URLLoaderFactoryImplTest content_unittests with PlzNavigate enabled

The main resource requests which the content_unittests were making using
mojo were failing in RDHI because the renderer is not supposed to make
main resource requests with PlzNavigate. Worked around this by setting
the process type to PROCESS_TYPE_UNKNOWN in ResourceMessageFilter for
these unittests and added a check in the related code to only enforce
these checks for PROCESS_TYPE_RENDERER.

The other change was in the TestURLLoaderClient methods which run the
message loop waiting for states like response received, body received etc.
For some reason, with PlzNavigate enabled the response for the body
arrives immediately following the header response causing the next wait to
just block. The fixes were to see if already have a response_body_ in the
TestURLLoaderClient::RunUntilResponseBodyArrived method and return
immediately and a similar check in TestURLLoaderClient::RunUntilComplete()

BUG=439423
Committed: https://crrev.com/6c452f51321725e3831cdcf5747cd46623d98b26
Cr-Commit-Position: refs/heads/master@{#414640}

[ananta, 2016-08-25 18:57:32]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2016-08-25 19:06:33]

ananta@chromium.org changed reviewers:
+ clamy@chromium.org

[ananta, 2016-08-25 19:06:33]

+clamy

[ananta, 2016-08-25 20:26:32]

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

[commit-bot: I haz the power, 2016-08-25 20:27:21]

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-08-25 20:53:09]

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

[commit-bot: I haz the power, 2016-08-25 20:53:10]

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

[clamy, 2016-08-25 22:17:56]

Thanks! It does seem to cause a bit of failures though. Could you have a look?

As an alternative approach, I wonder if you could create a
TestNavigationURLLoader in the PlzNavigate case (instead of calling
BeginNavigation). This is what we do in some ResourceDispatcherHostTests.

Other options: provide a blob url so that it becomes a valid type of request
from the renderer/change the resource type to a subresource type.

https://codereview.chromium.org/2283473002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2283473002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1285: if ((process_type
== PROCESS_TYPE_RENDERER) && is_navigation_stream_request
Could you add a comment explaining that this is due to tests?

[ananta, 2016-08-25 22:48:36]

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

[ananta, 2016-08-25 22:49:15]

The tests were crashing due to a deref of a null filter_ ptr in RDHI. That is
fixed now. PTAL.

https://codereview.chromium.org/2283473002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2283473002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1285: if ((process_type
== PROCESS_TYPE_RENDERER) && is_navigation_stream_request
On 2016/08/25 22:17:55, clamy wrote:
> Could you add a comment explaining that this is due to tests?

Done.

[commit-bot: I haz the power, 2016-08-25 22:49:45]

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

[clamy, 2016-08-25 23:14:04]

Thanks! Lgtm with nits.

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

https://codereview.chromium.org/2283473002/diff/20001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1285: // The
process_type check is to ensure that we don't block unittests from
nit: s/we don't block unittests/unittests are not blocked

https://codereview.chromium.org/2283473002/diff/20001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1694: filter_ ?
(filter_->process_type() == PROCESS_TYPE_RENDERER) : false;
nit: The child_id != -1 below also checks if this is a renderer-initiated
request. I think it can be removed from the condition.

[ananta, 2016-08-25 23:39:20]

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

https://codereview.chromium.org/2283473002/diff/20001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1285: // The
process_type check is to ensure that we don't block unittests from
On 2016/08/25 23:14:04, clamy wrote:
> nit: s/we don't block unittests/unittests are not blocked

Done.

https://codereview.chromium.org/2283473002/diff/20001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1694: filter_ ?
(filter_->process_type() == PROCESS_TYPE_RENDERER) : false;
On 2016/08/25 23:14:04, clamy wrote:
> nit: The child_id != -1 below also checks if this is a renderer-initiated
> request. I think it can be removed from the condition.

Done.

[ananta, 2016-08-26 00:35:55]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-08-26 00:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2283473002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2016-08-26 00:36:29]

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-08-26 02:57:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-26 03:03:59]

Description was changed from

==========
Fixes for the failing URLLoaderFactoryImplTest content_unittests with
PlzNavigate enabled

The main resource requests which the content_unittests were making using
mojo were failing in RDHI because the renderer is not supposed to make
main resource requests with PlzNavigate. Worked around this by setting
the process type to PROCESS_TYPE_UNKNOWN in ResourceMessageFilter for
these unittests and added a check in the related code to only enforce
these checks for PROCESS_TYPE_RENDERER.

The other change was in the TestURLLoaderClient methods which run the
message loop waiting for states like response received, body received etc.
For some reason, with PlzNavigate enabled the response for the body
arrives immediately following the header response causing the next wait to
just block. The fixes were to see if already have a response_body_ in the
TestURLLoaderClient::RunUntilResponseBodyArrived method and return
immediately and a similar check in TestURLLoaderClient::RunUntilComplete()

BUG=439423
==========

to

==========
Fixes for the failing URLLoaderFactoryImplTest content_unittests with
PlzNavigate enabled

The main resource requests which the content_unittests were making using
mojo were failing in RDHI because the renderer is not supposed to make
main resource requests with PlzNavigate. Worked around this by setting
the process type to PROCESS_TYPE_UNKNOWN in ResourceMessageFilter for
these unittests and added a check in the related code to only enforce
these checks for PROCESS_TYPE_RENDERER.

The other change was in the TestURLLoaderClient methods which run the
message loop waiting for states like response received, body received etc.
For some reason, with PlzNavigate enabled the response for the body
arrives immediately following the header response causing the next wait to
just block. The fixes were to see if already have a response_body_ in the
TestURLLoaderClient::RunUntilResponseBodyArrived method and return
immediately and a similar check in TestURLLoaderClient::RunUntilComplete()

BUG=439423

Committed: https://crrev.com/6c452f51321725e3831cdcf5747cd46623d98b26
Cr-Commit-Position: refs/heads/master@{#414640}
==========

[commit-bot: I haz the power, 2016-08-26 03:04:00]

Patchset 3 (id:??) landed as
https://crrev.com/6c452f51321725e3831cdcf5747cd46623d98b26
Cr-Commit-Position: refs/heads/master@{#414640}