Implement Back-To-Ad Intervention behind a feature flag [chromium/src : main]
Yao Xiao (Gerrit)
Yao Xiao added 17 comments![]( "Open in Gerrit")
Implement Back-Forward-To-Ad Intervention behind a feature flagYao XiaoNaming discussion, which applies throughout:
Maybe Back-To-Ad Intervention is clear enough, and the documentation can be clear that it applies to forward as well? The current name is a bit hard to parse and isn't as obvious that it's about ads.
Alternatively, "History Ad Intervention" could capture both cases and is a bit closer to the existing "History Manipulation Intervention."
Done. I switched to 'Back-To-Ad Intervention'.
intervention (`is_possibly_skippable_ad_entry`). *Exception*: If the
target entry is same-origin, the ad intervention check
(`is_possibly_skippable_ad_entry`) is ignored, and only standardYao XiaoI think it'll be important to document why this is done and why it doesn't cause problems (e.g., in the markdown file mentioned below, and in comments on the code doing it), because I'm having trouble remembering why it's ok.
Done. (I added the rationale in inline code comments and updated documentation in history_manipulation_intervention.md.)
Design Doc:Yao XiaoCan we update the markdown in docs/history_manipulation_intervention.md with a description of the new behavior? It's worth doing this now (with a mention of the flag for the time being), so that we can be sure there's a way to describe the new policy which is understandable by others.
We may be getting to the point that a diagram or flowchart may be required to understand the flow of this code, but maybe we can start with having that in the design doc as we figure out how the loop should behave (so that we can easily refine it before putting it into the markdown).
Done. I've restructured things a bit, e.g. moving the invariants to the 'Standard Intervention' section where they fit better.
// entries.Yao XiaoCan we elaborate which ad entries? It's not all of them (e.g., an ad landing page if you click on an ad), just those that are silently inserted into session history.
Done
std::optional<int> GetIndexWithSkipping(int from_index, Direction direction);Yao Xiao(Not for this CL) I'm curious if we still need the "ForGoBack" and "ForGoForward" helpers, or if the callers of those can just pass a Direction. If that makes sense (after the other questions are resolved), let's do it in a followup CL to avoid adding more changes to this CL.
Ack. Filed bug https://crbug.com/483673045.
int limit_check = (direction == Direction::kForward) ? GetEntryCount() : -1;Yao Xiaonit: I'm not sure we need this defined as a local. We could put the values directly into the lambda below without doing another `direction` conditional here. I think that would also make the comparison in the lambda easier to read.
Done
// Helper lambda to check bounds based on directionYao Xiaominor nit: Always end comments with a period.
Done
auto in_bounds = [&](int idx) {Yao Xiaonit: is_in_bounds
Done
Yao XiaoTo help explain the has_value() check on line 1282, it might be worth adding a comment here saying that having a nullopt `result_index` here means all of the entries are skippable due to the standard history manipulation intervention.
Actually, is it worth also returning early here in that case, so that the whole ad block below doesn't need to be nested within the has_value() conditional?
Done
// 2. Back-Forward-To-Ad Intervention Logic (Dry Run):Yao XiaoI'm not sure about the "Dry Run" part of this comment, since this block includes the actual behavior change (lines 1330-1334) as well.
Done. Removed the "Dry Run" part.
for (int index = result_index.value(); in_bounds(index); index += step) {Yao XiaoJust to be clear that this is intentional, let's note in a comment that we're starting from where the above intervention left off, and skipping further entries-- not just ones due to the history ad intervention, but either intervention.
I haven't reviewed the tests yet, but is there one covering the case where we skip a combined block of entries?
Done. I've updated the comment under "2. Back-Forward-To-Ad Intervention Logic" to explicitly state this.
Regarding tests: Yes, the test cases `StandardSkippableEntriesFollowedByAdEntries` and `AdEntriesFollowedByStandardSkippableEntries` specifically cover the scenario where we skip a combined block containing both types of entries.
NavigationEntryImpl* start_entry = GetEntryAtIndex(from_index);Yao Xiao(nit: current_entry would be closer to the terminology we usually use.)
However, looking closer, I'm not sure what entry we're actually getting here. `from_index` may not be the current index, and could be one of the iterative steps in GetIndexForOffsetWithSkipping from https://chromium-review.googlesource.com/c/chromium/src/+/7262337.
Do we actually want to be doing the cross-origin check on those iterative entries?
This is also relevant for metrics, even with the check on line 1308. I left a note about that case below.
Regarding the name: I used 'start_entry' exactly because 'from_index' may not be the current index.
> Do we actually want to be doing the cross-origin check on those iterative entries?
For skipping, I think so. Conceptually, it makes sense to handle GetIndexForOffset as iterative steps of Back/Forward. Now that we have a logic for the basic "go back one step" case, I prefer to maintain consistency here.
For metrics, I'm only planning to record them on the first iteration step. I've added more details in the comment below.
url::Origin target_origin = url::Origin::Create(target_entry->GetURL());Yao XiaoThis is unfortunately tricky, for several reasons:
- We can't safely convert a URL to an Origin, per [Origin::Create](https://source.chromium.org/chromium/chromium/src/+/main:url/origin.h;drc=8393737e4619e84795f4c565912e1b044fef2a82;l=139-165). For example, two different about:blank documents could have different inherited origins.
- An alternative would be using the current RFH's last committed origin and the FrameNavigationEntry's committed_origin(), but the latter isn't always defined-- it only gets set after we've visited that particular FrameNavigationEntry, and won't be set for new navigations (or restored ones, I think).
- The question above about iterative calls to this function make it less clear whether we want to use the current RFH as a comparison point or not.
Thanks for the note. I can see that `Origin::Create` is lossy.
However, in the context of this intervention, a 'false positive' for cross-origin status (treating about:blank as cross-origin) results in a safe failure mode: we simply proceed with the skipping logic. As noted in the rationale for the 'same-origin exception,' this check is a heuristic to limit the scope of the intervention rather than a strict security boundary.
Given that the risk is low and actually favors the user (by skipping potential junk entries), I believe Origin::Create is sufficient here? PLMK if you have a different approach in mind.
if (from_index == GetCurrentEntryIndex()) {Yao XiaoPer the question above, I'm unsure if this will work as expected. Even if we limit the metrics reporting to one of the iterative calls (i.e., when it starts from the current index), will that be the right value to report?
At first glance, it seems like a GetIndexForOffsetWithSkipping call could only skip standard intervention entries in the first iteration (and not report anything here), then skip ad related entries in the second iteration (which should have caused us to report metrics overall but wouldn't happen because from_index isn't the current index on that iteration).
Seems like we might want a test for that case, if we aren't catching this now.
(As a side note, I'm finding this quite complex and hard to reason about, which means it's hard to maintain and make changes. The more we can do to document and simplify this, the easier it will be to work in this code in the future.)
I believe the existing behavior for metrics is intentional and acceptable:
- If the multi-step navigation `GetIndexForOffsetWithSkipping` occurs where the first step is "clean" (standard intervention only) but the second step involves ad-skipping, we will indeed skip reporting the metric for that second step.
- For the logic within `GetIndexWithSkipping`, it doesn't matter if there are prior standard skippable entries. As long as ad entries would be skipped, we record a UKM and attribute it to the page *where the back navigation started*.
While we could attempt to attribute the skip to a specific ad entry, it adds complexity -- it is more convenient to record a UKM for the active page than for an arbitrary page from the session history. Overall, this reporting scope is sufficient to collect data for future analysis.
I have added more comments to clarify the reasoning behind the metric recording.
Regarding the test: I have an existing test (`AdEntriesFollowedByStandardSkippableEntries`) that covers the attribution logic for the single-step case. Since `GetIndexForOffsetWithSkipping` is Android-specific, and given that the code explicitly gates metrics on `from_index == GetCurrentEntryIndex()`, I'd prefer to rely on the current unit tests unless you feel strongly that the multi-step edge case needs a specific regression test.
RenderFrameHostImpl* rfh = frame_tree_->root()->current_frame_host();Yao XiaoNot all history navigations involve the main frame. If this is only for the UKM, we might want to make that clearer.
Is it ok to report the UKM if no main frame navigation is involved, though? Or do we have a way to guarantee that this does involve a main frame navigation, based on other invariants about the intervention?
In the context of back-to-ad intervention, we treat subframe entries as same-doc main frame entries. So we can/should still report a metric (URL attributed to the main frame page). The test case `AdReplaceStateFollowedByAdFramePushState` covers this scenario.
// Only execute the skip if it takes the user to a cross-origin site ANDYao XiaoI'm guessing you mean "cross-origin document" or "cross-origin page," but I think we also need to elaborate about what we're intending to compare against (e.g., current document vs where we would have ended up without the intervention, etc).
Done (Clarified).
// back/forward buttons.Yao XiaoLet's link to the bug with more context.
Done
Related details
- Charlie Reis
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. |
Charlie Reis (Gerrit)
Charlie Reis added 33 comments![]( "Open in Gerrit")
Thanks for the updates, and for the documentation! I'm still not sure about some of the points below, but it seems like we're making progress.
intervention (`is_possibly_skippable_ad_entry`). *Exception*: If the
target entry is same-origin, the ad intervention check
(`is_possibly_skippable_ad_entry`) is ignored, and only standardYao XiaoI think it'll be important to document why this is done and why it doesn't cause problems (e.g., in the markdown file mentioned below, and in comments on the code doing it), because I'm having trouble remembering why it's ok.
Done. (I added the rationale in inline code comments and updated documentation in history_manipulation_intervention.md.)
Thanks!
// entry as a candidate for skipping, as it lacks an ad-entry-creator signal.This comment is worth keeping even if we can't directly test it, perhaps above the ExecJs call.
// page where the back navigation started.Would it also have reported metrics for GoForwardWouldSkipAd, if the URL wasn't about:blank? That might be worth mentioning if we can't test for it.
EXPECT_EQ(GetWebContents()->GetLastCommittedURL(), GURL(url::kAboutBlankURL));Is it worth testing the metric here as well? (I'm unclear if the goal is to do that on every test with skipping or not, since it is done again around line 330 but not in the following test.)
IN_PROC_BROWSER_TEST_F(AdNavigationEntryBrowserTest,Charlie ReisNote to self: I still need to review the tests.
Done
std::optional<int> GetIndexForGoBack(bool performing_navigation);This new parameter is fairly awkward. If there isn't a way to do the metric reporting from the places performing the navigation, can we at least keep this more internal to the class without having it leak outside?
One option would be making this a default false parameter that no one has to provide, except for the internal GoBack/GoForward/GoToOffset functions.
Another would be having a private helper function that takes this parameter so that no callers outside NavigationControllerImpl would have to decide whether they need to pass it or not. I'm not sure if that's worth the extra code, if we can be clear in the documentation here that most callers should leave it as false.
NavigationEntryImpl* start_entry = GetEntryAtIndex(from_index);Yao Xiao(nit: current_entry would be closer to the terminology we usually use.)
However, looking closer, I'm not sure what entry we're actually getting here. `from_index` may not be the current index, and could be one of the iterative steps in GetIndexForOffsetWithSkipping from https://chromium-review.googlesource.com/c/chromium/src/+/7262337.
Do we actually want to be doing the cross-origin check on those iterative entries?
This is also relevant for metrics, even with the check on line 1308. I left a note about that case below.
Regarding the name: I used 'start_entry' exactly because 'from_index' may not be the current index.
> Do we actually want to be doing the cross-origin check on those iterative entries?For skipping, I think so. Conceptually, it makes sense to handle GetIndexForOffset as iterative steps of Back/Forward. Now that we have a logic for the basic "go back one step" case, I prefer to maintain consistency here.
For metrics, I'm only planning to record them on the first iteration step. I've added more details in the comment below.
I could agree with doing the origin check iteratively as you describe. It does seem important to document this (and call out why `start_entry` is not `current_entry`).
url::Origin target_origin = url::Origin::Create(target_entry->GetURL());Yao XiaoThis is unfortunately tricky, for several reasons:
- We can't safely convert a URL to an Origin, per [Origin::Create](https://source.chromium.org/chromium/chromium/src/+/main:url/origin.h;drc=8393737e4619e84795f4c565912e1b044fef2a82;l=139-165). For example, two different about:blank documents could have different inherited origins.
- An alternative would be using the current RFH's last committed origin and the FrameNavigationEntry's committed_origin(), but the latter isn't always defined-- it only gets set after we've visited that particular FrameNavigationEntry, and won't be set for new navigations (or restored ones, I think).
- The question above about iterative calls to this function make it less clear whether we want to use the current RFH as a comparison point or not.
Thanks for the note. I can see that `Origin::Create` is lossy.
However, in the context of this intervention, a 'false positive' for cross-origin status (treating about:blank as cross-origin) results in a safe failure mode: we simply proceed with the skipping logic. As noted in the rationale for the 'same-origin exception,' this check is a heuristic to limit the scope of the intervention rather than a strict security boundary.
Given that the risk is low and actually favors the user (by skipping potential junk entries), I believe Origin::Create is sufficient here? PLMK if you have a different approach in mind.
a 'false positive' for cross-origin status (treating about:blank as cross-origin) results in a safe failure mode
Actually, it's the opposite. Two cross-origin about:blank documents would get treated as same-origin using this approach, and thus the intervention would be bypassed.
I don't think it's super likely to be abused, but this is generally an unsafe pattern that we don't want people copying. I would suggest:
(1) If the main frame FrameNavigationEntry has a committed_origin(), let's use that. It will usually be present in cases like this, when we've already visited that entry in the current session.
(2) Fall back to Origin::Create with a comment about why it's ok not to be perfect here for the intervention.
if (from_index == GetCurrentEntryIndex()) {Yao XiaoPer the question above, I'm unsure if this will work as expected. Even if we limit the metrics reporting to one of the iterative calls (i.e., when it starts from the current index), will that be the right value to report?
At first glance, it seems like a GetIndexForOffsetWithSkipping call could only skip standard intervention entries in the first iteration (and not report anything here), then skip ad related entries in the second iteration (which should have caused us to report metrics overall but wouldn't happen because from_index isn't the current index on that iteration).
Seems like we might want a test for that case, if we aren't catching this now.
(As a side note, I'm finding this quite complex and hard to reason about, which means it's hard to maintain and make changes. The more we can do to document and simplify this, the easier it will be to work in this code in the future.)
I believe the existing behavior for metrics is intentional and acceptable:
- If the multi-step navigation `GetIndexForOffsetWithSkipping` occurs where the first step is "clean" (standard intervention only) but the second step involves ad-skipping, we will indeed skip reporting the metric for that second step.
- For the logic within `GetIndexWithSkipping`, it doesn't matter if there are prior standard skippable entries. As long as ad entries would be skipped, we record a UKM and attribute it to the page *where the back navigation started*.
While we could attempt to attribute the skip to a specific ad entry, it adds complexity -- it is more convenient to record a UKM for the active page than for an arbitrary page from the session history. Overall, this reporting scope is sufficient to collect data for future analysis.
I have added more comments to clarify the reasoning behind the metric recording.
Regarding the test: I have an existing test (`AdEntriesFollowedByStandardSkippableEntries`) that covers the attribution logic for the single-step case. Since `GetIndexForOffsetWithSkipping` is Android-specific, and given that the code explicitly gates metrics on `from_index == GetCurrentEntryIndex()`, I'd prefer to rely on the current unit tests unless you feel strongly that the multi-step edge case needs a specific regression test.
Sorry, I don't think I agree with this.
If the second iteration is affected by the back-to-ad intervention, I think we should be reporting that in metrics.
Your reasoning [above](https://chromium-review.googlesource.com/c/chromium/src/+/7533035/comment/fe790f01_1607397b/) ("Conceptually, it makes sense to handle GetIndexForOffset as iterative steps of Back/Forward") seems to imply the same thing. The second iteration would be like clicking back again, which would have reported the metrics.
RenderFrameHostImpl* rfh = frame_tree_->root()->current_frame_host();Yao XiaoNot all history navigations involve the main frame. If this is only for the UKM, we might want to make that clearer.
Is it ok to report the UKM if no main frame navigation is involved, though? Or do we have a way to guarantee that this does involve a main frame navigation, based on other invariants about the intervention?
In the context of back-to-ad intervention, we treat subframe entries as same-doc main frame entries. So we can/should still report a metric (URL attributed to the main frame page). The test case `AdReplaceStateFollowedByAdFramePushState` covers this scenario.
Assuming this is ok, we still need to make sure that other code doesn't mistake `rfh` as being involved in the navigation. Can we rename it to something like `main_frame_rfh_for_ukm` and/or include a comment about this?
}What happens if the rest of the entries are skippable, when they weren't before (like line 1288)?
It looks like we'll finish the loop above without updating `result_index_with_ad_skipping` and the back-to-ad intervention won't skip the ad entries.
If that's true, we should fix that and add a test for it.
NavigationEntryImpl* start_entry = GetEntryAtIndex(from_index);Please move the comment about the cross-origin exception here above the origin computations, to make it clear why this block is needed.
// collect data for future analysis.Does this mean a different site can be blamed for the ad? Or can we be clear in the metrics that this isn't about fault but about the number of times something like this occurs? (If so, I'm not sure why we need UKM rather than just UMA.)
### 1. Standard history manipulation interventionMaybe we should say "Original" (throughout) to avoid implying there's a standard or spec associated with this (given the note below about how that hasn't been done yet).
## InvariantsPlease do not remove these invariants. There is a lot of reasoning and example bugs described here that are important when considering changes to the intervention. It looks like the new draft has removed the bulk of this.
The back-to-ad intervention (gated by feature flag `BackToAdIntervention`) makesThanks for mentioning this. Let's move it to a second sentence where we can be a little clearer, like:
This is currently disabled by default and gated by the `BackToAdIntervention` feature flag.
(Out of curiosity, will you be adding a chrome://flags entry for this in a future CL? We can update this to point to that once that's done.)
into session history.Let's add: ", even if the page does have a user activation."
1. User is on `a.com`.Maybe add something like "and grants it a user activation."
3. If the user presses back, the first same-document page (`a1`) will bes/page/entry/
skipped, and the browser will go back to the page before `a.com` (e.g., aMaybe add: "because it was caused by an ad"?
skipped, and the browser will go back to the page before `a.com` (e.g., a
New Tab Page).script (target of `replaceState`) AND subsequently creates a new entry(e.g., ...)
Same on next line.
(initiator of `pushState`), it is an ad-tagged entry. Whenever the user tries to
navigate back to this entry, the ad script logic will run again to turn the page
into an ad, effectively showing an unseen or unwanted ad.I might suggest:
If the user navigates back to such an entry, the ad script logic has a chance to inject a new ad into the user's session history.
Maybe we can move the description of the origin restriction here?
Because this only impacts browser UI, this is allowed by the spec, which onlyTo preserve more of the old line 14, let's slightly rephrase this to:
"The intervention only impacts the browser back/forward buttons and not the `history.back()/forward()` APIs. As a result, this is..."
explanatory message.Will back-to-ad need an equivalent message at some point?
* When a `NavigationEntry` is active and the document creates anotherAn entry can contain multiple documents. Does this mean "one of its documents"?
* In the context of this tagging, a subframe entry is treated the same way asI'm wondering if there's a better way to phrase this. The tracking is done on NavigationEntry and not FrameNavigationEntry, so maybe it's clearer to say that a NavigationEntry's tagging is affected by both subframe and same-document navigations?
### 3. Intervention Logicnit: This seems like it should be a separate section, since it's not a third variant of the intervention.
either marked as `should_skip_on_back_forward_ui_` (Standard) or are annit: or as
One exception to this rule is that we only skip `is_possibly_skippable_ad_entry`
if it takes the user to a cross-origin page relative to the page where theI don't think this quite captures it. The entry doesn't take the user somewhere or affect the exception on its own. At least in the current implementation, it's the end result of the skipping decision (which involves multiple skippable entries, from both versions of the intervention) which might be ignored if the navigation would land on a main frame which is cross-origin to the current main frame.
That means [A1, B_ad, C_skip, D_ad, A2*] might decide not to skip D_ad because the outcome would have been A1, which is same origin with A2. I'm still not thrilled with this, but if we're going to keep it we should be clear what the behavior is.
currently maintain leniency regarding potential back-to-ad abuse when the usernit: are currently lenient
Related details
- Yao Xiao
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. |
Yao Xiao (Gerrit)
Yao Xiao added 30 comments![]( "Open in Gerrit")
// entry as a candidate for skipping, as it lacks an ad-entry-creator signal.This comment is worth keeping even if we can't directly test it, perhaps above the ExecJs call.
Done
Would it also have reported metrics for GoForwardWouldSkipAd, if the URL wasn't about:blank? That might be worth mentioning if we can't test for it.
Done. It was indeed because UKM doesn't record about:blank URLs, so I've added a comment explaining that. Additionally, I added the test CrossOriginNavigationFollowedByAdReplaceStateFollowedByAdPushState to explicitly show that kHistoryGoForwardWouldSkipAd triggers in that scenario.
EXPECT_EQ(GetWebContents()->GetLastCommittedURL(), GURL(url::kAboutBlankURL));Is it worth testing the metric here as well? (I'm unclear if the goal is to do that on every test with skipping or not, since it is done again around line 330 but not in the following test.)
My goal wasn't to test this on every case. The assertion at `AdReplaceStateFollowedByAdFramePushState` was specifically to verify subframe UKM logging and main-frame URL attribution. Standard main-frame same-doc navigations don't really need the same redundant coverage. I'm happy to add it for consistency if you think it's needed, though!
std::optional<int> GetIndexForGoBack(bool performing_navigation);This new parameter is fairly awkward. If there isn't a way to do the metric reporting from the places performing the navigation, can we at least keep this more internal to the class without having it leak outside?
One option would be making this a default false parameter that no one has to provide, except for the internal GoBack/GoForward/GoToOffset functions.
Another would be having a private helper function that takes this parameter so that no callers outside NavigationControllerImpl would have to decide whether they need to pass it or not. I'm not sure if that's worth the extra code, if we can be clear in the documentation here that most callers should leave it as false.
Done. I went with the first suggestion: I've added a default false value to the parameter in these two public methods and expanded the documentation to clarify usage. (I kept the parameter mandatory for private methods to ensure internal callers remain explicit about the behavior.)
NavigationEntryImpl* start_entry = GetEntryAtIndex(from_index);Yao Xiao(nit: current_entry would be closer to the terminology we usually use.)
However, looking closer, I'm not sure what entry we're actually getting here. `from_index` may not be the current index, and could be one of the iterative steps in GetIndexForOffsetWithSkipping from https://chromium-review.googlesource.com/c/chromium/src/+/7262337.
Do we actually want to be doing the cross-origin check on those iterative entries?
This is also relevant for metrics, even with the check on line 1308. I left a note about that case below.
Charlie ReisRegarding the name: I used 'start_entry' exactly because 'from_index' may not be the current index.
> Do we actually want to be doing the cross-origin check on those iterative entries?For skipping, I think so. Conceptually, it makes sense to handle GetIndexForOffset as iterative steps of Back/Forward. Now that we have a logic for the basic "go back one step" case, I prefer to maintain consistency here.
For metrics, I'm only planning to record them on the first iteration step. I've added more details in the comment below.
I could agree with doing the origin check iteratively as you describe. It does seem important to document this (and call out why `start_entry` is not `current_entry`).
Done
url::Origin target_origin = url::Origin::Create(target_entry->GetURL());Yao XiaoThis is unfortunately tricky, for several reasons:
- We can't safely convert a URL to an Origin, per [Origin::Create](https://source.chromium.org/chromium/chromium/src/+/main:url/origin.h;drc=8393737e4619e84795f4c565912e1b044fef2a82;l=139-165). For example, two different about:blank documents could have different inherited origins.
- An alternative would be using the current RFH's last committed origin and the FrameNavigationEntry's committed_origin(), but the latter isn't always defined-- it only gets set after we've visited that particular FrameNavigationEntry, and won't be set for new navigations (or restored ones, I think).
- The question above about iterative calls to this function make it less clear whether we want to use the current RFH as a comparison point or not.
Charlie ReisThanks for the note. I can see that `Origin::Create` is lossy.
However, in the context of this intervention, a 'false positive' for cross-origin status (treating about:blank as cross-origin) results in a safe failure mode: we simply proceed with the skipping logic. As noted in the rationale for the 'same-origin exception,' this check is a heuristic to limit the scope of the intervention rather than a strict security boundary.
Given that the risk is low and actually favors the user (by skipping potential junk entries), I believe Origin::Create is sufficient here? PLMK if you have a different approach in mind.
a 'false positive' for cross-origin status (treating about:blank as cross-origin) results in a safe failure mode
Actually, it's the opposite. Two cross-origin about:blank documents would get treated as same-origin using this approach, and thus the intervention would be bypassed.
I don't think it's super likely to be abused, but this is generally an unsafe pattern that we don't want people copying. I would suggest:
(1) If the main frame FrameNavigationEntry has a committed_origin(), let's use that. It will usually be present in cases like this, when we've already visited that entry in the current session.
(2) Fall back to Origin::Create with a comment about why it's ok not to be perfect here for the intervention.
Done. Added and switched to using a helper function.
if (from_index == GetCurrentEntryIndex()) {Yao XiaoPer the question above, I'm unsure if this will work as expected. Even if we limit the metrics reporting to one of the iterative calls (i.e., when it starts from the current index), will that be the right value to report?
At first glance, it seems like a GetIndexForOffsetWithSkipping call could only skip standard intervention entries in the first iteration (and not report anything here), then skip ad related entries in the second iteration (which should have caused us to report metrics overall but wouldn't happen because from_index isn't the current index on that iteration).
Seems like we might want a test for that case, if we aren't catching this now.
(As a side note, I'm finding this quite complex and hard to reason about, which means it's hard to maintain and make changes. The more we can do to document and simplify this, the easier it will be to work in this code in the future.)
Charlie ReisI believe the existing behavior for metrics is intentional and acceptable:
- If the multi-step navigation `GetIndexForOffsetWithSkipping` occurs where the first step is "clean" (standard intervention only) but the second step involves ad-skipping, we will indeed skip reporting the metric for that second step.
- For the logic within `GetIndexWithSkipping`, it doesn't matter if there are prior standard skippable entries. As long as ad entries would be skipped, we record a UKM and attribute it to the page *where the back navigation started*.
While we could attempt to attribute the skip to a specific ad entry, it adds complexity -- it is more convenient to record a UKM for the active page than for an arbitrary page from the session history. Overall, this reporting scope is sufficient to collect data for future analysis.
I have added more comments to clarify the reasoning behind the metric recording.
Regarding the test: I have an existing test (`AdEntriesFollowedByStandardSkippableEntries`) that covers the attribution logic for the single-step case. Since `GetIndexForOffsetWithSkipping` is Android-specific, and given that the code explicitly gates metrics on `from_index == GetCurrentEntryIndex()`, I'd prefer to rely on the current unit tests unless you feel strongly that the multi-step edge case needs a specific regression test.
Sorry, I don't think I agree with this.
If the second iteration is affected by the back-to-ad intervention, I think we should be reporting that in metrics.
Your reasoning [above](https://chromium-review.googlesource.com/c/chromium/src/+/7533035/comment/fe790f01_1607397b/) ("Conceptually, it makes sense to handle GetIndexForOffset as iterative steps of Back/Forward") seems to imply the same thing. The second iteration would be like clicking back again, which would have reported the metrics.
I see your point, and conceptually I agree.
The challenge here is purely technical. Because we rely on `browser_client->LogWebFeatureForCurrentPage(rfh)`, we can't easily attribute metrics to a non-current page without building out a new API.
My rationale for the current scope is a cost-to-benefit trade-off. I'm assuming single-step navigations dominate user behavior. While we will intentionally under-report interventions that happen deep within a multi-step navigation, those will still be overwhelmingly caught by users performing standard single-step backs.
In my view, the complexity of introducing a new API to record metrics for historical entries outweighs the benefit of 100% consistency in metric reporting.
Are you open to keeping this scoped to the current page for now to get our initial data, perhaps with an added TODO noting the multi-step gap?
RenderFrameHostImpl* rfh = frame_tree_->root()->current_frame_host();Yao XiaoNot all history navigations involve the main frame. If this is only for the UKM, we might want to make that clearer.
Is it ok to report the UKM if no main frame navigation is involved, though? Or do we have a way to guarantee that this does involve a main frame navigation, based on other invariants about the intervention?
Charlie ReisIn the context of back-to-ad intervention, we treat subframe entries as same-doc main frame entries. So we can/should still report a metric (URL attributed to the main frame page). The test case `AdReplaceStateFollowedByAdFramePushState` covers this scenario.
Assuming this is ok, we still need to make sure that other code doesn't mistake `rfh` as being involved in the navigation. Can we rename it to something like `main_frame_rfh_for_ukm` and/or include a comment about this?
Done
What happens if the rest of the entries are skippable, when they weren't before (like line 1288)?
It looks like we'll finish the loop above without updating `result_index_with_ad_skipping` and the back-to-ad intervention won't skip the ad entries.
If that's true, we should fix that and add a test for it.
Great catch! I've fixed the issue and added a test for this scenario (`AdReplaceStateFollowedByAdPushState_NewTab`).
NavigationEntryImpl* start_entry = GetEntryAtIndex(from_index);Please move the comment about the cross-origin exception here above the origin computations, to make it clear why this block is needed.
Done
// collect data for future analysis.Does this mean a different site can be blamed for the ad? Or can we be clear in the metrics that this isn't about fault but about the number of times something like this occurs? (If so, I'm not sure why we need UKM rather than just UMA.)
The ad entry's site is *typically* the one reported in back navigation cases (our primary real-world scenario, tested via `AdReplaceStateFollowedByAdPushState`).
It can, however, attribute a non-ad site in forward navigations (`CrossOriginNavigationFollowedByAdReplaceStateFollowedByAdPushState`) or when mixed with standard skippable entries (`AdEntriesFollowedByOriginalSkippableEntries`).
Therefore, UKM is still valuable so that we can identify the specific sites causing the most common occurrences.
I've clarified this more in the code comment.
Maybe we should say "Original" (throughout) to avoid implying there's a standard or spec associated with this (given the note below about how that hasn't been done yet).
Done
## InvariantsPlease do not remove these invariants. There is a lot of reasoning and example bugs described here that are important when considering changes to the intervention. It looks like the new draft has removed the bulk of this.
Ack. I have relocated them to better fit the new structure. Most of them are now only applicable to the original intervention, so I moved them to the Invariants section under '1. NavigationEntry tagging for original history manipulation intervention'.
The two invariants that still broadly apply can now be found under 'Common Invariants' at the bottom of the page.
Let me know if you think a different structure would make this easier to follow.
The back-to-ad intervention (gated by feature flag `BackToAdIntervention`) makesThanks for mentioning this. Let's move it to a second sentence where we can be a little clearer, like:
This is currently disabled by default and gated by the `BackToAdIntervention` feature flag.
(Out of curiosity, will you be adding a chrome://flags entry for this in a future CL? We can update this to point to that once that's done.)
Done! Regarding the chrome://flags entry, I wasn't planning on adding one at this time.
Let's add: ", even if the page does have a user activation."
Done
Maybe add something like "and grants it a user activation."
Done
3. If the user presses back, the first same-document page (`a1`) will beYao Xiaos/page/entry/
Done
skipped, and the browser will go back to the page before `a.com` (e.g., aMaybe add: "because it was caused by an ad"?
Done
skipped, and the browser will go back to the page before `a.com` (e.g., a
New Tab Page).Done
script (target of `replaceState`) AND subsequently creates a new entry(e.g., ...)
Same on next line.
Done
(initiator of `pushState`), it is an ad-tagged entry. Whenever the user tries to
navigate back to this entry, the ad script logic will run again to turn the page
into an ad, effectively showing an unseen or unwanted ad.I might suggest:
If the user navigates back to such an entry, the ad script logic has a chance to inject a new ad into the user's session history.
Done
Maybe we can move the description of the origin restriction here?
I found it hard to explain the origin restriction here without introducing the concept of tagging first. It also ends up requiring a cross-reference back to the original history intervention. I've left it in the original spot for now to keep the flow clear, but let me know if you have a specific phrasing in mind that might make it work!
Because this only impacts browser UI, this is allowed by the spec, which onlyTo preserve more of the old line 14, let's slightly rephrase this to:
"The intervention only impacts the browser back/forward buttons and not the `history.back()/forward()` APIs. As a result, this is..."
Done
explanatory message.Will back-to-ad need an equivalent message at some point?
Agreed. We will likely need an equivalent message at some point to maintain consistency across the board.
* When a `NavigationEntry` is active and the document creates anotherAn entry can contain multiple documents. Does this mean "one of its documents"?
Done
* In the context of this tagging, a subframe entry is treated the same way asI'm wondering if there's a better way to phrase this. The tracking is done on NavigationEntry and not FrameNavigationEntry, so maybe it's clearer to say that a NavigationEntry's tagging is affected by both subframe and same-document navigations?
Done
nit: This seems like it should be a separate section, since it's not a third variant of the intervention.
Done
either marked as `should_skip_on_back_forward_ui_` (Standard) or are anYao Xiaonit: or as
Done
One exception to this rule is that we only skip `is_possibly_skippable_ad_entry`
if it takes the user to a cross-origin page relative to the page where theI don't think this quite captures it. The entry doesn't take the user somewhere or affect the exception on its own. At least in the current implementation, it's the end result of the skipping decision (which involves multiple skippable entries, from both versions of the intervention) which might be ignored if the navigation would land on a main frame which is cross-origin to the current main frame.
That means [A1, B_ad, C_skip, D_ad, A2*] might decide not to skip D_ad because the outcome would have been A1, which is same origin with A2. I'm still not thrilled with this, but if we're going to keep it we should be clear what the behavior is.
Ack. I've clarified based on the existing behavior.
currently maintain leniency regarding potential back-to-ad abuse when the userYao Xiaonit: are currently lenient
Done
Related details
- Charlie Reis
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. |
Charlie Reis (Gerrit)
Charlie Reis added 20 comments![]( "Open in Gerrit")
Thanks! I think we're getting close. For unrelated reasons, this week has been particularly challenging for me, so I haven't been able to do a full pass. I do think this round of comments should get us close to wrapping things up, though!
// page where the back navigation started.Yao XiaoWould it also have reported metrics for GoForwardWouldSkipAd, if the URL wasn't about:blank? That might be worth mentioning if we can't test for it.
Done. It was indeed because UKM doesn't record about:blank URLs, so I've added a comment explaining that. Additionally, I added the test CrossOriginNavigationFollowedByAdReplaceStateFollowedByAdPushState to explicitly show that kHistoryGoForwardWouldSkipAd triggers in that scenario.
Great, thanks!
EXPECT_EQ(GetWebContents()->GetLastCommittedURL(), GURL(url::kAboutBlankURL));Yao XiaoIs it worth testing the metric here as well? (I'm unclear if the goal is to do that on every test with skipping or not, since it is done again around line 330 but not in the following test.)
My goal wasn't to test this on every case. The assertion at `AdReplaceStateFollowedByAdFramePushState` was specifically to verify subframe UKM logging and main-frame URL attribution. Standard main-frame same-doc navigations don't really need the same redundant coverage. I'm happy to add it for consistency if you think it's needed, though!
That sounds fine-- not need to do it in every test.
std::optional<int> GetIndexForGoBack(bool performing_navigation);Yao XiaoThis new parameter is fairly awkward. If there isn't a way to do the metric reporting from the places performing the navigation, can we at least keep this more internal to the class without having it leak outside?
One option would be making this a default false parameter that no one has to provide, except for the internal GoBack/GoForward/GoToOffset functions.
Another would be having a private helper function that takes this parameter so that no callers outside NavigationControllerImpl would have to decide whether they need to pass it or not. I'm not sure if that's worth the extra code, if we can be clear in the documentation here that most callers should leave it as false.
Done. I went with the first suggestion: I've added a default false value to the parameter in these two public methods and expanded the documentation to clarify usage. (I kept the parameter mandatory for private methods to ensure internal callers remain explicit about the behavior.)
Thanks, that's better.
if (from_index == GetCurrentEntryIndex()) {Yao XiaoPer the question above, I'm unsure if this will work as expected. Even if we limit the metrics reporting to one of the iterative calls (i.e., when it starts from the current index), will that be the right value to report?
At first glance, it seems like a GetIndexForOffsetWithSkipping call could only skip standard intervention entries in the first iteration (and not report anything here), then skip ad related entries in the second iteration (which should have caused us to report metrics overall but wouldn't happen because from_index isn't the current index on that iteration).
Seems like we might want a test for that case, if we aren't catching this now.
(As a side note, I'm finding this quite complex and hard to reason about, which means it's hard to maintain and make changes. The more we can do to document and simplify this, the easier it will be to work in this code in the future.)
Charlie ReisI believe the existing behavior for metrics is intentional and acceptable:
- If the multi-step navigation `GetIndexForOffsetWithSkipping` occurs where the first step is "clean" (standard intervention only) but the second step involves ad-skipping, we will indeed skip reporting the metric for that second step.
- For the logic within `GetIndexWithSkipping`, it doesn't matter if there are prior standard skippable entries. As long as ad entries would be skipped, we record a UKM and attribute it to the page *where the back navigation started*.
While we could attempt to attribute the skip to a specific ad entry, it adds complexity -- it is more convenient to record a UKM for the active page than for an arbitrary page from the session history. Overall, this reporting scope is sufficient to collect data for future analysis.
I have added more comments to clarify the reasoning behind the metric recording.
Regarding the test: I have an existing test (`AdEntriesFollowedByStandardSkippableEntries`) that covers the attribution logic for the single-step case. Since `GetIndexForOffsetWithSkipping` is Android-specific, and given that the code explicitly gates metrics on `from_index == GetCurrentEntryIndex()`, I'd prefer to rely on the current unit tests unless you feel strongly that the multi-step edge case needs a specific regression test.
Yao XiaoSorry, I don't think I agree with this.
If the second iteration is affected by the back-to-ad intervention, I think we should be reporting that in metrics.
Your reasoning [above](https://chromium-review.googlesource.com/c/chromium/src/+/7533035/comment/fe790f01_1607397b/) ("Conceptually, it makes sense to handle GetIndexForOffset as iterative steps of Back/Forward") seems to imply the same thing. The second iteration would be like clicking back again, which would have reported the metrics.
I see your point, and conceptually I agree.
The challenge here is purely technical. Because we rely on `browser_client->LogWebFeatureForCurrentPage(rfh)`, we can't easily attribute metrics to a non-current page without building out a new API.
My rationale for the current scope is a cost-to-benefit trade-off. I'm assuming single-step navigations dominate user behavior. While we will intentionally under-report interventions that happen deep within a multi-step navigation, those will still be overwhelmingly caught by users performing standard single-step backs.
In my view, the complexity of introducing a new API to record metrics for historical entries outweighs the benefit of 100% consistency in metric reporting.
Are you open to keeping this scoped to the current page for now to get our initial data, perhaps with an added TODO noting the multi-step gap?
I see. This might be ok if we sufficiently document the limitation, including in the metrics descriptions.
Looking closer, it appears the iterative case might only be needed by Android WebView's goBackOrForward(int steps), which uses GoToOffset, per https://chromium-review.googlesource.com/c/chromium/src/+/2738819. It's unfortunate that this WebView API is adding this much complexity, but if that's really the only iterative case, then I agree that the lack of metrics in the iterative case shouldn't matter nearly as much in practice.
We should just be sure the limitations are clear to those interpreting the data.
// here because this must be a capability check, not a genuine navigation.Did you mean "might" instead of "must"?
It seems like we could use `performing_navigation` to tell the difference and decide whether to report a metric.
I could be ok with doing the metric in a followup if needed, but at first glance I don't understand the reason it's tricky.
// collect data for future analysis.Yao XiaoDoes this mean a different site can be blamed for the ad? Or can we be clear in the metrics that this isn't about fault but about the number of times something like this occurs? (If so, I'm not sure why we need UKM rather than just UMA.)
The ad entry's site is *typically* the one reported in back navigation cases (our primary real-world scenario, tested via `AdReplaceStateFollowedByAdPushState`).
It can, however, attribute a non-ad site in forward navigations (`CrossOriginNavigationFollowedByAdReplaceStateFollowedByAdPushState`) or when mixed with standard skippable entries (`AdEntriesFollowedByOriginalSkippableEntries`).
Therefore, UKM is still valuable so that we can identify the specific sites causing the most common occurrences.
I've clarified this more in the code comment.
Ok, thanks for updating the comment. I'm a bit worried about this making the data difficult to interpret, but maybe documentation for the UKMs can point out the considerations?
// collect data for future analysis.Yao XiaoDoes this mean a different site can be blamed for the ad? Or can we be clear in the metrics that this isn't about fault but about the number of times something like this occurs? (If so, I'm not sure why we need UKM rather than just UMA.)
The ad entry's site is *typically* the one reported in back navigation cases (our primary real-world scenario, tested via `AdReplaceStateFollowedByAdPushState`).
It can, however, attribute a non-ad site in forward navigations (`CrossOriginNavigationFollowedByAdReplaceStateFollowedByAdPushState`) or when mixed with standard skippable entries (`AdEntriesFollowedByOriginalSkippableEntries`).
Therefore, UKM is still valuable so that we can identify the specific sites causing the most common occurrences.
I've clarified this more in the code comment.
Acknowledged
## InvariantsYao XiaoPlease do not remove these invariants. There is a lot of reasoning and example bugs described here that are important when considering changes to the intervention. It looks like the new draft has removed the bulk of this.
Ack. I have relocated them to better fit the new structure. Most of them are now only applicable to the original intervention, so I moved them to the Invariants section under '1. NavigationEntry tagging for original history manipulation intervention'.
The two invariants that still broadly apply can now be found under 'Common Invariants' at the bottom of the page.
Let me know if you think a different structure would make this easier to follow.
Thanks for preserving them. That breakdown seems to work, with one suggestion below about the order.
The back-to-ad intervention (gated by feature flag `BackToAdIntervention`) makesYao XiaoThanks for mentioning this. Let's move it to a second sentence where we can be a little clearer, like:
This is currently disabled by default and gated by the `BackToAdIntervention` feature flag.
(Out of curiosity, will you be adding a chrome://flags entry for this in a future CL? We can update this to point to that once that's done.)
Done! Regarding the chrome://flags entry, I wasn't planning on adding one at this time.
Ah, a chrome://flags entry might be worthwhile for repro'ing bugs that people report, especially on platforms like ChromeOS or Android where it's hard to pass command line flags. Certainly no need in this CL, though.
Yao XiaoMaybe we can move the description of the origin restriction here?
I found it hard to explain the origin restriction here without introducing the concept of tagging first. It also ends up requiring a cross-reference back to the original history intervention. I've left it in the original spot for now to keep the flow clear, but let me know if you have a specific phrasing in mind that might make it work!
I see. Maybe we can just include a forward reference to it here, like:
The back-to-ad intervention has one exception to limit it to cross-origin navigations, which is described in greater detail below.
intervention.nit: Rephrase, since I find it confusing to say the member dictates the heuristic. Something like:
The original history manipulation intervention tags history items (i.e., `NavigationEntryImpl` objects) as skippable using the `should_skip_on_back_forward_ui_` member.
explanatory message.Yao XiaoWill back-to-ad need an equivalent message at some point?
Agreed. We will likely need an equivalent message at some point to maintain consistency across the board.
Sounds good. I'll let you decide whether to track that with a TODO or otherwise.
**Invariants:**Maybe "Invariants for Original Intervention", to be clear at a glance?
1. A history entry is marked `should_skip_on_back_forward_ui_` if the document =Is this `=` a typo?
entries), its history entry is not `should_skip_on_back_forward_ui_`. With annit: not marked
One exception to this rule is that we only skip `is_possibly_skippable_ad_entry`
if it takes the user to a cross-origin page relative to the page where theYao XiaoI don't think this quite captures it. The entry doesn't take the user somewhere or affect the exception on its own. At least in the current implementation, it's the end result of the skipping decision (which involves multiple skippable entries, from both versions of the intervention) which might be ignored if the navigation would land on a main frame which is cross-origin to the current main frame.
That means [A1, B_ad, C_skip, D_ad, A2*] might decide not to skip D_ad because the outcome would have been A1, which is same origin with A2. I'm still not thrilled with this, but if we're going to keep it we should be clear what the behavior is.
Ack. I've clarified based on the existing behavior.
Thanks, that explanation is much clearer.
Are there any back-to-ad-specific invariants we can list? I thought we came up with some in your doc.
### Common InvariantsCan we move this above the "NavigationEntry tagging" section? These invariant descriptions don't depend on the tagging details, and reading them first may be useful. (It also makes it easier to see that they aren't part of the back-to-ad intervention description, by the ordering of the doc and not just the formatting of the section titles.)
Related details
- Yao Xiao
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. |
Yao Xiao (Gerrit)
Yao Xiao added 10 comments![]( "Open in Gerrit")
if (from_index == GetCurrentEntryIndex()) {You're right that the WebView API is the only iterative case here. Unfortunately, unlike standard UMA/UKMs, UseCounter metrics don't support adding custom descriptions. To ensure the limitation is still well-documented, I've filed and linked to an explicit bug (crbug.com/489138113).
// here because this must be a capability check, not a genuine navigation.Did you mean "might" instead of "must"?
It seems like we could use `performing_navigation` to tell the difference and decide whether to report a metric.
I could be ok with doing the metric in a followup if needed, but at first glance I don't understand the reason it's tricky.
Good call. I used "must" because I *believe* it's impossible to hit this state during an actual navigation (`performing_navigation == true`). The preceding capability check will have already disabled the back button, preventing the user from clicking it at all. I've added a `DCHECK(!performing_navigation)` to enforce this assumption.
The existing navigation metric is correct as-is. The gap I meant is that we don't currently track the edge case where the intervention disables the back button entirely. Since logging during capability every check would be too noisy, I've filed https://crbug.com/489109712 to track finding a way to measure this. I've also simplified the code comment here to avoid further confusion.
Yao XiaoMaybe we can move the description of the origin restriction here?
Charlie ReisI found it hard to explain the origin restriction here without introducing the concept of tagging first. It also ends up requiring a cross-reference back to the original history intervention. I've left it in the original spot for now to keep the flow clear, but let me know if you have a specific phrasing in mind that might make it work!
I see. Maybe we can just include a forward reference to it here, like:
The back-to-ad intervention has one exception to limit it to cross-origin navigations, which is described in greater detail below.
Done
nit: Rephrase, since I find it confusing to say the member dictates the heuristic. Something like:
The original history manipulation intervention tags history items (i.e., `NavigationEntryImpl` objects) as skippable using the `should_skip_on_back_forward_ui_` member.
Done
Maybe "Invariants for Original Intervention", to be clear at a glance?
Done
1. A history entry is marked `should_skip_on_back_forward_ui_` if the document =Is this `=` a typo?
Done
entries), its history entry is not `should_skip_on_back_forward_ui_`. With anYao Xiaonit: not marked
Done
Are there any back-to-ad-specific invariants we can list? I thought we came up with some in your doc.
I've added some back-to-ad specific invariants.
Can we move this above the "NavigationEntry tagging" section? These invariant descriptions don't depend on the tagging details, and reading them first may be useful. (It also makes it easier to see that they aren't part of the back-to-ad intervention description, by the ordering of the doc and not just the formatting of the section titles.)
- Charlie Reis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 2 comments![]( "Open in Gerrit")
// here because this must be a capability check, not a genuine navigation.Yao XiaoDid you mean "might" instead of "must"?
It seems like we could use `performing_navigation` to tell the difference and decide whether to report a metric.
I could be ok with doing the metric in a followup if needed, but at first glance I don't understand the reason it's tricky.
Good call. I used "must" because I *believe* it's impossible to hit this state during an actual navigation (`performing_navigation == true`). The preceding capability check will have already disabled the back button, preventing the user from clicking it at all. I've added a `DCHECK(!performing_navigation)` to enforce this assumption.
The existing navigation metric is correct as-is. The gap I meant is that we don't currently track the edge case where the intervention disables the back button entirely. Since logging during capability every check would be too noisy, I've filed https://crbug.com/489109712 to track finding a way to measure this. I've also simplified the code comment here to avoid further confusion.
The preceding capability check will have already disabled the back button, preventing the user from clicking it at all. I've added a DCHECK(!performing_navigation) to enforce this assumption.
Hmm, I don't think that's correct, and I think that DCHECK might fail.
The back button stays enabled when all of the previous entries are skippable, as part of the changes in https://crbug.com/339188522. Still, the DCHECK would normally be safe for subtle reasons-- in that case, CanGoBack does indeed return false and the back button stays enabled via a separate check in [ShouldEnableBackButton](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_controller_impl.cc;drc=17f7265f1db3f3ca266310d68d3a44b1e3361dc4;l=1230-1248), so we would never get this far in GoBack due to [this condition](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_controller_impl.cc;drc=17f7265f1db3f3ca266310d68d3a44b1e3361dc4;l=1345-1347).
However, if the kBackToAdIntervention feature is disabled, we'll proceed through here in the `GoBack` case to get to the metrics below. That looks like it will cause the DCHECK to fail. I've just patched the CL in and confirmed that it fails by adding the following variation of the AdReplaceStateFollowedByAdPushState_NewTab test, which runs with the feature disabled:
```
IN_PROC_BROWSER_TEST_F(BackToAdInterventionDisabledBrowserTest,
AdReplaceStateFollowedByAdPushState_NewTab) {
// Open a new foreground tab. This gives us a pristine history stack that
// starts at frame_factory.html.
ui_test_utils::NavigateToURLWithDisposition(
browser(), GetURL("frame_factory.html"),
WindowOpenDisposition::NEW_FOREGROUND_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
// Quick sanity check to ensure our new active tab only has 1 history entry.
ASSERT_EQ(GetWebContents()->GetController().GetEntryCount(), 1);
ASSERT_TRUE(content::ExecJs(GetWebContents(), R"(
executeHistoryReplaceStateFromAdScript('replaced_state');
executeHistoryPushStateFromAdScript('new_state');
)"));
EXPECT_THAT(
GetNavigationUrls(),
::testing::ElementsAre(GetURL("replaced_state"), GetURL("new_state")));
EXPECT_TRUE(GetWebContents()->GetController().CanGoBack());
GoBack(); // <-- Fails DCHECK.
}
```
Yao XiaoAre there any back-to-ad-specific invariants we can list? I thought we came up with some in your doc.
I've added some back-to-ad specific invariants.
- Yao Xiao
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. |
Yao Xiao (Gerrit)
Yao Xiao added 1 comment![]( "Open in Gerrit")
Great catch, thank you for pointing this out and providing the test case! I've removed the DCHECK and switched to logging the metrics when `performing_navigation`. I've also added your test case as `BackToAdInterventionDisabledBrowserTest.AdReplaceStateFollowedByAdPushState_NewTab`.
Related details
- Charlie Reis
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. |
Charlie Reis (Gerrit)
Charlie Reis voted and added 6 comments![]( "Open in Gerrit")
Votes added by Charlie Reis
| Code-Review | +1 |
6 comments
Thanks! LGTM at this point, and thanks for your patience with the long review. While it does add complexity that we'll need to maintain going forward, I think there's enough documentation that this should be feasible, and the outcome seems worthwhile.
// here because this must be a capability check, not a genuine navigation.Thanks! That new logic looks good now.
3. An ad script on `b.com` invokes `replaceState` (creating entry `a1`) andnit: b1
subsequently `pushState` (creating entry `a2`). The page appearance does notnit: b2
4. If the user presses back, the first same-document entry (`a1`) will benit: b1
2. `NavigationController::CanGoBack()` will return false if all entries are
marked to be skipped on back/forward UI. On Android, pressing the back buttonLet's preserve this sentence somewhere in the UI Behavior section, since the CanGoBack behavior turned out to matter for our [reasoning](https://chromium-review.googlesource.com/c/chromium/src/+/7533035/comment/9366bb1c_726cb084/).
Related details
- Yao Xiao
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. |
Yao Xiao (Gerrit)
Yao Xiao added 5 comments![]( "Open in Gerrit")
3. An ad script on `b.com` invokes `replaceState` (creating entry `a1`) andYao Xiaonit: b1
Done
subsequently `pushState` (creating entry `a2`). The page appearance does notYao Xiaonit: b2
Done
4. If the user presses back, the first same-document entry (`a1`) will beYao Xiaonit: b1
Done
2. `NavigationController::CanGoBack()` will return false if all entries are
marked to be skipped on back/forward UI. On Android, pressing the back buttonLet's preserve this sentence somewhere in the UI Behavior section, since the CanGoBack behavior turned out to matter for our [reasoning](https://chromium-review.googlesource.com/c/chromium/src/+/7533035/comment/9366bb1c_726cb084/).
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
jkarlin@: PTAL at `ad_navigation_entry_browsertest.cc`. Thanks! (Note: Charlie has already done a thorough initial review of this.)
asvitkine@: PTAL at `ukm_features.cc`. Thanks! (We expect the collection volume to be <1% of page views. See the patchset description.).
Related details
- Alexei Svitkine
- Josh Karlin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexei Svitkine
| Code-Review | +1 |
1 comment
- Josh Karlin
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josh Karlin (Gerrit)
Josh Karlin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Josh Karlin
| Code-Review | +1 |
1 comment
- Yao Xiao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Implement Back-To-Ad Intervention behind a feature flag
Consolidates back/forward history skipping into GetIndexWithSkipping to
facilitate the Back-To-Ad intervention, and implements the intervention
logic:
- Feature Enabled: The navigation skips entries flagged by either
original history intervention (`should_skip_on_back_forward_ui`) or ad
intervention (`is_possibly_skippable_ad_entry`). *Exception*: If the
target entry is same-origin, the ad intervention check
(`is_possibly_skippable_ad_entry`) is ignored, and only original
history intervention is applied. See inline code comments for details.
- Feature Disabled: The ad intervention logic is bypassed, but metrics
are still recorded for analysis.
Note:
1. A better test file name should now be
'back_to_ad_intervention_browsertest.cc', but we defer the
renaming to a future CL. The old name is retained here to simplify
the diff for code review.
2. Regarding the allowlisting in ukm_feature.cc: We expect the
collection volume to be <1% of page views. This is because the
skipping logic relies on History API usage from ad scripts, and
existing UKM data (`HistoryApi.PushState`) confirms that API is used
on <1% of page loads.
Design Doc:
https://docs.google.com/document/d/1BN1c_VAM1yIIUch7ISQh1B4cOeOTV5k2FrJ1rEu0ST8/edit?resourcekey=0-DeM1fa3iJzoipqLLlQ_aUw&tab=t.0#heading=h.j7mf5f2xjyoe
- M chrome/browser/subresource_filter/ad_navigation_entry_browsertest.cc
- M components/page_load_metrics/browser/observers/use_counter/ukm_features.cc
- M content/browser/renderer_host/navigation_controller_history_intervention_browsertest.cc
- M content/browser/renderer_host/navigation_controller_impl.cc
- M content/browser/renderer_host/navigation_controller_impl.h
- M content/browser/renderer_host/navigation_entry_impl.cc
- M content/browser/renderer_host/navigation_entry_impl.h
- M content/public/browser/navigation_entry.h
- M content/public/common/content_features.cc
- M content/public/common/content_features.h
- M docs/history_manipulation_intervention.md
- M third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
- M tools/metrics/histograms/metadata/blink/enums.xml
Code-Review: +1 by Alexei Svitkine, +1 by Josh Karlin, +1 by Charlie Reis
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |