[ios][apcv2] Graft xorigin frames [chromium/src : main]
Vincent Boisselle (Gerrit)
Vincent Boisselle voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 34 comments![]( "Open in Gerrit")
thanks so so much Vince! this is looking really great, and very sorry for the delay in reviewing
Note: I quickly glossed over the unittest diff because it is quite hard to review
[ios][apcv2] Graft xorigin framesCan we add some comprehensive information to this commit message?
// Maps from local to remote token.question: do you think it's worth it documentation-wise to copy the information above about remote and local frames? or should it move to something more generic that encompasses these two methods?
std::optional<RemoteFrameToken> ChildFrameRegistrar::LookupLocalFrame(I defer to you and Tommy as local owners here, but do you think it's worthwhile to maintain a reverse lookup map instead of iterating over all the tokens at every lookup here? WDYT? if we do this operation often in the page context extraction, we may be introducing some latency here
at the very least I think we should document this in the header!
// grafting. Content is cached if there is no claim to fulfil immediately.type: `fulfill`
// Claims a frame's content for grafting. Puts the `placeholder` in a waitlistthe claim / give terminology is quite confusing to me throughout this file. do you think it's clear enough for future readers? WDYT instead of something like `RegisterPlaceholder`, `FulfillPlaceholder` and `FulfillUnresolvedPlaceholders`?
// Grafter that receives claims on frame content and frame content to construct
// the frame tree.do you mind adding a comment that explains the overview of the algorithm? (especially given the terminology used below in methods/comments). maybe we can also link to the DD tree grafting section?
#import "components/autofill/core/common/unique_ids.h"nit: can we add a TODO to do the migration off of `components/autofill`?
what do you think about making the tree grafter more self-contained, and reusable? ideally this would work for any node grafting impl, and token/ID types. for the types, we can use a template, and we pass callbacks to the grafter which take a node as param (similar to your `PlaceUnclaimedFrames` impl).
something like (code example created through prompting - definitely needs rework, but just to illustrate what I mean): https://paste.googleplex.com/5323044430151680
// Can't claim content for the `token` more than once.this shouldn't ever happen right? assuming no single frame should get an extraction pass more than once, this shouldn't be called twice. can we log a metric here (or add a TODO for that)
// Can't give give frame content for the `token` more than once.ditto
for (auto& [_, o] : unclaimed_frame_content_) {can we add a TODO here to log how often this happens? a overall TODO for metrics in this file would be helpful too I think
bool include_xorigin_frames,I'm not sure the name for this parameter is totally accurate, since this frame can't see x-origin frames. instead maybe something like `include_cross_origin_frame_tokens`? can we add a small blurb about it in the comment above?
should this file include new tests for `include_xorigin_frames`?
// Deserializes a string frame ID into a LocalFrameToken.IIUC this to go from JS frame ID to base::UnguessableToken to autofill::LocalFrameToken ? there isn't a more direct way of achieving this? and if not, can we document that in the comment?
#import "components/autofill/core/common/unique_ids.h"nit: add todo to cleanup dep on `components/autofill`
#import "components/autofill/ios/browser/autofill_util.h"ditto
// children of the main frame node since they couldn't be grafted atsuper nit: `by` instead of `at`?
frameId:do you think `localFrameId` would be an improvement to make this clearer?
valueAsDict.FindBool(kRemoteFrameTokenKey)) {I don't think this should be `bool`
if (frameId) {if there is no frameId, we would lose this frame. could we handle this more gracefully?
// for any of the main WebFrame's children iframes. Children can be remote
// or same origin frames.mismatch between terminologies (remote/local, same/cross origin)
// or same origin frames.super nit: extra space
if (const std::string* token_string =
value.FindString(kRemoteFrameTokenKey)) {can we extract the assignment from the `if` for legibility? and early return instead of nesting `if`s
if (std::optional<autofill::RemoteFrameToken> remote =
DeserializeFrameIdAsRemoteFrameToken(*token_string)) {ditto
if (std::optional<autofill::LocalFrameToken> local =
[self frameRegistrar]->LookupChildFrame(*remote)) {ditto
// True to graft cross-origin frames.
bool graft_xorigin_frames() const;nit: could you elaborate on the comment here
#import "components/autofill/core/common/unique_ids.h"
#import "components/autofill/ios/form_util/child_frame_registrar.h"TODO for removing the dep
constexpr base::TimeDelta kRegistrationTimeout = base::Seconds(5);this feels long, can it be reduced without introducing flakiness?
u"__gCrWeb.getRegisteredApi('pageContextWrapperTest')"
".getFunction('registerAllRemoteFrames')()");can we make this a const since I think I see it used multiple times in this file. even as a const, is this maintainable in case the original lib changes?
// Data extracted on remote frames (other than the main frame) on the same
// or different origin.
type RemoteFrameData = SameOriginFrameData|CrossOriginFrameData;nit: can you move this below the definitions of SameOriginFrameData and CrossOriginFrameData?
if (!contentDoc) {is this the most idiomatic way of knowing that this is a cross-origin frame?
keepXoriginFrames: boolean): ExtractionResult {similar to above, can we have a naming closer to including cross origin frame tokens?
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 7 comments![]( "Open in Gerrit")
for (auto [r, l] : lookup_map_) {AI suggestion: use `const auto& [r, l]` to remove unnecessary copying
std::map<autofill::LocalFrameToken, optimization_guide::proto::ContentNode>
unclaimed_frame_content_;
// Frame content that is claimed but wasn't yet available.
std::map<autofill::LocalFrameToken, optimization_guide::proto::ContentNode*>
claims_;can the content nodes here be unique pointers, for better lifecycle destruction of any remaining pointers and explicit ownership?
do we want to post a task on the main thread here just to give it the change to maybe finish some extractions or registrations?
for (auto& [ot, _] : unclaimed_frame_content_) {
for (auto& [at, _] : claims_) {
if (ot == at) {
return true;
}
}
}instead of nested loops, we can probably just iterate over one map and check for existence in the other, this would be more efficient
FrameGrafter _grafter;can we make this a unique pointer, for its lifecycle to be tied to the page context wrapper?
// TODO(crbug.com/460823916): The grafter should re-attempt claim uponthis is an important TODO, I fear there will be quite some unfinished registrations
return null;would it make sense to return a `CrossOriginFrameData` with a remote frame registration in this catch, if `keepXoriginFrames` is true?
Vincent Boisselle (Gerrit)
Vincent Boisselle added 41 comments![]( "Open in Gerrit")
I still need to change the license years
Can we add some comprehensive information to this commit message?
Done
question: do you think it's worth it documentation-wise to copy the information above about remote and local frames? or should it move to something more generic that encompasses these two methods?
Done
std::optional<RemoteFrameToken> ChildFrameRegistrar::LookupLocalFrame(I defer to you and Tommy as local owners here, but do you think it's worthwhile to maintain a reverse lookup map instead of iterating over all the tokens at every lookup here? WDYT? if we do this operation often in the page context extraction, we may be introducing some latency here
at the very least I think we should document this in the header!
I changed the logic to not have to do that
AI suggestion: use `const auto& [r, l]` to remove unnecessary copying
those are relatively small structures
std::map<autofill::LocalFrameToken, optimization_guide::proto::ContentNode>
unclaimed_frame_content_;
// Frame content that is claimed but wasn't yet available.
std::map<autofill::LocalFrameToken, optimization_guide::proto::ContentNode*>
claims_;can the content nodes here be unique pointers, for better lifecycle destruction of any remaining pointers and explicit ownership?
unclaimed_frame_content_ has its own optimization_guide::proto::ContentNode objects, which is moved there, then moved back to the placeholder when the fulfilling the claim.
For the claims_ since we populate empty placeholders in the apc tree, we need a pointer to it. The grafter itself doesn't own the tree.
Or we could use callbacks instead, not sure it would be better
OR we make some sort of weakptr wrapper around the content pointers
----
Regarding memory management
If the wrapper can only be used once, should be fine
It looks like the wrapper was mad to be only used once
// grafting. Content is cached if there is no claim to fulfil immediately.Vincent Boisselletype: `fulfill`
Done
// Claims a frame's content for grafting. Puts the `placeholder` in a waitlistthe claim / give terminology is quite confusing to me throughout this file. do you think it's clear enough for future readers? WDYT instead of something like `RegisterPlaceholder`, `FulfillPlaceholder` and `FulfillUnresolvedPlaceholders`?
sure
// Grafter that receives claims on frame content and frame content to construct
// the frame tree.do you mind adding a comment that explains the overview of the algorithm? (especially given the terminology used below in methods/comments). maybe we can also link to the DD tree grafting section?
Done
#import "components/autofill/core/common/unique_ids.h"nit: can we add a TODO to do the migration off of `components/autofill`?
Done
what do you think about making the tree grafter more self-contained, and reusable? ideally this would work for any node grafting impl, and token/ID types. for the types, we can use a template, and we pass callbacks to the grafter which take a node as param (similar to your `PlaceUnclaimedFrames` impl).
something like (code example created through prompting - definitely needs rework, but just to illustrate what I mean): https://paste.googleplex.com/5323044430151680
I don't see any use case other than frames.
I would keep it specialized for now,
if we need that elsewhere we can then generalize
// Can't claim content for the `token` more than once.this shouldn't ever happen right? assuming no single frame should get an extraction pass more than once, this shouldn't be called twice. can we log a metric here (or add a TODO for that)
Nothing is impossible but will likely not happen.
// Can't give give frame content for the `token` more than once.Vincent Boisselleditto
Done
do we want to post a task on the main thread here just to give it the change to maybe finish some extractions or registrations?
I reworked the logic to wait on registration
can we add a TODO here to log how often this happens? a overall TODO for metrics in this file would be helpful too I think
Done
for (auto& [ot, _] : unclaimed_frame_content_) {
for (auto& [at, _] : claims_) {
if (ot == at) {
return true;
}
}
}instead of nested loops, we can probably just iterate over one map and check for existence in the other, this would be more efficient
ah yeah, there was time it wasn't possible but now that I use the local frame token in both we can just do that.
I'm not sure the name for this parameter is totally accurate, since this frame can't see x-origin frames. instead maybe something like `include_cross_origin_frame_tokens`? can we add a small blurb about it in the comment above?
`include_cross_origin_frame_content`
this goes beyond the tokens where the content is grafted to the apc tree
should this file include new tests for `include_xorigin_frames`?
Done
// Deserializes a string frame ID into a LocalFrameToken.IIUC this to go from JS frame ID to base::UnguessableToken to autofill::LocalFrameToken ? there isn't a more direct way of achieving this? and if not, can we document that in the comment?
there is no other way to go from a string to base::UnguessableToken
what extra documentation do we need ?
#import "components/autofill/core/common/unique_ids.h"nit: add todo to cleanup dep on `components/autofill`
Done
#import "components/autofill/ios/browser/autofill_util.h"Vincent Boisselleditto
Done
adding the todo to the header will be sufficient
can we make this a unique pointer, for its lifecycle to be tied to the page context wrapper?
its lifecycle is already ties to the wrapper
I will be deleted first , before other _pageContext so there is less risk of dangling content pointers.
// children of the main frame node since they couldn't be grafted atsuper nit: `by` instead of `at`?
Done
do you think `localFrameId` would be an improvement to make this clearer?
Done
I don't think this should be `bool`
I guess no tests hit that case lol
A string indeed
if (frameId) {if there is no frameId, we would lose this frame. could we handle this more gracefully?
One thing we could do is to put that in the list of unregistered content then autoplace the content at the end of extraction
but note that this should be rare as registration isn't needed to give frame content.
// or same origin frames.Vincent Boissellesuper nit: extra space
Done
// for any of the main WebFrame's children iframes. Children can be remote
// or same origin frames.mismatch between terminologies (remote/local, same/cross origin)
Done
if (const std::string* token_string =
value.FindString(kRemoteFrameTokenKey)) {can we extract the assignment from the `if` for legibility? and early return instead of nesting `if`s
we dont want to early return if there is remote frame token
I managed to remove one level of nesting.
// TODO(crbug.com/460823916): The grafter should re-attempt claim uponthis is an important TODO, I fear there will be quite some unfinished registrations
yup, but the solution isn't that trivial so I prefer having it as a next step
if (std::optional<autofill::RemoteFrameToken> remote =
DeserializeFrameIdAsRemoteFrameToken(*token_string)) {Vincent Boisselleditto
Done
if (std::optional<autofill::LocalFrameToken> local =
[self frameRegistrar]->LookupChildFrame(*remote)) {Vincent Boisselleditto
Done
return;Vincent Boisselleis this return useful?
the finest return
// True to graft cross-origin frames.
bool graft_xorigin_frames() const;nit: could you elaborate on the comment here
Done
#import "components/autofill/core/common/unique_ids.h"
#import "components/autofill/ios/form_util/child_frame_registrar.h"TODO for removing the dep
If we change the tested file, we will have to change this file, so I think one comment per module is sufficient.
constexpr base::TimeDelta kRegistrationTimeout = base::Seconds(5);this feels long, can it be reduced without introducing flakiness?
Done
u"__gCrWeb.getRegisteredApi('pageContextWrapperTest')"
".getFunction('registerAllRemoteFrames')()");can we make this a const since I think I see it used multiple times in this file. even as a const, is this maintainable in case the original lib changes?
Done
// Data extracted on remote frames (other than the main frame) on the same
// or different origin.
type RemoteFrameData = SameOriginFrameData|CrossOriginFrameData;nit: can you move this below the definitions of SameOriginFrameData and CrossOriginFrameData?
Done
is this the most idiomatic way of knowing that this is a cross-origin frame?
I think so
and also probably the only way
return null;would it make sense to return a `CrossOriginFrameData` with a remote frame registration in this catch, if `keepXoriginFrames` is true?
I did a few tests and I have never seen cases where an exception is thrown if contentDocument can't be access. Have you seen that ?
There is a console log error message but no exception is thrown
I think we could remove this try/catch altogether
----
I reworked the logic to return on both an exception and a null content node
similar to above, can we have a naming closer to including cross origin frame tokens?
idk, I could use `keepCrossOriginFrameData` but I would not refer to token which is a bit too narrow IMO
Related details
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 12 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
12 comments
waiting for registration at the end of the tree walking as an approach to fix the inter-frame messaging wait delay sgtm! thanks Vince!
// not match a placeholder. Call this function when it is determined that (1)nit: you can remote "not" I think given you have "doesn't"
std::map<autofill::LocalFrameToken, optimization_guide::proto::ContentNode>
unclaimed_frame_content_;
// Frame content that is claimed but wasn't yet available.
std::map<autofill::LocalFrameToken, optimization_guide::proto::ContentNode*>
claims_;Vincent Boissellecan the content nodes here be unique pointers, for better lifecycle destruction of any remaining pointers and explicit ownership?
unclaimed_frame_content_ has its own optimization_guide::proto::ContentNode objects, which is moved there, then moved back to the placeholder when the fulfilling the claim.
For the claims_ since we populate empty placeholders in the apc tree, we need a pointer to it. The grafter itself doesn't own the tree.
Or we could use callbacks instead, not sure it would be better
OR we make some sort of weakptr wrapper around the content pointers
----
Regarding memory managementIf the wrapper can only be used once, should be fine
It looks like the wrapper was mad to be only used once
Acknowledged
// Deserializes a string frame ID into a LocalFrameToken.Vincent BoisselleIIUC this to go from JS frame ID to base::UnguessableToken to autofill::LocalFrameToken ? there isn't a more direct way of achieving this? and if not, can we document that in the comment?
there is no other way to go from a string to base::UnguessableToken
what extra documentation do we need ?
Acknowledged
std::optional<autofill::LocalFrameToken> DeserializeFrameIdAsLocalFrameToken(AI suggestion:
You are attempting to static_cast a std::optional<base::UnguessableToken> (returned by DeserializeJavaScriptFrameId) to std::optional<autofill::LocalFrameToken>. static_cast cannot convert between distinct template instantiations of std::optional even if the underlying types are convertible or aliases. Suggestion: Unwrap the optional and construct the token explicitly.
```
auto token = autofill::DeserializeJavaScriptFrameId(serialized_id);
return token ? std::make_optional(autofill::LocalFrameToken(*token)) : std::nullopt;
```
- (void)stopRegistrationTimer {nit: comment
- (void)completeRegistrationWaitWithBarrier:(base::RepeatingClosure)barrier {nit: comment
if (frameId) {Vincent Boisselleif there is no frameId, we would lose this frame. could we handle this more gracefully?
One thing we could do is to put that in the list of unregistered content then autoplace the content at the end of extraction
but note that this should be rare as registration isn't needed to give frame content.
I think the former sgtm
_registrationTimer.Start(FROM_HERE, kRegistrationTimeout, run_barrier_once);As we spoke offline: Let's add a TODO for a metric to see how often the registration timeout occurs
return;Vincent Boisselleis this return useful?
the finest return
kept by mistake?
_discoveredRemoteTokens.push_back(*remote);can we use the grafter as a source of truth instead of maintaining both the grafter and _discoveredRemoteTokens?
return null;Vincent Boissellewould it make sense to return a `CrossOriginFrameData` with a remote frame registration in this catch, if `keepXoriginFrames` is true?
I did a few tests and I have never seen cases where an exception is thrown if contentDocument can't be access. Have you seen that ?
There is a console log error message but no exception is thrown
I think we could remove this try/catch altogether
----
I reworked the logic to return on both an exception and a null content node
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 7 comments![]( "Open in Gerrit")
// not match a placeholder. Call this function when it is determined that (1)nit: you can remote "not" I think given you have "doesn't"
Done
std::optional<autofill::LocalFrameToken> DeserializeFrameIdAsLocalFrameToken(AI suggestion:
You are attempting to static_cast a std::optional<base::UnguessableToken> (returned by DeserializeJavaScriptFrameId) to std::optional<autofill::LocalFrameToken>. static_cast cannot convert between distinct template instantiations of std::optional even if the underlying types are convertible or aliases. Suggestion: Unwrap the optional and construct the token explicitly.
```
auto token = autofill::DeserializeJavaScriptFrameId(serialized_id);
return token ? std::make_optional(autofill::LocalFrameToken(*token)) : std::nullopt;
```
Done
- (void)stopRegistrationTimer {Vincent Boissellenit: comment
Done
- (void)completeRegistrationWaitWithBarrier:(base::RepeatingClosure)barrier {Vincent Boissellenit: comment
Done
_registrationTimer.Start(FROM_HERE, kRegistrationTimeout, run_barrier_once);As we spoke offline: Let's add a TODO for a metric to see how often the registration timeout occurs
Done
return;Vincent Boisselleis this return useful?
Nicolas MacBeththe finest return
kept by mistake?
the finest
_discoveredRemoteTokens.push_back(*remote);Vincent Boissellecan we use the grafter as a source of truth instead of maintaining both the grafter and _discoveredRemoteTokens?
Done
Related details
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted Code-Review+1![]( "Open in Gerrit")
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Commit-Queue | +1 |
2 comments
Doesn't solve the registration racing problem yet.Can you update the commit message, thanks!
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Message from Vincent Boisselle![]( "Open in Gerrit")
- Nicolas MacBeth
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vincent Boisselle
| Commit-Queue | +1 |
1 comment
Can you update the commit message, thanks!
Done
Related details
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |