New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 783981 link

Starred by 2 users
Status: Fixed
Owner:
jam@chromium.org
Closed: Dec 2017
Cc:
vakh@chromium.org
nparker@chromium.org
█ jialiul@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
Proj-Servicification

Blocked on:
issue 783977
Blocking:
issue 598073



Sign in to add a comment

Remove ClientSideDetectionHost's usage of WebContentsObserver::DidGetResourceResponseStart and delete that callback

Project Member Reported by jam@chromium.org, Nov 10 2017 
This assumes that networking is running in the same process, which isn't the case with the network service.

For notifications about frame requests, WebContentsObserver::DidFinishNavigation is a substitute. r515725 did this for SafeBrowsingNavigationObserver.

ClientSideDetectionHost is the remaining user. It gets notifications for both frame request and subresources. For the latter, we would have to start plumbing this data from the renderer to the browser, to be compatible with the mode where network runs in a separate process.

Then we can remove WebContentsObserver::DidGetResourceResponseStart altogether.

Blocking this on  bug 783977  because we have no integration tests for ClientSideDetectionHost.

Comment 1 by jam@chromium.org, Nov 21 2017

Owner: jam@chromium.org
Status: Started (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 27 2017 

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

commit 444d4b0f6ac8d291948a016a0374fc1fdb593961
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Nov 27 18:50:41 2017

Convert ClientSideDetectionHost to work with the network service.

Instead of observing a ResourceDispatcherHostDelegate (which isn't used by the network service), monitor frame requests in the browser and subresource requests from the renderer.

A follow-up cl will remove the callbacks from ResourceDispatcherHostDelegate & WebContentsObserver.

Bug:  783981 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I0130bd5d9250314a36e3dabd7d2053fa16a45ddd
Reviewed-on: https://chromium-review.googlesource.com/784053
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519351}
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/browser/safe_browsing/client_side_detection_host.cc
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/browser/safe_browsing/client_side_detection_host.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/browser/safe_browsing/client_side_detection_host_browsertest.cc
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/common/render_messages.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/renderer/chrome_render_thread_observer.cc
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/renderer/extensions/extension_localization_peer.cc
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/chrome/renderer/extensions/extension_localization_peer.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/content/common/content_param_traits_macros.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/content/public/renderer/resource_dispatcher_delegate.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/content/renderer/loader/resource_dispatcher.h
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/content/renderer/loader/resource_dispatcher_unittest.cc
[modify] https://crrev.com/444d4b0f6ac8d291948a016a0374fc1fdb593961/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 29 2017 

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

commit e01bcb5c471f31277d081f010df12e5d7b09fc19
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Nov 29 21:14:13 2017

Add a histogram for how often a request which revokes a certificate exception is for a frame.

The motivation is that if it's a small % of the time, then monitoring frames only would be sufficient.

Bug:  783981 
Change-Id: Id90a871b1175eddc10597766488be286bc8b763d
Reviewed-on: https://chromium-review.googlesource.com/795355
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520243}
[modify] https://crrev.com/e01bcb5c471f31277d081f010df12e5d7b09fc19/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/e01bcb5c471f31277d081f010df12e5d7b09fc19/content/browser/ssl/ssl_manager.h
[modify] https://crrev.com/e01bcb5c471f31277d081f010df12e5d7b09fc19/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/e01bcb5c471f31277d081f010df12e5d7b09fc19/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/e01bcb5c471f31277d081f010df12e5d7b09fc19/tools/metrics/histograms/histograms.xml
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 4 2017 

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

commit d488264cd62f91e27147ac6f04cc3a466b92f789
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Dec 04 21:45:19 2017

Remove SSLManager dependency on WebContentsImpl::DidGetResourceResponseStart.

That callback is going away as it's not compatible with network service. Plumb information about subresource responses starting from the renderer to WebContents instead.

A followup will undo much of the code added in r519351 after this.

Bug:  783981 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Id04381daa669cb2ac05dcf0f5d7d890960c6c06b
Reviewed-on: https://chromium-review.googlesource.com/804947
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521469}
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/ssl/ssl_manager.h
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/common/frame.mojom
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/renderer/render_frame_impl.h
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/renderer/render_thread_impl.h
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/content/test/test_render_frame.cc
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/d488264cd62f91e27147ac6f04cc3a466b92f789/tools/metrics/histograms/histograms.xml
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4 2017 

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

commit 9b8f7fa46433815c7f947a57b951c60fd57d1184
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Dec 04 23:52:25 2017

Switch ClientSideDetectionHost to use new WebContentsObserver callback to learn about subresource responses.

 Issue 804947  introduces the new method because it has other uses, so no need for safe browsing code to send its own IPC. This change reverts most of r519351.

Bug:  783981 
Change-Id: Ifbd4866278d665c06bb6b4147161281470bf04cd
Reviewed-on: https://chromium-review.googlesource.com/805008
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521531}
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/chrome/browser/safe_browsing/client_side_detection_host.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/chrome/browser/safe_browsing/client_side_detection_host.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/chrome/common/render_messages.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/chrome/renderer/chrome_render_thread_observer.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/chrome/renderer/extensions/extension_localization_peer.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/chrome/renderer/extensions/extension_localization_peer.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/browser/loader/resource_dispatcher_host_unittest.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/common/content_param_traits_macros.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/public/common/common_param_traits_macros.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/public/renderer/resource_dispatcher_delegate.h
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/renderer/loader/resource_dispatcher.cc
[modify] https://crrev.com/9b8f7fa46433815c7f947a57b951c60fd57d1184/content/renderer/loader/resource_dispatcher_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 5 2017 

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

commit 4b29275111de86971c2046b7bd5ef3a7879b9829
Author: John Abd-El-Malek <jam@chromium.org>
Date: Tue Dec 05 02:14:22 2017

Small followup to r519351 to make ClientSideDetectionHost ignore same-document navigations.

Bug:  783981 
Change-Id: I41afc98d8c9d60051cde53f3d17c28515ec9a8aa
Reviewed-on: https://chromium-review.googlesource.com/807403
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521589}
[modify] https://crrev.com/4b29275111de86971c2046b7bd5ef3a7879b9829/chrome/browser/safe_browsing/client_side_detection_host.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 5 2017 

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

commit fde08e6daf69acc4143cc6f10cf7e61199f6a511
Author: John Abd-El-Malek <jam@chromium.org>
Date: Tue Dec 05 04:06:24 2017

Remove WebContentsObserver::DidGetResourceResponseStart since it's no longer used.

BUG= 783981 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I5fa5a48492d1bbded7fa5c43be2caf97626b659d
Reviewed-on: https://chromium-review.googlesource.com/791511
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521611}
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/chrome/browser/safe_browsing/client_side_detection_host.cc
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/chrome/browser/safe_browsing/client_side_detection_host.h
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/loader/loader_delegate.h
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/loader_delegate_impl.cc
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/loader_delegate_impl.h
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/content/public/browser/BUILD.gn
[delete] https://crrev.com/89af823eeee241570872af2c084fad53b74f6713/content/public/browser/resource_request_details.cc
[delete] https://crrev.com/89af823eeee241570872af2c084fad53b74f6713/content/public/browser/resource_request_details.h
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/fde08e6daf69acc4143cc6f10cf7e61199f6a511/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Comment 8 by jam@chromium.org, Dec 11 2017

Status: Fixed (was: Started)
►

Sign in to add a comment