Under review: https://chromium-review.googlesource.com/c/chromium/src/+/1056587

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27f8891c7cd9a86b2c92508cf6818726df4058d6

commit 27f8891c7cd9a86b2c92508cf6818726df4058d6
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu May 24 13:17:58 2018

Migrate ContentHashFetcher to SimpleURLLoader

This CL migrates ContentHashFetcher away from URLRequestFetcher to
SimpleURLLoader.

In order to accomplish this, a few other related changes are also performed:

1) The regular flow of the code is ContentVerifier -> ContentHash ->
   ContentHashFetcher. In ContentVerifier, for instance, UI, IO and
   "File tasks" threads take place.
   Previously, the URLRequestFetcher logic residing in ContentHashFetcher
   executed in the "file tasks" thread.

   As part of the migration from URLRequestFetcher to SimpleURLLoader
   machinery, this CL also changes the ContentHashFetcher logic to
   execute to the IO thread.
   Note that it could be possible to keep the new SimpleURLLoader logic
   in the "file tasks" thread. However, this would impose a way bigger change,
   and require unittests to be considerably rewritten as well (see for
   patchsets 7, 8 and 9.
   The issue is that mojo objects are not thread safe. We could create a new
   URLLoaderFactory for the extensions thread, but then we'd need to create a
   new one when the network service crashes (Something this SharedURLLoaderFactory
   one, already bound to the IOThread, magically handles), but that would be
   significantly more complicated.

2) content_verifier_hash_fetch_behavior_browsertest.cc was changed
   to support running with the network service feature both enabled
   and disabled. This is a pattern that is also present in various other
   unittests.

In summary, the migration to use SimpleURLLoader performed here includes
also a change in the thread that ContentHashFetcher runs on, but no
functionality change is expected from this CL.

BUG=773295, 844926 

Change-Id: If570f69d01ff75ac59d8d043f8687621336dddcf
Reviewed-on: https://chromium-review.googlesource.com/1056587
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561480}
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/chrome/browser/extensions/content_verifier_hash_fetch_behavior_browsertest.cc
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/BUILD.gn
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/DEPS
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_hash_fetcher_unittest.cc
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_verifier.cc
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_verifier.h
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_verifier/content_hash.cc
[modify] https://crrev.com/27f8891c7cd9a86b2c92508cf6818726df4058d6/extensions/browser/content_verifier/content_hash.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/082ca997aba8426949fb918d8f38f552100d96b0

commit 082ca997aba8426949fb918d8f38f552100d96b0
Author: Gabriel Charette <gab@chromium.org>
Date: Thu May 24 18:47:07 2018

Revert "Migrate ContentHashFetcher to SimpleURLLoader"

This reverts commit 27f8891c7cd9a86b2c92508cf6818726df4058d6.

Reason for revert: suspect for  crbug.com/846417 

Original change's description:
> Migrate ContentHashFetcher to SimpleURLLoader
> 
> This CL migrates ContentHashFetcher away from URLRequestFetcher to
> SimpleURLLoader.
> 
> In order to accomplish this, a few other related changes are also performed:
> 
> 1) The regular flow of the code is ContentVerifier -> ContentHash ->
>    ContentHashFetcher. In ContentVerifier, for instance, UI, IO and
>    "File tasks" threads take place.
>    Previously, the URLRequestFetcher logic residing in ContentHashFetcher
>    executed in the "file tasks" thread.
> 
>    As part of the migration from URLRequestFetcher to SimpleURLLoader
>    machinery, this CL also changes the ContentHashFetcher logic to
>    execute to the IO thread.
>    Note that it could be possible to keep the new SimpleURLLoader logic
>    in the "file tasks" thread. However, this would impose a way bigger change,
>    and require unittests to be considerably rewritten as well (see for
>    patchsets 7, 8 and 9.
>    The issue is that mojo objects are not thread safe. We could create a new
>    URLLoaderFactory for the extensions thread, but then we'd need to create a
>    new one when the network service crashes (Something this SharedURLLoaderFactory
>    one, already bound to the IOThread, magically handles), but that would be
>    significantly more complicated.
> 
> 2) content_verifier_hash_fetch_behavior_browsertest.cc was changed
>    to support running with the network service feature both enabled
>    and disabled. This is a pattern that is also present in various other
>    unittests.
> 
> In summary, the migration to use SimpleURLLoader performed here includes
> also a change in the thread that ContentHashFetcher runs on, but no
> functionality change is expected from this CL.
> 
> BUG=773295, 844926 
> 
> Change-Id: If570f69d01ff75ac59d8d043f8687621336dddcf
> Reviewed-on: https://chromium-review.googlesource.com/1056587
> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561480}

TBR=lazyboy@chromium.org,rdevlin.cronin@chromium.org,mmenke@chromium.org,tonikitoo@igalia.com

