The Appcache subresource URL factory needs to inform the URLLoaderClient if there is a failure.

The Appcache subresource URL factory needs to inform the URLLoaderClient if there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a request handler, etc.
We already notify the client about the latter case above. Need to do this for an invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the default network loader.

BUG=715632
TEST=Covered by browser test. AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation

Review-Url: https://codereview.chromium.org/2991443002
Cr-Commit-Position: refs/heads/master@{#489710}
Committed: https://chromium.googlesource.com/chromium/src/+/4a9dd98efab25e78f28b800520c1868aaad2cb1f

[ananta, 2017-07-21 21:11:33]

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

[ananta, 2017-07-21 21:11:34]

https://codereview.chromium.org/2991443002/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2991443002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:434: : (info_ ?
info_->http_response_info() : nullptr);
info_ was null in some tests leading to a crash

https://codereview.chromium.org/2991443002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:447: : (info_ ?
info_->response_data_size() : 0);
ditto

[ananta, 2017-07-21 21:11:42]

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

[commit-bot: I haz the power, 2017-07-21 21:11:55]

Dry run: CQ is trying da patch.

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

[jam, 2017-07-21 21:21:59]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-07-21 21:22:25]

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

[commit-bot: I haz the power, 2017-07-21 21:22:34]

Dry run: CQ is trying da patch.

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

[jam, 2017-07-21 21:26:39]

nice, content/renderer lgtm

Can you add a browsertest for this bug?

[ananta, 2017-07-21 21:49:09]

On 2017/07/21 21:26:39, jam wrote:
> nice, content/renderer lgtm
> 
> Can you add a browsertest for this bug?

Will do that in a later patchset.

[commit-bot: I haz the power, 2017-07-22 00:34:38]

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

[commit-bot: I haz the power, 2017-07-22 00:34:39]

Dry run: This issue passed the CQ dry run.

[jam, 2017-07-23 23:06:32]

On 2017/07/21 21:26:39, jam wrote:
> nice, content/renderer lgtm
> 
> Can you add a browsertest for this bug?

Generally it's better to add the test with the change. Since this isn't an
urgent patch afaik, any reason to not do that?

[michaeln, 2017-07-24 22:15:05]

lgtm 2

[ananta, 2017-07-25 00:30:17]

Description was changed from

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
==========

to

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation
==========

[ananta, 2017-07-25 00:30:32]

On 2017/07/23 23:06:32, jam wrote:
> On 2017/07/21 21:26:39, jam wrote:
> > nice, content/renderer lgtm
> > 
> > Can you add a browsertest for this bug?
> 
> Generally it's better to add the test with the change. Since this isn't an
> urgent patch afaik, any reason to not do that?

Thanks. Added a browser test for the same.

[ananta, 2017-07-25 00:31:04]

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

[commit-bot: I haz the power, 2017-07-25 00:31:16]

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-07-25 00:58:30]

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

[commit-bot: I haz the power, 2017-07-25 00:58:31]

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

[jam, 2017-07-25 01:00:05]

On 2017/07/25 00:30:32, ananta wrote:
> On 2017/07/23 23:06:32, jam wrote:
> > On 2017/07/21 21:26:39, jam wrote:
> > > nice, content/renderer lgtm
> > > 
> > > Can you add a browsertest for this bug?
> > 
> > Generally it's better to add the test with the change. Since this isn't an
> > urgent patch afaik, any reason to not do that?
> 
> Thanks. Added a browser test for the same.


Thanks for the test. It looks like the test would still pass without the fix
though? i.e. it's checking that the page navigated correctly, which it will. but
it needs to check what happens with the subresource load, since that's where the
fix is.

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_browsertest.cc (right):

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:28:
command_line->AppendSwitchASCII("enable-features",
nit: use content::kEnableFeatures

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:29:
features::kNetworkService.name);
nit: indentation

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:33:
host_resolver()->AddRule("*", "127.0.0.1");
are you sure you need this? this is for tests that navigate to arbitrary
origins, which this test isn't since it's getting its URLs from the embedded
test server

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:43: if
(!base::FeatureList::IsEnabled(features::kNetworkService))
nit: this isn't needed since the test sets it

[ananta, 2017-07-25 01:14:28]

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_browsertest.cc (right):

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:28:
command_line->AppendSwitchASCII("enable-features",
On 2017/07/25 01:00:05, jam wrote:
> nit: use content::kEnableFeatures

Done.

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:29:
features::kNetworkService.name);
On 2017/07/25 01:00:05, jam wrote:
> nit: indentation

Done.

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:33:
host_resolver()->AddRule("*", "127.0.0.1");
On 2017/07/25 01:00:05, jam wrote:
> are you sure you need this? this is for tests that navigate to arbitrary
> origins, which this test isn't since it's getting its URLs from the embedded
> test server

Thanks. Removed.

https://codereview.chromium.org/2991443002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_browsertest.cc:43: if
(!base::FeatureList::IsEnabled(features::kNetworkService))
On 2017/07/25 01:00:05, jam wrote:
> nit: this isn't needed since the test sets it

Removed

[ananta, 2017-07-25 01:14:39]

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

[commit-bot: I haz the power, 2017-07-25 01:14:48]

Dry run: CQ is trying da patch.

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

[ananta, 2017-07-25 01:42:12]

Description was changed from

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation
==========

to

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigationWithFactoryErrorOn
and
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigationWithFactoryErrorOff
==========

