Hi evem@, thanks for reporting this. Some questions for you:

 1. What's the consequence of not sending this IPC? Is this a big issue?
 2. At a high level, what is the requested DOM tree used for? DOM tree of the malicious page or the interstitial DOM? Is this used for SB extended reports?
 3. Could you link me to where chrome instantiates ThreatDomDetails? I only see this here [1], but mobile doesn't use FULL_SAFE_BROWSING [2]

Note to self: the code paths changed as part of b8b65b2d9322569cbc9ca66bd4b3177c94267980.

[1] https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_client.cc?q=%5CbThreatDOMDetails%5Cb+f:chrome&l=546&dr=C
[2] https://cs.chromium.org/chromium/src/build/config/BUILD.gn?q=FULL_SAFE_BROWSING&l=113&dr=C

Hi ntfschr@,

I'm not sure about the consequences of this problem/what the DOM tree is eventually used for so I've Cc'd jialiul@ to maybe answer some of the safe browsing specific questions :)

To answer 3, I came across this problem when doing a mojo conversion. I noticed that while the browser context in WebView instantiates a ThreatDetails, a ThreatDOMDetails is never instantiated on the renderer side meaning that when ThreatDetails sends IPCs to the renderer they never get delivered. So I think the problem is with what's happening in android_webview not chrome itself.

The link to the mojo conversion if it's of interest is: https://chromium-review.googlesource.com/c/chromium/src/+/899405

+threatDetail expert lpz@. 
lpz@, do you think we should revisit the decision to include threatdetail collection on Android too?
 https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_client.cc?q=%5CbThreatDOMDetails%5Cb+f:chrome&l=546&dr=C

To evem@ and ntfschr@'s question, these dom trees for extended reporting (a.k.a the checkbox on the interstitial). Not sending this IPC won't break anything, but clank/webview is not sending anything for extended reporting users either, thus safe browsing service loses some visibility to these blacklist sites. 

Adding safebrowsing component label, since it might be more than just a WebView issue. 

Ahh, this is interesting. It definitely sounds like it's worth revisiting if we want ThreatDetails on Android. We probably want interstitial reports from Clank at the very least, and it's also be great to make sure we can get ad sampling reports there too.

For WebView, we probably still want complete reports from interstitials (otherwise the checkbox on the interstitial isn't very useful). It's also be good to have some control over which triggers get enabled on WV though (eg: ad sampler doesn't need to be running).

It sounds like we need to decide about ThreatDetails for the Android platform. lpz@ would you be more appropriate to lead that decision?

> For WebView, we probably still want complete reports from interstitials

WebView and clank should both do SB extended reporting (do you mean something else by "interstitial reports"?). If that's not true anymore, that would be a surprise to me.

I'll schedule some time with ntfschr and jialiul to talk through this and then update here.

#if defined(FULL_SAFE_BROWSING)
  safe_browsing::ThreatDOMDetails::Create(render_frame, registry);
#endif

Little digging into the history of this line, it was an ancient decision as far as I can tell. This line was there in early 2015. (I vaguely remember that the Clank/ safe browsing integration happened in 2016).
So it is a bug haven't been noticed for ..er.. 2 years.... 

Brief summary of meeting:
- Confirmed that reports from clank do not contain the subframe data, so they're essentially incomplete.
- We agree that we should include ThreatDOMDetails on Android (both Clank and WebView).
- For Clank, we can change the #ifdef(FULL_SAFE_BROWSING) to be more relaxed (eg: "any" safe browsing) - might need some new build flags in chrome/renderer/chrome_content_renderer_client.cc
- for WebView, it has its own parallel file in android_webview/renderer/aw_content_renderer_client.cc, and we'll need to mimic that change here.
- Regarding performance impact, we don't plan to spend too much time worrying about it. Android does have the top-page reporting enabled, so we're just extending it to include subframes. And report generation is a relatively rare event, so not expecting much impact.

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

commit 109a829e7b5ecf10e474b4f30f504235d933d15d
Author: Luke Zielinski <lpz@chromium.org>
Date: Wed Mar 14 13:39:11 2018

Use ThreatDOMDetails on Android.

This ensures that SafeBrowsing reports from Android are complete.
Right now, those reports only contain data about the top-level page
but nothing from subframes. ThreatDOMDetails is responsible for
capturing data from the subframes.

Bug:  811567 
Change-Id: I540574b41e646b79805ef3d3166d49e386daf803
Reviewed-on: https://chromium-review.googlesource.com/959381
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Commit-Queue: Luke Z <lpz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543066}
[modify] https://crrev.com/109a829e7b5ecf10e474b4f30f504235d933d15d/chrome/renderer/chrome_content_renderer_client.cc