Change-Id: I31235cf26cfff34aac0ff1e4047575ed2881722b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 773295,  844926 ,  846417 
Reviewed-on: https://chromium-review.googlesource.com/1072110
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561572}
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/chrome/browser/extensions/content_verifier_hash_fetch_behavior_browsertest.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/BUILD.gn
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/DEPS
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_hash_fetcher_unittest.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier.h
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier/content_hash.cc
[modify] https://crrev.com/082ca997aba8426949fb918d8f38f552100d96b0/extensions/browser/content_verifier/content_hash.h

Reopening for another take on this.

 Issue 840416  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82937e5541f6a5b9213e369df7a848da9384b15f

commit 82937e5541f6a5b9213e369df7a848da9384b15f
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Wed Jun 06 21:37:19 2018

Remove FetchParam parameter from fetch closure

The closure in case is ContentHash::DidFetchVerifiedContents
which takes a |const FetchParams&| parameter, but does not use it.
This CL removes it.

This is a driven by simplication as part of the preparation for
making FetchParams non-copyable.

Next, FetchParams will store a scoped_refptr<network::SharedURLLoaderFactory>
rather than a raw network::mojom::URLLoaderFactory pointer.

BUG= 844926 

Change-Id: I237c93be558f4cc7c65c5de2f73e8722a246863d
Reviewed-on: https://chromium-review.googlesource.com/1089272
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#565043}
[modify] https://crrev.com/82937e5541f6a5b9213e369df7a848da9384b15f/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/82937e5541f6a5b9213e369df7a848da9384b15f/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/82937e5541f6a5b9213e369df7a848da9384b15f/extensions/browser/content_verifier/content_hash.cc
[modify] https://crrev.com/82937e5541f6a5b9213e369df7a848da9384b15f/extensions/browser/content_verifier/content_hash.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f473308f56d073ec3d2acd9c265be32ed5fba2d

commit 0f473308f56d073ec3d2acd9c265be32ed5fba2d
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu Jun 07 18:58:34 2018

Make ContentHash::FetchParam non-copyable

We are in the process of migrating ContentHashFetcher from
URLRequestFetcher to SimpleURLLoader (see  bug 844926 ).

For this, we need to stop storing URLRequestContextGetter
raw pointers in ContentHash::FetchParam. Instead it will store
a std::unique_ptr<network::SharedURLLoaderFactoryInfo> instance,
which is non-copyable.

This is a preparation CL that adapts ContentHash::FetchParam
to hold non-copyable members. For this, it removes both copy-ctor
and the assignment overload (=), and uses std::move to pass
FetchParam instances around.

BUG= 844926 

Change-Id: I417543ddd377eef39146fcce69c325a35c499f6c
Reviewed-on: https://chromium-review.googlesource.com/1089273
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565368}
[modify] https://crrev.com/0f473308f56d073ec3d2acd9c265be32ed5fba2d/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/0f473308f56d073ec3d2acd9c265be32ed5fba2d/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/0f473308f56d073ec3d2acd9c265be32ed5fba2d/extensions/browser/content_hash_fetcher_unittest.cc
[modify] https://crrev.com/0f473308f56d073ec3d2acd9c265be32ed5fba2d/extensions/browser/content_verifier.cc
[modify] https://crrev.com/0f473308f56d073ec3d2acd9c265be32ed5fba2d/extensions/browser/content_verifier/content_hash.cc
[modify] https://crrev.com/0f473308f56d073ec3d2acd9c265be32ed5fba2d/extensions/browser/content_verifier/content_hash.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0285cde6e11a1178dbdb30a3503ae6603526c88

commit f0285cde6e11a1178dbdb30a3503ae6603526c88
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Mon Jun 18 21:51:38 2018

Reland "Migrate ContentHashFetcher to SimpleURLLoader"

This is a reland of 27f8891c7cd9a86b2c92508cf6818726df4058d6

The main difference from the original CL is that instead of passing raw
network::mojom::URLLoaderFactory pointers, this CL takes a more conservative
approach where a thread-agnostic network::mojom::URLLoaderFactoryPtrInfo instance
is passed through ContentHash::FetchParams, which allows us to "bind" the
network::URLLoaderFactoryPtr instance only when it is actually going to be used
(in ContentHashFetcher::Start).

Also, differently than the original/reverted CL, this CL does not change
the threads ContentHash methods run on.

Last, in order to adapt ContentVerifierHashTest, we:

1) Replace the use of net::TestURLRequestInterceptor by content::URLLoaderInterceptor.
2) Install the interceptor at the beginning of each test execution flow, so
that it is able to intercept all relevant URLLoaderFactory creation requests.

Some other minor differences, were because of some preparation CLs already landed,
eg https://crrev.com/c/1089273.

Original change's description:
> Migrate ContentHashFetcher to SimpleURLLoader
>
> This CL migrates ContentHashFetcher away from URLRequestFetcher to
> SimpleURLLoader.
>
> In order to accomplish this, a few other related changes are also performed:
>
> 1) The regular flow of the code is ContentVerifier -> ContentHash ->
>    ContentHashFetcher. In ContentVerifier, for instance, UI, IO and
>    "File tasks" threads take place.
>    Previously, the URLRequestFetcher logic residing in ContentHashFetcher
>    executed in the "file tasks" thread.
>
>    As part of the migration from URLRequestFetcher to SimpleURLLoader
>    machinery, this CL also changes the ContentHashFetcher logic to
>    execute to the IO thread.
>    Note that it could be possible to keep the new SimpleURLLoader logic
>    in the "file tasks" thread. However, this would impose a way bigger change,
>    and require unittests to be considerably rewritten as well (see for
>    patchsets 7, 8 and 9.
>    The issue is that mojo objects are not thread safe. We could create a new
>    URLLoaderFactory for the extensions thread, but then we'd need to create a
>    new one when the network service crashes (Something this SharedURLLoaderFactory
>    one, already bound to the IOThread, magically handles), but that would be
>    significantly more complicated.
>
> 2) content_verifier_hash_fetch_behavior_browsertest.cc was changed
>    to support running with the network service feature both enabled
>    and disabled. This is a pattern that is also present in various other
>    unittests.
>
> In summary, the migration to use SimpleURLLoader performed here includes
> also a change in the thread that ContentHashFetcher runs on, but no
> functionality change is expected from this CL.
>
> BUG=773295, 844926 
>
> Change-Id: If570f69d01ff75ac59d8d043f8687621336dddcf
> Reviewed-on: https://chromium-review.googlesource.com/1056587
> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561480}

Bug: 773295,  844926 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ie2f3665bcca5378ff67bf78bec38bdc225ff6c26
Reviewed-on: https://chromium-review.googlesource.com/1076947
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568178}
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/chrome/browser/extensions/content_verifier_hash_fetch_behavior_browsertest.cc
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/BUILD.gn
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/DEPS
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_hash_fetcher.cc
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_hash_fetcher.h
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_hash_fetcher_unittest.cc
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_verifier.cc
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_verifier.h
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_verifier/content_hash.cc
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/extensions/browser/content_verifier/content_hash.h
[modify] https://crrev.com/f0285cde6e11a1178dbdb30a3503ae6603526c88/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb02821074ba14de3d6a503ef204b694b25017a4

commit cb02821074ba14de3d6a503ef204b694b25017a4
Author: Matt Menke <mmenke@chromium.org>
Date: Tue Jun 19 15:40:30 2018

Annotate many of the browser_tests disabled under the NetworkService.

Also remove a couple that either no longer exist, have been disabled
generally due to flakiness, or are now passing.

BUG= 844950 ,  844951 ,  844952 ,  853251 ,  844928 ,
BUG= 843205 ,  844949 ,  844925 ,  844939 , 821021,
BUG=853798,  844973 ,  844927 ,  844926 ,  844950 

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I094a012fe2076c7badf86a094140c7d74db183be
Reviewed-on: https://chromium-review.googlesource.com/1104802
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568464}
[modify] https://crrev.com/cb02821074ba14de3d6a503ef204b694b25017a4/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/edc06504938b6b1038d682e3ac94b718cc8e8ad9

commit edc06504938b6b1038d682e3ac94b718cc8e8ad9
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Tue Aug 07 02:11:06 2018

fixup! Reland "Migrate ContentHashFetcher to SimpleURLLoader"

Now that [1] is fixed, we do not need to install the URLInterceptor
as the first thing we do when bootstrap'ing ContentVerifierHashTest.

This CL is a follow up of [2]. It reverts some of the flow changed by
[2] now that we can.

[1] https://crrev.com/c/1117822 (Fix for URLLoaderInterceptor with StoragePartition).
[2] https://crrev.com/c/1076947 (Reland "Migrate ContentHashFetcher to SimpleURLLoader").

BUG= 844926 

Change-Id: I1f4b2651b3f14196c1523109e8ade5f9cdb3ecac
Reviewed-on: https://chromium-review.googlesource.com/1163862
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#581095}
[modify] https://crrev.com/edc06504938b6b1038d682e3ac94b718cc8e8ad9/chrome/browser/extensions/content_verifier_hash_fetch_behavior_browsertest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77c6418c8821725bd51bd2931d665369e5db3f68

commit 77c6418c8821725bd51bd2931d665369e5db3f68
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Tue Aug 14 19:25:11 2018

fixup! Reland "Migrate ContentHashFetcher to SimpleURLLoader"

This CL is a trivial fixup of https://crrev.com/c/1076947; it deletes
some left-over code in ContentVerifier::BindURLLoaderFactoryRequestOnUIThread:
|network::mojom::URLLoaderFactoryParamsPtr params|.

TBR=rdevlin.cronin@chromium.org

Bug= 844926 

Change-Id: I493c29034ba61b59a99a4c9d2fd0ebd1a5717145
Reviewed-on: https://chromium-review.googlesource.com/1174654
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#582993}
[modify] https://crrev.com/77c6418c8821725bd51bd2931d665369e5db3f68/extensions/browser/content_verifier.cc