[ananta, 2017-07-25 01:43:48]

Added a way to indicate to the AppCache subresource factory that we are in test
mode.This turns off the error for a missing host which helps validate the
RenderFrameImpl fix that the factory gets cleared correctly.

Added two tests which test both these cases now.

[ananta, 2017-07-25 01:43:58]

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

[commit-bot: I haz the power, 2017-07-25 01:44:08]

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-07-25 03:10:18]

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

[commit-bot: I haz the power, 2017-07-25 03:10:19]

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

[jam, 2017-07-25 15:00:52]

https://codereview.chromium.org/2991443002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2991443002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_subresource_url_factory.h:71: // behaviors
like returning errors for a missing host, etc are turned off in
this seems specific to this one test; it's not intuitive what 'testing mode'
causes to fail, and a boolean won't scale for other tests that want different
behavior.

is there another way we can do this without changing the production code? what
if the test sets up a C++ handler for the EmbeddedTestServer using
RegisterRequestHandler (example:
https://cs.chromium.org/chromium/src/net/test/embedded_test_server/embedded_t...).
Then it can count how many times chrome goes to the network. If the old
(appcache) subresource loader is used, then we'd have fewer server hits than we
expect.

[ananta, 2017-07-25 22:26:50]

https://codereview.chromium.org/2991443002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2991443002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_subresource_url_factory.h:71: // behaviors
like returning errors for a missing host, etc are turned off in
On 2017/07/25 15:00:51, jam wrote:
> this seems specific to this one test; it's not intuitive what 'testing mode'
> causes to fail, and a boolean won't scale for other tests that want different
> behavior.
> 
> is there another way we can do this without changing the production code? what
> if the test sets up a C++ handler for the EmbeddedTestServer using
> RegisterRequestHandler (example:
>
https://cs.chromium.org/chromium/src/net/test/embedded_test_server/embedded_t...).
> Then it can count how many times chrome goes to the network. If the old
> (appcache) subresource loader is used, then we'd have fewer server hits than
we
> expect.

Thanks. done. I had to create a custom embedded server to ensure that the
handler gets added at the front of the other handlers. The base class also
creates a server. However it registers the default handlers before  us which
causes our handler to never get called.

[ananta, 2017-07-25 22:28:17]

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

[commit-bot: I haz the power, 2017-07-25 22:28:28]

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-07-25 23:41:54]

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

[commit-bot: I haz the power, 2017-07-25 23:41:55]

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

[ananta, 2017-07-26 00:02:10]

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

[commit-bot: I haz the power, 2017-07-26 00:02:18]

Dry run: CQ is trying da patch.

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

[ananta, 2017-07-26 00:15:02]

Description was changed from

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigationWithFactoryErrorOn
and
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigationWithFactoryErrorOff
==========

to

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation
==========

[jam, 2017-07-26 02:44:44]

lgtm, thanks!

[commit-bot: I haz the power, 2017-07-26 02:52:49]

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

[commit-bot: I haz the power, 2017-07-26 02:52:50]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-07-26 18:33:47]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-07-26 18:33:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2991443002/#ps120001
(title: "Disable the test for Android")

[commit-bot: I haz the power, 2017-07-26 18:33:57]

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-07-26 18:44:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1501094027447570,
"parent_rev": "4c63a5002110cb17e715ccae921ff6a5d8df7cb4", "commit_rev":
"4a9dd98efab25e78f28b800520c1868aaad2cb1f"}

[commit-bot: I haz the power, 2017-07-26 18:44:56]

Description was changed from

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation
==========

to

==========
The Appcache subresource URL factory needs to inform the URLLoaderClient if
there is a failure.

Failure cases include, an invalid AppCacheHost instance, failure to create a
request handler, etc.
We already notify the client about the latter case above. Need to do this for an
invalid host as well.
This caused the w3schools AppCache test page to spin indefinitely if we pass in
an invalid URL.

Additionally on the renderer side we need to reset the cached URLLoaderFactory
pointer if a null factory
is passed in the CommitNavigation IPC. This ensures that the request hits the
default network loader.

BUG=715632
TEST=Covered by browser test.
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation

Review-Url: https://codereview.chromium.org/2991443002
Cr-Commit-Position: refs/heads/master@{#489710}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a9dd98efab25e78f28b800520c1...
==========

[commit-bot: I haz the power, 2017-07-26 18:44:57]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/4a9dd98efab25e78f28b800520c1...

[mmenke, 2017-07-27 04:46:05]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2988923002/ by mmenke@chromium.org.

The reason for reverting is:
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation
is very flaky, with a lot of red try runs on the CQ and the builders.

Windows CQ has a 2 hour backlog (Or did earlier), and while this isn't the only
reason, it looks like a major contributor..

[mmenke, 2017-07-27 04:48:11]

On 2017/07/27 04:46:05, mmenke wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2988923002/ by mailto:mmenke@chromium.org.
> 
> The reason for reverting is:
>
AppCacheNetworkServiceBrowserTest.VerifySubresourceFactoryClearedOnNewNavigation
> is very flaky, with a lot of red try runs on the CQ and the builders.
> 
> Windows CQ has a 2 hour backlog (Or did earlier), and while this isn't the
only
> reason, it looks like a major contributor..

Sample failures (Both on Mac, but I believe I've seen it on other bots):

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.10_...

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.12_...