[Heavy Ad] Add Capability to Send Report to the Embedding Frame [chromium/src : main]
Alex Moshchuk (Gerrit)
Alex Moshchuk added 4 comments![]( "Open in Gerrit")
if (parent && (&render_frame_host->GetPage() == &parent->GetPage()) &&Can these actually ever be different, given that parent is obtained via `GetParent()`? Fenced frames wouldn't do it, since GetParent() doesn't cross the fenced boundary, unless you meant to use GetParentOrOuterDocument - is this expected to work for a fenced frame? (I'd expect no, since that'd leak information.)
parent->IsRenderFrameLive()) {If a parent isn't live, it can't have any children (see [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=4193;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18)), so I'm also not sure about this check.
DCHECK_EQ(child_frame->GetParent(), this);Nit: latest guidance is to prefer CHECK_EQ
// additional info for the child frame before the report is sent.Can you mention the token could refer to either a frame or a proxy, in case the child frame is remote?
Related details
- Daniel Cheng
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 5 comments![]( "Open in Gerrit")
if (child_frame_token->Is<RemoteFrameToken>()) {
child_frame = blink::RemoteFrame::FromFrameToken(
child_frame_token->GetAs<RemoteFrameToken>());
} else {
child_frame = blink::LocalFrame::FromFrameToken(
child_frame_token->GetAs<LocalFrameToken>());
}I'm not sure why it's named differently, but you can use `Frame::ResolveFrame()` to replace these lines.
if (auto* frame_owner = child_frame->DeprecatedLocalOwner()) {I would have expected the intervention to be routed to the frame that owns the frame element for `child_frame` if `child_frame_token` is set. Is that not the case?
(Maybe the Mojo comments should be a bit clearer about which process gets the report)
builder.Append(frame_owner->ElementInfoForInterventionReport());This is still kind of inefficient. Do we really need the `url` or can we just use the `src` attribute from the frame element instead?
KURL url = GetDocument().CompleteURL(url_);
// Any URLs in the report should strip the username, password, and fragment.
// https://w3c.github.io/reporting/#capability-urls
String sanitized_url = url.StrippedForUseAsReferrer();
StringBuilder builder;
builder.Append("id=");
builder.Append(GetIdAttribute());
builder.Append(";url=");
builder.Append(sanitized_url);
return builder.ReleaseString();Probably just put this all in the intervention handler? No one else calls this anyway, and it's a bit less efficient this way.
return String();And we won't need this empty implementation either then (we would use `DynamicTo<HTMLFrameElementBase>` to get to the bits we needed)
Related details
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao added 8 comments![]( "Open in Gerrit")
if (parent && (&render_frame_host->GetPage() == &parent->GetPage()) &&Can these actually ever be different, given that parent is obtained via `GetParent()`? Fenced frames wouldn't do it, since GetParent() doesn't cross the fenced boundary, unless you meant to use GetParentOrOuterDocument - is this expected to work for a fenced frame? (I'd expect no, since that'd leak information.)
Ack. I had the same suspicion but wasn't sure about edge cases (FFs, Portals, etc.), so I played it safe. Given your second opinion, I've removed the condition.
If a parent isn't live, it can't have any children (see [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=4193;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18)), so I'm also not sure about this check.
Ack. Removed.
Nit: latest guidance is to prefer CHECK_EQ
Done
// additional info for the child frame before the report is sent.Can you mention the token could refer to either a frame or a proxy, in case the child frame is remote?
Done
if (child_frame_token->Is<RemoteFrameToken>()) {
child_frame = blink::RemoteFrame::FromFrameToken(
child_frame_token->GetAs<RemoteFrameToken>());
} else {
child_frame = blink::LocalFrame::FromFrameToken(
child_frame_token->GetAs<LocalFrameToken>());
}I'm not sure why it's named differently, but you can use `Frame::ResolveFrame()` to replace these lines.
Done
if (auto* frame_owner = child_frame->DeprecatedLocalOwner()) {I would have expected the intervention to be routed to the frame that owns the frame element for `child_frame` if `child_frame_token` is set. Is that not the case?
(Maybe the Mojo comments should be a bit clearer about which process gets the report)
It is indeed the case. (Not sure which part of the code seemed to contradict this though).
Anyway, I've updated the Mojo comments to make this more clear.
builder.Append(frame_owner->ElementInfoForInterventionReport());This is still kind of inefficient. Do we really need the `url` or can we just use the `src` attribute from the frame element instead?
In general, we have the flexibility in crafting the message. However, the [specification](https://w3c.github.io/reporting/#capability-urls) dictates that we must strip the URLs thoroughout the report. This sounds like we need to convert 'src' to URL first, so as to use the `StrippedForUseAsReferrer()` method (mirroring the [existing practice](https://source.chromium.org/chromium/chromium/src/+/main:net/reporting/reporting_service.cc?q=%22sanitized_url%20%3D%20url.GetAsReferrer%22) in the network stack for stripping the report's main URL).
KURL url = GetDocument().CompleteURL(url_);
// Any URLs in the report should strip the username, password, and fragment.
// https://w3c.github.io/reporting/#capability-urls
String sanitized_url = url.StrippedForUseAsReferrer();
StringBuilder builder;
builder.Append("id=");
builder.Append(GetIdAttribute());
builder.Append(";url=");
builder.Append(sanitized_url);
return builder.ReleaseString();Probably just put this all in the intervention handler? No one else calls this anyway, and it's a bit less efficient this way.
This now relies on private member `HTMLFrameElementBase::url_`, unless you think it's okay to expose `url_` publicly? (see also my other comment for why I think `url_` is needed).
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
if (auto* child_frame_owner = child_frame->DeprecatedLocalOwner()) {If this is always going to be sent to the parent process, the right thing is to call:
```
auto* child_frame_owner = CheckedTo<HTMLFrameOwnerElement>(child_frame->Owner());
```
Because the deprecated function is... well, deprecated
if (auto* frame_owner = child_frame->DeprecatedLocalOwner()) {Yao XiaoI would have expected the intervention to be routed to the frame that owns the frame element for `child_frame` if `child_frame_token` is set. Is that not the case?
(Maybe the Mojo comments should be a bit clearer about which process gets the report)
It is indeed the case. (Not sure which part of the code seemed to contradict this though).
Anyway, I've updated the Mojo comments to make this more clear.
The null check for the frame owner contradicts this.
builder.Append(frame_owner->ElementInfoForInterventionReport());Yao XiaoThis is still kind of inefficient. Do we really need the `url` or can we just use the `src` attribute from the frame element instead?
In general, we have the flexibility in crafting the message. However, the [specification](https://w3c.github.io/reporting/#capability-urls) dictates that we must strip the URLs thoroughout the report. This sounds like we need to convert 'src' to URL first, so as to use the `StrippedForUseAsReferrer()` method (mirroring the [existing practice](https://source.chromium.org/chromium/chromium/src/+/main:net/reporting/reporting_service.cc?q=%22sanitized_url%20%3D%20url.GetAsReferrer%22) in the network stack for stripping the report's main URL).
Sure, but we're not able to use `url_` literally anyway: we need to pass it to `CompleteURL()`. So we could just do that with the source attribute instead.
Related details
- Alex Moshchuk
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao added 4 comments![]( "Open in Gerrit")
if (auto* child_frame_owner = child_frame->DeprecatedLocalOwner()) {If this is always going to be sent to the parent process, the right thing is to call:
```
auto* child_frame_owner = CheckedTo<HTMLFrameOwnerElement>(child_frame->Owner());
```Because the deprecated function is... well, deprecated
Done
if (auto* frame_owner = child_frame->DeprecatedLocalOwner()) {Yao XiaoI would have expected the intervention to be routed to the frame that owns the frame element for `child_frame` if `child_frame_token` is set. Is that not the case?
(Maybe the Mojo comments should be a bit clearer about which process gets the report)
Daniel ChengIt is indeed the case. (Not sure which part of the code seemed to contradict this though).
Anyway, I've updated the Mojo comments to make this more clear.
The null check for the frame owner contradicts this.
I see. Agree it can only be a `HTMLFrameOwnerElement`. Fixed.
I've kept the null check as a safety precaution against a potential race condition where the FrameOwner could detach before receiving the Mojo message from the browser process. If you're confident that the FrameOwner must be non-null here, I can switch to a `DCHECK(child_frame_owner)`.
builder.Append(frame_owner->ElementInfoForInterventionReport());Yao XiaoThis is still kind of inefficient. Do we really need the `url` or can we just use the `src` attribute from the frame element instead?
Daniel ChengIn general, we have the flexibility in crafting the message. However, the [specification](https://w3c.github.io/reporting/#capability-urls) dictates that we must strip the URLs thoroughout the report. This sounds like we need to convert 'src' to URL first, so as to use the `StrippedForUseAsReferrer()` method (mirroring the [existing practice](https://source.chromium.org/chromium/chromium/src/+/main:net/reporting/reporting_service.cc?q=%22sanitized_url%20%3D%20url.GetAsReferrer%22) in the network stack for stripping the report's main URL).
Sure, but we're not able to use `url_` literally anyway: we need to pass it to `CompleteURL()`. So we could just do that with the source attribute instead.
Done
And we won't need this empty implementation either then (we would use `DynamicTo<HTMLFrameElementBase>` to get to the bits we needed)
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
LGTM for Blink and mojo with nit addressed
if (auto* frame_owner = child_frame->DeprecatedLocalOwner()) {Yao XiaoI would have expected the intervention to be routed to the frame that owns the frame element for `child_frame` if `child_frame_token` is set. Is that not the case?
(Maybe the Mojo comments should be a bit clearer about which process gets the report)
Daniel ChengIt is indeed the case. (Not sure which part of the code seemed to contradict this though).
Anyway, I've updated the Mojo comments to make this more clear.
Yao XiaoThe null check for the frame owner contradicts this.
I see. Agree it can only be a `HTMLFrameOwnerElement`. Fixed.
I've kept the null check as a safety precaution against a potential race condition where the FrameOwner could detach before receiving the Mojo message from the browser process. If you're confident that the FrameOwner must be non-null here, I can switch to a `DCHECK(child_frame_owner)`.
Yes it must be non-null here; if it is detached, it will not get Mojo messages.
Related details
- Alex Moshchuk
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao added 2 comments![]( "Open in Gerrit")
csharrison@: PTAL at heavy_ad_features.h/.cc. Thanks!
if (auto* frame_owner = child_frame->DeprecatedLocalOwner()) {Yao XiaoI would have expected the intervention to be routed to the frame that owns the frame element for `child_frame` if `child_frame_token` is set. Is that not the case?
(Maybe the Mojo comments should be a bit clearer about which process gets the report)
Daniel ChengIt is indeed the case. (Not sure which part of the code seemed to contradict this though).
Anyway, I've updated the Mojo comments to make this more clear.
Yao XiaoThe null check for the frame owner contradicts this.
Daniel ChengI see. Agree it can only be a `HTMLFrameOwnerElement`. Fixed.
I've kept the null check as a safety precaution against a potential race condition where the FrameOwner could detach before receiving the Mojo message from the browser process. If you're confident that the FrameOwner must be non-null here, I can switch to a `DCHECK(child_frame_owner)`.
Yes it must be non-null here; if it is detached, it will not get Mojo messages.
- Alex Moshchuk
- Charlie Harrison
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
- Charlie Harrison
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao added 1 comment![]( "Open in Gerrit")
KURL url = GetDocument().CompleteURL(url_);
// Any URLs in the report should strip the username, password, and fragment.
// https://w3c.github.io/reporting/#capability-urls
String sanitized_url = url.StrippedForUseAsReferrer();
StringBuilder builder;
builder.Append("id=");
builder.Append(GetIdAttribute());
builder.Append(";url=");
builder.Append(sanitized_url);
return builder.ReleaseString();Yao XiaoProbably just put this all in the intervention handler? No one else calls this anyway, and it's a bit less efficient this way.
This now relies on private member `HTMLFrameElementBase::url_`, unless you think it's okay to expose `url_` publicly? (see also my other comment for why I think `url_` is needed).
Resolved.
Related details
- Charlie Harrison
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |