[soft-navs] SoftNavigationContext created in terms of Interactions. [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 19 comments![]( "Open in Gerrit")
This looks quite reasonable, but I need to read SNH again with a fresh brain in the morning. Some of the failed tests / crashes look interesting, but I didn't look into them.
const PerformanceTimelineEntryIdInfo interaction_id_ =I know it's rather innocuous having this, but I think it would be better to push this to the patch it's used in, unless you can do a stack and you think we'll get them both in before branch. Same for HashChangeEvent.
/*is_browser_initiated=*/false, /*is_synchronously_committed=*/true);Are these needed or should can they continue using the default values?
/*is_synchronously_committed=*/true);Is this needed, or should it just continue using the default value?
if (auto id = event_timing_scope.GetInteractionIdInfo()) {style nit: this might be clearer without auto since the type isn't obvious
(see https://google.github.io/styleguide/cppguide.html#Type_deduction).
CHECK(entry);
CHECK(entry->IsKnownToBeAnInteraction());Should these checks also be applied to the `initial_event_timing_`?
uint64_t interaction_id = initial_event_timing_->interactionId();nit: consider inlining below since this is only used once.
CHECK(window_);Why move these up? FWIW I had the CHECK(window_) below the flag check because I didn't want the check to fail in prod without the feature enabled.
uint64_t interaction_id = initial_event_timing_->interactionId();nit: same here - consider inlining below since this is only used once.
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Do we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
if (context) {How can this be null? Entries should be automatically removed by GC and never null.
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failnit: removing "created in one window" makes sound unclear to me now.
for (const auto& tracked_context : interaction_id_to_context_.Values()) {I think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
SoftNavigationContext* context_for_task =Move above line 287 and use that in the ternary expression?
if (context && context->OnPaintFinished()) {I don't think this can be null
TRACE_EVENT_BEGIN("loading", "SoftNavigation",We really should move this into SNC...
class PerformanceEventTiming;nit: merge with other forward declarations above.
reporting_info.processing_end_time = start_time;Worth giving this some kind of duration?
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));We could also, not necessarily now, make a base class for tests with an id generator member and this as a method, and each interaction could use this (or use the same ID via a "should_increment" parameter or something).
Related details
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 16 comments![]( "Open in Gerrit")
Ty for review!
Replies first, will work on addressing these next...
const PerformanceTimelineEntryIdInfo interaction_id_ =I know it's rather innocuous having this, but I think it would be better to push this to the patch it's used in, unless you can do a stack and you think we'll get them both in before branch. Same for HashChangeEvent.
Sg. This was more of an "initial direction" that proved unneeded.
/*is_browser_initiated=*/false, /*is_synchronously_committed=*/true);Are these needed or should can they continue using the default values?
good eye-- I had added interaction_id to last param initially, then switched the order specifically to get to do that-- but forgot to make this change :P
// Soft LCP. It is the startTime of the last event timing entry before first
// paint.this changed. It is now first event timing.
Pray I do not change it further :)
CHECK(entry);
CHECK(entry->IsKnownToBeAnInteraction());Should these checks also be applied to the `initial_event_timing_`?
In the constructor? Good idea!
uint64_t interaction_id = initial_event_timing_->interactionId();nit: consider inlining below since this is only used once.
I added `GetInteractionIdInfo` to this class specifically so I wouldn't need to hard-code things like this here, but missed this one. ty
CHECK(window_);Why move these up? FWIW I had the CHECK(window_) below the flag check because I didn't want the check to fail in prod without the feature enabled.
Oh I thought SoftNavigationHeuristicsEnabled needed `window_` and so moved up... ty for noticing
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Do we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
I can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failnit: removing "created in one window" makes sound unclear to me now.
sg.
I wonder if EnsureContextForCurrentWindow should just be folded into GetSoftNavigationContextForCurrentTask() now that we don't pass context* around?
The only other call site is `ReportSoftNavigationToMetrics` but that is already after we emit to perf timeline and have a whole bunch of effects, so probably not needed there.
(Ill leave for this patch)
for (const auto& tracked_context : interaction_id_to_context_.Values()) {I think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Let me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
uint64_t interaction_id) const {TODO: try to pass the full typed struct where possible.
SoftNavigationContext* context_for_task =Move above line 287 and use that in the ternary expression?
sg, originally I didnt even have this but some of the tests failed because I made mistakes while setting them up so added this guard. It will be possible to remove completely, I just wanted to check the try-bots with it. Should have left a comment.
I could also leave this in, but add NotFatal? since the worst effect of this getting wrong is that we assign the wrong id/starttime to the wrong context. We should know, but maybe not worth crashing.
TRACE_EVENT_BEGIN("loading", "SoftNavigation",We really should move this into SNC...
I can do that. I think there are a few other reporting things in here that could, but I'll limit the moves for now.
reporting_info.processing_end_time = start_time;Worth giving this some kind of duration?
Yeah. Even better would be to merge the Utils that window_performance_test.cc uses (which has a lot more careful setup of invarients and simulation for lifecycle)...
Might have to move/rename util files around, but for now I think I'll just duplicate the code.
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));(From AI reviewer:)
Hardcoding the interaction ID to (1, 1) can lead to incorrect test behavior. For example, in `SoftNavigationHeuristicsTest.HeuristicNotResetDuringGCWithActiveContext`, two separate click events are simulated, but they will end up with the same interaction ID and reuse the same `SoftNavigationContext`, which is likely not the intent.
Consider using an ID generator here, or passing the ID in as a parameter, similar to the fix in `InteractionEffectsMonitorTest`.
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));We could also, not necessarily now, make a base class for tests with an id generator member and this as a method, and each interaction could use this (or use the same ID via a "should_increment" parameter or something).
I think that the InteractionID Unittest base in window_performance_test.cc does some of this and probably the soft nav / icp tests should eventually become a subclass of that... But yeah, larger test refactor for later.
Related details
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
Also, found the bug that caused tests to crash:
I added `PerformanceEventTiming::IsKnownToBeAnInteraction` but theres a subtle bug: some interactions which have an id>0 are "ignored" and treated as 0 anyway-- like autoscroll, which is where the tests were failing.
We were accidentally trying to create context with id==0 which the hashmap didn't like using as a key. Good catch, bots!
(There autoscroll are distinct from gesture scroll, which we wont count for different reasons)
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Let me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
I added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Related details
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny added 17 comments![]( "Open in Gerrit")
/*is_browser_initiated=*/false, /*is_synchronously_committed=*/true);Michal MocnyAre these needed or should can they continue using the default values?
good eye-- I had added interaction_id to last param initially, then switched the order specifically to get to do that-- but forgot to make this change :P
Done
Is this needed, or should it just continue using the default value?
Done
if (auto id = event_timing_scope.GetInteractionIdInfo()) {style nit: this might be clearer without auto since the type isn't obvious
(see https://google.github.io/styleguide/cppguide.html#Type_deduction).
Done
// Soft LCP. It is the startTime of the last event timing entry before first
// paint.this changed. It is now first event timing.
Pray I do not change it further :)
Done
CHECK(entry);
CHECK(entry->IsKnownToBeAnInteraction());Michal MocnyShould these checks also be applied to the `initial_event_timing_`?
In the constructor? Good idea!
Done
// `heuristics` will be null if the `window_` was detached but this contextrestore this
uint64_t interaction_id = initial_event_timing_->interactionId();Michal Mocnynit: consider inlining below since this is only used once.
I added `GetInteractionIdInfo` to this class specifically so I wouldn't need to hard-code things like this here, but missed this one. ty
Done
Michal MocnyWhy move these up? FWIW I had the CHECK(window_) below the flag check because I didn't want the check to fail in prod without the feature enabled.
Oh I thought SoftNavigationHeuristicsEnabled needed `window_` and so moved up... ty for noticing
Done
uint64_t interaction_id = initial_event_timing_->interactionId();Michal Mocnynit: same here - consider inlining below since this is only used once.
Done
How can this be null? Entries should be automatically removed by GC and never null.
Done
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failMichal Mocnynit: removing "created in one window" makes sound unclear to me now.
sg.
I wonder if EnsureContextForCurrentWindow should just be folded into GetSoftNavigationContextForCurrentTask() now that we don't pass context* around?
The only other call site is `ReportSoftNavigationToMetrics` but that is already after we emit to perf timeline and have a whole bunch of effects, so probably not needed there.
(Ill leave for this patch)
After the change to the "hot path" check for SNH, this function became a one liner-- so I just inlined it and remvoed it.
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Michal MocnyLet me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
I added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Done
TODO: try to pass the full typed struct where possible.
Done
I don't think this can be null
Done
Michal MocnyWe really should move this into SNC...
I can do that. I think there are a few other reporting things in here that could, but I'll limit the moves for now.
I moved a small amount of tracing now, but added a comment to the crbug to followup
nit: merge with other forward declarations above.
Done
Michal MocnyWorth giving this some kind of duration?
Yeah. Even better would be to merge the Utils that window_performance_test.cc uses (which has a lot more careful setup of invarients and simulation for lifecycle)...
Might have to move/rename util files around, but for now I think I'll just duplicate the code.
fixed a small amount here, but added comment to crbug to followup with a larger unification here.
Scott Haseley (Gerrit)
Scott Haseley added 4 comments![]( "Open in Gerrit")
Still catching up. Brain-dumping my sleep thoughts before I grab breakfast.
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Michal MocnyDo we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
I can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
The problem was simulated event (untrusted) causing a trusted event. This happens because of form controls. See https://chromium-review.googlesource.com/c/chromium/src/+/7466724 (if you're passing that test, this might be okay now)
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Michal MocnyLet me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
I added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Thought of that at 4AM in my sleep too... Yeah I think we can just check context->GetWindow() == Window() (making these up). That was why this was added IIRC (cross-window main frame case w/ window.opener()).
If we want to (eventually) exclude other contexts (e.g. if we shutdown early), we can check if they've been shutdown or whatever.
SoftNavigationContext* context = GetRelevantContext(interaction_id);Since the map holds WeakMember<context>, I think there's an edge case where we can create multiple contexts for the same interaction_id. Not sure if you thought about this? I think most won't matter in practice, but the retainer dependency is a bit gnarly (it relies on the paint attribution tracker to manage lifetime).
Okay example:
- keydown
- dom update
- paint tracker holds strong reference
- keyup
finds context
Benign example:
- keydown, create context
- keydown does nothing
- GC happens -- context GCed
- keyup
- new context
Edge case:
- keydown, create context; hold key (same can happen once pointer down gets id)
- update some dom
- paint
- emit ICP
- ...
- remove said updated dom
- GC
- context GCed because no more retainers
- keyup
- no matching context for id, new context
Related details
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +1 |
1 comment
Michal MocnyMove above line 287 and use that in the ternary expression?
sg, originally I didnt even have this but some of the tests failed because I made mistakes while setting them up so added this guard. It will be possible to remove completely, I just wanted to check the try-bots with it. Should have left a comment.
I could also leave this in, but add NotFatal? since the worst effect of this getting wrong is that we assign the wrong id/starttime to the wrong context. We should know, but maybe not worth crashing.
- Scott Haseley
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
Looks that this line (processingEnd) needs to be deleted, to reflect that it's no longer part of the SoftNavigation object.
Related details
- Michal Mocny
- Scott Haseley
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. |
Scott Haseley (Gerrit)
Scott Haseley added 5 comments![]( "Open in Gerrit")
Just a couple comments, will review again after lunch.
// Soft LCP. It is the startTime of the last event timing entry before first
// paint.Michal Mocnythis changed. It is now first event timing.
Pray I do not change it further :)
Done
😄
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Michal MocnyDo we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
Scott HaseleyI can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
The problem was simulated event (untrusted) causing a trusted event. This happens because of form controls. See https://chromium-review.googlesource.com/c/chromium/src/+/7466724 (if you're passing that test, this might be okay now)
Ah, yeah it's still failing.
The problem is that if you dispatch a simulated click to an element with a target, we dispatch a trusted click to the target. So we create EventTimings for such events, but soft navs filters them out.
I suspect these should be filtered out from EventTiming as well, and maybe that's relatively safe to do? Not sure.
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failMichal Mocnynit: removing "created in one window" makes sound unclear to me now.
Michal Mocnysg.
I wonder if EnsureContextForCurrentWindow should just be folded into GetSoftNavigationContextForCurrentTask() now that we don't pass context* around?
The only other call site is `ReportSoftNavigationToMetrics` but that is already after we emit to perf timeline and have a whole bunch of effects, so probably not needed there.
(Ill leave for this patch)
After the change to the "hot path" check for SNH, this function became a one liner-- so I just inlined it and remvoed it.
Sweet!
// TODO(crbug.com/40871933): We don't care to support datetime modals, but
// this behaviour might be similar for iframes, and might be worth
// supporting.note: We ran into this in practice with window.open(), in which case you get two main frames in the same process that can script each other.
Related details
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 6 comments![]( "Open in Gerrit")
// `heuristics` will be null if the `window_` was detached but this contextMichal Mocnyrestore this
Done
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Michal MocnyDo we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
Scott HaseleyI can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
The problem was simulated event (untrusted) causing a trusted event. This happens because of form controls. See https://chromium-review.googlesource.com/c/chromium/src/+/7466724 (if you're passing that test, this might be okay now)
Done
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Michal MocnyLet me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
Scott HaseleyI added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Thought of that at 4AM in my sleep too... Yeah I think we can just check context->GetWindow() == Window() (making these up). That was why this was added IIRC (cross-window main frame case w/ window.opener()).
If we want to (eventually) exclude other contexts (e.g. if we shutdown early), we can check if they've been shutdown or whatever.
Done
// TODO(crbug.com/40871933): We don't care to support datetime modals, but
// this behaviour might be similar for iframes, and might be worth
// supporting.note: We ran into this in practice with window.open(), in which case you get two main frames in the same process that can script each other.
(this comment just moved from somewhere earlier, I didn't add it-- I hope nothing abut the specifics change here.)
SoftNavigationContext* context = GetRelevantContext(interaction_id);Great points-- but I think this would be WAI and the fact that each attempt to get a context by id with an event timing doesn't assume there was a past event means this should just work.
Or do you spot a problem?
We should add tests for it afterwards!
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));(From AI reviewer:)
Hardcoding the interaction ID to (1, 1) can lead to incorrect test behavior. For example, in `SoftNavigationHeuristicsTest.HeuristicNotResetDuringGCWithActiveContext`, two separate click events are simulated, but they will end up with the same interaction ID and reuse the same `SoftNavigationContext`, which is likely not the intent.
Consider using an ID generator here, or passing the ID in as a parameter, similar to the fix in `InteractionEffectsMonitorTest`.
Cannot use a generator here, but we will move this into a test base that will do this, in a subsequent patch. The tests that need separate ids can manually set them, for now.
Related details
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +1 |
Looks that this line (processingEnd) needs to be deleted, to reflect that it's no longer part of the SoftNavigation object.
I added it back to tracing for now, I thought it wasn't needed at all but forgot about these tests.
We could also expose more of the event timings if we want... or even the full list of them.
Related details
- Johannes Henkel
- Scott Haseley
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. |
Scott Haseley (Gerrit)
Scott Haseley added 13 comments![]( "Open in Gerrit")
InteractionContextWrapperScope interaction_context_wrapper;nit/optional: maybe comment that browser-initiated navigations are considered interactions and this scope sets up handling that for metrics?
}ultra-nit: probably keep the newline before this and add one after to separate the task-attribution block
// is wrapper in this scope, it std::move() the task_scope up to it instead.```suggestion
// is wrapped in this scope, it std::move()d the task_scope up to it instead.
```
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!
(I actually think I need to nix the TaskScope move c'tors!)
if (!event.IsFullyTrusted()) {Do you know if there's a spec issue for this? This could be a compat issue.
void AddEventTiming(PerformanceEventTiming* entry);Can we remove this? `last_event_timing_` doesn't matter anymore now that you're using the first one.
explicit SoftNavigationContext(LocalDOMWindow& window,nit: can be removed now
TRACE_EVENT_BEGIN("loading", "SoftNavigation",nice
CHECK(initial_event_timing_);nit: You could probably (?) make `initial_event_timing_` const and/or remove this check, since it's already guaranteed to be valid in the c'tor and we don't change it.
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {Did something change that can make this null now, like in tests? Previously, the heuristics was guaranteed to be valid because:
- It's created by the heuristics class (except in tests)
- if window_ had a heuristics, it's guaranteed to stay until detach
- This is shut down on detatch, and OnPaintFinished() shouldn't be called after detach
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {You didn't like my style? :P
(FWIW I did that to be consistent with the `if (!condition) { return }` and have the actual logic at the end, as is somewhat idiomatic in chrome (not always tho..))
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {Similar question, have the guarantees changed? This should never be called after detach.
The problem with the last one is the non-determinism -- sometimes things are grouped, sometimes they're not, depending on GC.
I think it's fine for OT_v1 and shouldn't block this, but I think there's a disconnect in the layering that maybe we can fix when InteractionContext moves out of soft navs, like maybe the context creation and mapping needs to be built into the responsiveness metrics state machine (w/ stronger lifetime guarantees).
Related details
- Johannes Henkel
- Michal Mocny
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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!(I actually think I need to nix the TaskScope move c'tors!)
Update: I think this is something task attribution should solve, not event timing. Really this is a problem with propagating task state from the intercept() to handler(), which is clear causation. Let me try to add that.