Network service: SafeBrowsing check for frame-resources from browser.

Network service: SafeBrowsing check for frame-resources from browser.

This CL:
- adds safe_browsing::BrowserURLLoaderThrottle (which is a URLLoaderThrottle subclass) to throttle browser-side URL loading.

- adds ContentBrowserClient::CreateURLLoaderThrottles() and implements it in ChromeContentBrowserClient to return a safe_browsing::BrowserURLLoaderThrottle instance.

- changes NavigationURLLoaderNetworkService to use ThrottlingURLLoader on the IO thread. It no longer owns URLLoaderPtr or Binding<URLLoaderClient> on the UI thread. That means I had to proxy calls of these two interfaces between the IO and UI thread. But that is necessary because ThrottlingURLLoader needs to apply throttles living on the IO thread.

- changes ThrottlingURLLoader to also support StartLoaderCallback, which we use in place of URLLoaderFactory in the browser.

- uses PossiblyAssociatedInterfacePtr<URLLoader> and gets rid of AssociatedURLLoaderWrapper. It is less code and more efficient. (Eventually we will use non-associated URLLoaderPtr consistently.)

BUG=715673
Review-Url: https://codereview.chromium.org/2924723002
Cr-Commit-Position: refs/heads/master@{#480212}
Committed: https://chromium.googlesource.com/chromium/src/+/efcb7c795d6d84d1ab17e17d51efef624d79ce81

[yzshen1, 2017-06-10 00:19:53]

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

[commit-bot: I haz the power, 2017-06-10 00:20: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-06-10 00:22:32]

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

[commit-bot: I haz the power, 2017-06-10 00:22:33]

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

[yzshen1, 2017-06-10 00:37:59]

Description was changed from

==========
Network service: SafeBrowsing check for frame-resources from browser.
BUG=715673
==========

to

==========
Network service: SafeBrowsing check for frame-resources from browser.

This CL:
- adds SafeBrowsingBrowserThrottle (which is a URLLoaderThrottle subclass) to
throttle browser-side URL loading.

- adds ContentBrowserClient::CreateURLLoaderThrottles() and implements it in
ChromeContentBrowserClient to return a SafeBrowsingBrowserThrottle instance.

- changes NavigationURLLoaderNetworkService to use ThrottlingURLLoader on the IO
thread. It no longer owns URLLoaderPtr or Binding<URLLoaderClient> on the UI
thread. That means I had to proxy calls of these two interfaces between the IO
and UI thread. But that is necessary because ThrottlingURLLoader needs to apply
throttles living on the IO thread.

- changes ThrottlingURLLoader to also support StartLoaderCallback, which we use
in place of URLLoaderFactory in the browser.

- uses PossiblyAssociatedInterfacePtr<URLLoader> and gets rid of
AssociatedURLLoaderWrapper. It is less code and more efficient. (Eventually we
will use non-associated URLLoaderPtr consistently.)

BUG=715673
==========

[yzshen1, 2017-06-10 00:37:59]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org, kinuko@chromium.org, nparker@chromium.org, vakh@chromium.org

[yzshen1, 2017-06-10 00:43:19]

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

[commit-bot: I haz the power, 2017-06-10 00:43:47]

Dry run: CQ is trying da patch.

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

[yzshen1, 2017-06-10 00:43:54]

Hi, reviewers.

Would you please take a look? Thanks!

[yzshen1, 2017-06-10 00:43:55]

Hi, reviewers.

Would you please take a look? Thanks!

[commit-bot: I haz the power, 2017-06-10 02:08:43]

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

[commit-bot: I haz the power, 2017-06-10 02:08:44]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-06-12 12:39:19]

lgtm

https://codereview.chromium.org/2924723002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2924723002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:256:
base::WeakPtr<NavigationURLLoaderNetworkService> owner_;
Please comment that this is referenced only on UI thread.

[jam, 2017-06-12 15:49:45]

lgtm

in the future when we only have network service code path, we can move the
throttles and NavigationURLLoaderNetworkService to live on the UI thread only
right?

[jam, 2017-06-12 16:00:39]

also, are there now SB browser_tests that pass? If so, can we run them on the
bot (in a different cl)?

[yzshen1, 2017-06-12 20:04:16]

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

[commit-bot: I haz the power, 2017-06-12 20:05: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-06-12 20:09:47]

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

[commit-bot: I haz the power, 2017-06-12 20:09:48]

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...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yzshen1, 2017-06-12 20:12:31]

Thanks John and Kinuko.

> in the future when we only have network service code path, 
> we can move the
> throttles and NavigationURLLoaderNetworkService to live on > the UI thread
only right?

I haven't look at whether it makes sense to let (part of) the SafeBrowsing
database thread-safe or let it live on the UI thread. If it purely lives on the
UI thread, incoming mojo queries from renderers have to jump from the IO thread
to the UI thread so it won't save much.

It is something to investigate.

https://codereview.chromium.org/2924723002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2924723002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:256:
base::WeakPtr<NavigationURLLoaderNetworkService> owner_;
On 2017/06/12 12:39:19, kinuko wrote:
> Please comment that this is referenced only on UI thread.

Done.

[yzshen1, 2017-06-12 20:12:57]

On 2017/06/12 16:00:39, jam wrote:
> also, are there now SB browser_tests that pass? If so, can we run them on the
> bot (in a different cl)?

I will try them out and do that in a follow-up CL. Thanks!

[yzshen1, 2017-06-12 20:53:07]

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

[commit-bot: I haz the power, 2017-06-12 20:53:31]

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-06-12 22:39:25]

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

[commit-bot: I haz the power, 2017-06-12 22:39:26]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-06-15 21:52:59]

lgtm

Just to confirm my understanding:
There is no call flow between the following classes:
safe_browsing_browser_throttle, safe_browsing_renderer_throttle. Right?

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1187: "safe_browsing/safe_browsing_browser_throttle.cc",
Could you please create a target for these files in
chrome/browser/safe_browsing/BUILD.gn and add those targets as DEPS here?

I know you are following precedent, but the sooner we start doing it correctly,
the better.

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.h:385:
scoped_refptr<safe_browsing::SafeBrowsingService> safe_browsing_;
I'd prefer calling this safe_browsing_service_ or sb_service_

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_browser_throttle.cc (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_browser_throttle.cc:24:
during_destruction_ = true;
What's the use of setting this flag in the ~?
Once the object is destructed, this flag shouldn't be read anyway so its vale
shouldn't matter.

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_browser_throttle.h (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_browser_throttle.h:5: #ifndef
CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_BROWSER_THROTTLE_H_
Since this file is already under safe_browsing directory, do you mind
calling it browser_throttle.h ?

[yzshen1, 2017-06-16 21:24:57]

Description was changed from

==========
Network service: SafeBrowsing check for frame-resources from browser.

This CL:
- adds SafeBrowsingBrowserThrottle (which is a URLLoaderThrottle subclass) to
throttle browser-side URL loading.

- adds ContentBrowserClient::CreateURLLoaderThrottles() and implements it in
ChromeContentBrowserClient to return a SafeBrowsingBrowserThrottle instance.

- changes NavigationURLLoaderNetworkService to use ThrottlingURLLoader on the IO
thread. It no longer owns URLLoaderPtr or Binding<URLLoaderClient> on the UI
thread. That means I had to proxy calls of these two interfaces between the IO
and UI thread. But that is necessary because ThrottlingURLLoader needs to apply
throttles living on the IO thread.

- changes ThrottlingURLLoader to also support StartLoaderCallback, which we use
in place of URLLoaderFactory in the browser.

- uses PossiblyAssociatedInterfacePtr<URLLoader> and gets rid of
AssociatedURLLoaderWrapper. It is less code and more efficient. (Eventually we
will use non-associated URLLoaderPtr consistently.)

BUG=715673
==========

to

==========
Network service: SafeBrowsing check for frame-resources from browser.

This CL:
- adds safe_browsing::BrowserURLLoaderThrottle (which is a URLLoaderThrottle
subclass) to throttle browser-side URL loading.

- adds ContentBrowserClient::CreateURLLoaderThrottles() and implements it in
ChromeContentBrowserClient to return a safe_browsing::BrowserURLLoaderThrottle
instance.

- changes NavigationURLLoaderNetworkService to use ThrottlingURLLoader on the IO
thread. It no longer owns URLLoaderPtr or Binding<URLLoaderClient> on the UI
thread. That means I had to proxy calls of these two interfaces between the IO
and UI thread. But that is necessary because ThrottlingURLLoader needs to apply
throttles living on the IO thread.

- changes ThrottlingURLLoader to also support StartLoaderCallback, which we use
in place of URLLoaderFactory in the browser.

- uses PossiblyAssociatedInterfacePtr<URLLoader> and gets rid of
AssociatedURLLoaderWrapper. It is less code and more efficient. (Eventually we
will use non-associated URLLoaderPtr consistently.)

BUG=715673
==========

[yzshen1, 2017-06-16 21:27:09]

Thanks Varun!

> Just to confirm my understanding:
> There is no call flow between the following classes:
> safe_browsing_browser_throttle,
> safe_browsing_renderer_throttle. Right?

No, there isn't.

The two classes are used in browser/renderer to communicate with
SafeBrowsingUrlCheckerImpl. The former does that directly; the latter uses a
Mojo interface to do that.

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1187: "safe_browsing/safe_browsing_browser_throttle.cc",
On 2017/06/15 21:52:58, vakh (Varun Khaneja) wrote:
> Could you please create a target for these files in
> chrome/browser/safe_browsing/BUILD.gn and add those targets as DEPS here?
> 
> I know you are following precedent, but the sooner we start doing it
correctly,
> the better.

I feel that the current CL is already big and this is a relatively orthogonal
change. Do you mind if I put it in a separate CL?
I gave it a try but it moved quite a lot of lines and also I met some build
issues that I need to figure out.

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.h:385:
scoped_refptr<safe_browsing::SafeBrowsingService> safe_browsing_;
On 2017/06/15 21:52:58, vakh (Varun Khaneja) wrote:
> I'd prefer calling this safe_browsing_service_ or sb_service_

Done.

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_browser_throttle.cc (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_browser_throttle.cc:24:
during_destruction_ = true;
On 2017/06/15 21:52:58, vakh (Varun Khaneja) wrote:
> What's the use of setting this flag in the ~?
> Once the object is destructed, this flag shouldn't be read anyway so its vale
> shouldn't matter.

Because destruction of the SafeBrowsingUrlCheckerImpl this object owns will call
SafeBrowsingBrowserThrottle::OnCheckUrlResult(). In this case, we want
OnCheckUrlResult to be a no-op. Otherwise it will try to make virtual calls with
|delegate_| and crash.

I have made changes to SafeBrowsingUrlCheckerImpl's destruction behavior and
removed this boolean.

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_browser_throttle.h (right):

https://codereview.chromium.org/2924723002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_browser_throttle.h:5: #ifndef
CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_BROWSER_THROTTLE_H_
On 2017/06/15 21:52:59, vakh (Varun Khaneja) wrote:
> Since this file is already under safe_browsing directory, do you mind
> calling it browser_throttle.h ?

Done. I changed it to browser_url_loader_throttle.

[yzshen1, 2017-06-16 21:31:08]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2017-06-16 21:31:08]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
jam@chromium.org, vakh@chromium.org
Link to the patchset: https://codereview.chromium.org/2924723002/#ps160001
(title: ".")

[commit-bot: I haz the power, 2017-06-16 21:31: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, 2017-06-16 23:12:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497648668000360,
"parent_rev": "958a49a3e9f2f2fe39e5059f73f660e89de8fba3", "commit_rev":
"efcb7c795d6d84d1ab17e17d51efef624d79ce81"}

[commit-bot: I haz the power, 2017-06-16 23:12:46]

Description was changed from

==========
Network service: SafeBrowsing check for frame-resources from browser.

This CL:
- adds safe_browsing::BrowserURLLoaderThrottle (which is a URLLoaderThrottle
subclass) to throttle browser-side URL loading.

- adds ContentBrowserClient::CreateURLLoaderThrottles() and implements it in
ChromeContentBrowserClient to return a safe_browsing::BrowserURLLoaderThrottle
instance.

- changes NavigationURLLoaderNetworkService to use ThrottlingURLLoader on the IO
thread. It no longer owns URLLoaderPtr or Binding<URLLoaderClient> on the UI
thread. That means I had to proxy calls of these two interfaces between the IO
and UI thread. But that is necessary because ThrottlingURLLoader needs to apply
throttles living on the IO thread.

- changes ThrottlingURLLoader to also support StartLoaderCallback, which we use
in place of URLLoaderFactory in the browser.

- uses PossiblyAssociatedInterfacePtr<URLLoader> and gets rid of
AssociatedURLLoaderWrapper. It is less code and more efficient. (Eventually we
will use non-associated URLLoaderPtr consistently.)

BUG=715673
==========

to

==========
Network service: SafeBrowsing check for frame-resources from browser.

This CL:
- adds safe_browsing::BrowserURLLoaderThrottle (which is a URLLoaderThrottle
subclass) to throttle browser-side URL loading.

- adds ContentBrowserClient::CreateURLLoaderThrottles() and implements it in
ChromeContentBrowserClient to return a safe_browsing::BrowserURLLoaderThrottle
instance.

- changes NavigationURLLoaderNetworkService to use ThrottlingURLLoader on the IO
thread. It no longer owns URLLoaderPtr or Binding<URLLoaderClient> on the UI
thread. That means I had to proxy calls of these two interfaces between the IO
and UI thread. But that is necessary because ThrottlingURLLoader needs to apply
throttles living on the IO thread.

- changes ThrottlingURLLoader to also support StartLoaderCallback, which we use
in place of URLLoaderFactory in the browser.

- uses PossiblyAssociatedInterfacePtr<URLLoader> and gets rid of
AssociatedURLLoaderWrapper. It is less code and more efficient. (Eventually we
will use non-associated URLLoaderPtr consistently.)

BUG=715673

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

[commit-bot: I haz the power, 2017-06-16 23:12:47]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/efcb7c795d6d84d1ab17e17d51ef...