Report initiator url for resources fetched in event handler. [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Guohui Deng
| Commit-Queue | +1 |
1 comment
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 1 comment![]( "Open in Gerrit")
In addition, TaskAttribution uses a separate |taskIDForTest| for
scheduler::taskID(), which `task-tracking` tests rely on.
When TaskAttribution is forked with a variable setting to a new
value, |taskIDForTest| remains the same. This prevents
ResourceTimingContext propagated in event handler from conflicting
with the task-tracking tests.Would it be possible/easy to split this off into a separate CL and stack the other changes on top (gerrit supports this if you set the upstream branch in git)?
Related details
- Guohui Deng
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. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
In addition, TaskAttribution uses a separate |taskIDForTest| for
scheduler::taskID(), which `task-tracking` tests rely on.
When TaskAttribution is forked with a variable setting to a new
value, |taskIDForTest| remains the same. This prevents
ResourceTimingContext propagated in event handler from conflicting
with the task-tracking tests.Would it be possible/easy to split this off into a separate CL and stack the other changes on top (gerrit supports this if you set the upstream branch in git)?
- 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 3 comments![]( "Open in Gerrit")
// Currently, resource timing context is only propagated on the main thread.This block and the one below should be flag-guarded.
We'll want to check the overhead of this too on speedometer (with the feature enabled. I sometimes create a separate dependent branch enabling the feature to test this). I'm not sure if you have access to pinpoint, but the instructions are https://chromium.googlesource.com/chromium/src/+/main/docs/speed/perf_trybots.md (although it's speedometer3 now, not too). I'm happy to kick off a job for this too. I _think_ this should be okay, but we'll want to evaluate.
std::optional<scheduler::TaskAttributionTracker::TaskScope>Note: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
class RegisteredEventListener finalIs this needed for `NativeEventListener` as well as JS event listeners? Feels like `JSBasedEventListener` or `JSEventListener` might be a better place for this?
Related details
- Guohui Deng
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")
Related details
- Guohui Deng
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. |
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
// Currently, resource timing context is only propagated on the main thread.This block and the one below should be flag-guarded.
We'll want to check the overhead of this too on speedometer (with the feature enabled. I sometimes create a separate dependent branch enabling the feature to test this). I'm not sure if you have access to pinpoint, but the instructions are https://chromium.googlesource.com/chromium/src/+/main/docs/speed/perf_trybots.md (although it's speedometer3 now, not too). I'm happy to kick off a job for this too. I _think_ this should be okay, but we'll want to evaluate.
Added flag guard. Sorry I forgot this again and thank you for your patience!
Regarding the test: I can sign in and open the "run a try job" page so it looks like I have access! I will try to start the test later and if I run into problems I will ask for help. 😊
std::optional<scheduler::TaskAttributionTracker::TaskScope>Note: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
From the perspective of "initiator url" I much prefer propagating the "resource timing context" from script that sets up the handler, i.e., take the "context" at the "registration" (more details below). When I saw the planning of the AsyncContext I noticed they prefer propagating the "variables" along the script execution flow -- which makes sense for other applications of TaskAttribution(like soft navigation) and AsyncContext. I vaguely remembered I asked if asyncContext can accommodate this different propagating flow, and the answer was "yes with some extra work". So I hope this won't cause too much pain for you guys. If it adds too much work I will be happy to change. (maybe speaking with WebPerf WG too)
//////////////
The reason I prefer the current approach is that, the typical use case is that we "predict" the next resource fetch by previous resource that is fetched. In addition, the prediction is never reliable, because of the limitations that we have. (one noticeable limitation is that we can only pick up one script, instead of both the script that caused the event and the script that registered the handler). I think it's advantageous to report only "the script that registers the handler" because:
a) "registration script" would be expected to happen sooner. Typically, once the handler is registered, we can expect that the resource fetched in the handler can happen at any time.
b) "registration script" always exists. Events can result from user (and I would guess this is not a small portion) so that no "initiator" caused the event. Even if we make a "fallback" to report the "registering", then more different "initiator"s can be reported for the same resource and make the prediction more difficult. In addition, I prefer simpler definition.
What do you think?
Is this needed for `NativeEventListener` as well as JS event listeners? Feels like `JSBasedEventListener` or `JSEventListener` might be a better place for this?
It should be indeed in `JSBasedEventListener`. I made changes accordingly.
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. |
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Can all of this move into JSBasedEventListener, i.e. the constructor?
Aside from improving encapsulation, I think it might be required anyway for event attributes, e.g. `onfoo` for event "foo", when changing the listener. E.g.
```
document.onfoo = listener1;
// ... sometime later ...
document.onfoo = listener2;
```
In that case, IIUC, we update the callback in the existing registered listener and don't go down this code path. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/events/event_target.cc;l=822-825;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1
std::optional<scheduler::TaskAttributionTracker::TaskScope>Guohui DengNote: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
From the perspective of "initiator url" I much prefer propagating the "resource timing context" from script that sets up the handler, i.e., take the "context" at the "registration" (more details below). When I saw the planning of the AsyncContext I noticed they prefer propagating the "variables" along the script execution flow -- which makes sense for other applications of TaskAttribution(like soft navigation) and AsyncContext. I vaguely remembered I asked if asyncContext can accommodate this different propagating flow, and the answer was "yes with some extra work". So I hope this won't cause too much pain for you guys. If it adds too much work I will be happy to change. (maybe speaking with WebPerf WG too)
//////////////
The reason I prefer the current approach is that, the typical use case is that we "predict" the next resource fetch by previous resource that is fetched. In addition, the prediction is never reliable, because of the limitations that we have. (one noticeable limitation is that we can only pick up one script, instead of both the script that caused the event and the script that registered the handler). I think it's advantageous to report only "the script that registers the handler" because:a) "registration script" would be expected to happen sooner. Typically, once the handler is registered, we can expect that the resource fetched in the handler can happen at any time.
b) "registration script" always exists. Events can result from user (and I would guess this is not a small portion) so that no "initiator" caused the event. Even if we make a "fallback" to report the "registering", then more different "initiator"s can be reported for the same resource and make the prediction more difficult. In addition, I prefer simpler definition.What do you think?
w.r.t extra work, it's probably fine -- but long term it might depend on how AsyncContext gets implemented. Right now, we're setting up the context (for propagation) before dispatching the event, and you're hooking into a point after that, so it works. If we change this to also hook into events, we'll just have to make sure we get the ordering right.
---
I don't have a strong opinion on the feature itself, just want to make sure it's been thought through. But this should probably be documented in the explainer and discussed at WebPerfWG at some point (doesn't block this IMO).
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Similarly here, can this move into the js_based_event_listener? There's an `InvokeInternal()` method that runs to actually invoke the listener, and you could hook in there. You might need to create a helper method in the common base class to create the TaskScope.
Related details
- Guohui Deng
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Can all of this move into JSBasedEventListener, i.e. the constructor?
Aside from improving encapsulation, I think it might be required anyway for event attributes, e.g. `onfoo` for event "foo", when changing the listener. E.g.
```
document.onfoo = listener1;// ... sometime later ...
document.onfoo = listener2;
```In that case, IIUC, we update the callback in the existing registered listener and don't go down this code path. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/events/event_target.cc;l=822-825;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1
Done
std::optional<scheduler::TaskAttributionTracker::TaskScope>Guohui DengNote: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
Scott HaseleyFrom the perspective of "initiator url" I much prefer propagating the "resource timing context" from script that sets up the handler, i.e., take the "context" at the "registration" (more details below). When I saw the planning of the AsyncContext I noticed they prefer propagating the "variables" along the script execution flow -- which makes sense for other applications of TaskAttribution(like soft navigation) and AsyncContext. I vaguely remembered I asked if asyncContext can accommodate this different propagating flow, and the answer was "yes with some extra work". So I hope this won't cause too much pain for you guys. If it adds too much work I will be happy to change. (maybe speaking with WebPerf WG too)
//////////////
The reason I prefer the current approach is that, the typical use case is that we "predict" the next resource fetch by previous resource that is fetched. In addition, the prediction is never reliable, because of the limitations that we have. (one noticeable limitation is that we can only pick up one script, instead of both the script that caused the event and the script that registered the handler). I think it's advantageous to report only "the script that registers the handler" because:a) "registration script" would be expected to happen sooner. Typically, once the handler is registered, we can expect that the resource fetched in the handler can happen at any time.
b) "registration script" always exists. Events can result from user (and I would guess this is not a small portion) so that no "initiator" caused the event. Even if we make a "fallback" to report the "registering", then more different "initiator"s can be reported for the same resource and make the prediction more difficult. In addition, I prefer simpler definition.What do you think?
w.r.t extra work, it's probably fine -- but long term it might depend on how AsyncContext gets implemented. Right now, we're setting up the context (for propagation) before dispatching the event, and you're hooking into a point after that, so it works. If we change this to also hook into events, we'll just have to make sure we get the ordering right.
---
I don't have a strong opinion on the feature itself, just want to make sure it's been thought through. But this should probably be documented in the explainer and discussed at WebPerfWG at some point (doesn't block this IMO).
got it. I will take care of these. Thanks.
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Similarly here, can this move into the js_based_event_listener? There's an `InvokeInternal()` method that runs to actually invoke the listener, and you could hook in there. You might need to create a helper method in the common base class to create the TaskScope.
Done. As discussed, I moved the code to be besides "InvokeInternal()" and made additional tests for this.
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 7 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Maybe use existing `IsMainThread()` block and simplify a bit:
```
if (IsMainThread()) {
...
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
if (v8::Isolate* isolate =
ResourceInitiatorHelper::GetIsolateIfRunningScriptOnMainThread()) {
resource_timing_context_ = ...
}
}
}
```
std::optional<scheduler::TaskAttributionTracker::TaskScope>nit: I'd suggest moving this above the Step 10 comment because that is intended to match the spec and this isn't specced yet. And maybe add a comment above it pointing to the explainer.
nit: remove extra line (sorry to be so nit-picky(!), but removing the random extra whitespace/lines would improve readability a bit).
correct initiator_url is reported after event handler is overwritten .nit
```suggestion
correct initiator_url is reported after event handler is overwritten.
```
Related details
- Guohui Deng
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. |
Guohui Deng (Gerrit)
Guohui Deng added 6 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Maybe use existing `IsMainThread()` block and simplify a bit:
```
if (IsMainThread()) {
...
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
if (v8::Isolate* isolate =
ResourceInitiatorHelper::GetIsolateIfRunningScriptOnMainThread()) {
resource_timing_context_ = ...
}
}
}
```
Done
std::optional<scheduler::TaskAttributionTracker::TaskScope>nit: I'd suggest moving this above the Step 10 comment because that is intended to match the spec and this isn't specced yet. And maybe add a comment above it pointing to the explainer.
Done
nit: remove extra line (sorry to be so nit-picky(!), but removing the random extra whitespace/lines would improve readability a bit).
I really appreciate you being meticulous catching these errors. I am sorry I have missed so many in this file -- I forgot (again) that `git cl format` doesn't correct wpt test files.
correct initiator_url is reported after event handler is overwritten .nit
```suggestion
correct initiator_url is reported after event handler is overwritten.
```
Done
nit: remove leading whitespace line
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
1 comment
LGTM.
+Michal for second reviewer
+Nate to double-check bindings
Related details
- Guohui Deng
- Michal Mocny
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Chapin
| Code-Review | +1 |
1 comment
static v8::Isolate* GetIsolateIfRunningScriptOnMainThread() {Optional cleanup: Seems like this should maybe be a private implementation detail of this class, and `GetResourceTimingContext` and `GetScriptInitiatorUrl` should be 0-arg?
Related details
- Guohui Deng
- 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. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
static v8::Isolate* GetIsolateIfRunningScriptOnMainThread() {Optional cleanup: Seems like this should maybe be a private implementation detail of this class, and `GetResourceTimingContext` and `GetScriptInitiatorUrl` should be 0-arg?
The reason why this function needs to be exposed because we need to distinguish the following two:
a) There is no isolate that's in context.
b) There is an isolate that's in context but a valid `initiator-url` value is missing.
For a), the `initiator-url` is the parent file that statically includes the current resource; for b) we report empty string as the `initiator-url` because we know it's fetched by Javascript code but due to implementation limit we are not able to report the value.
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57841.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Code-Review+1![]( "Open in Gerrit")
- Guohui Deng
Guohui Deng (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Report initiator url for resources fetched in event handler.
When the event handler is set, the ResourceTimingContext from the
setting script is stored in the RegisteredEventListener and later
dispatched along with the event handler.
- M third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc
- M third_party/blink/renderer/bindings/core/v8/js_based_event_listener.h
- M third_party/blink/renderer/core/loader/resource_initiator_helper.h
- M third_party/blink/web_tests/TestExpectations
- M third_party/blink/web_tests/external/wpt/resource-timing/resources/loading-resource-lib.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/event-handler-overwritten.html
- M third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/event-handler.html
Code-Review: +1 by Scott Haseley, +1 by Michal Mocny, +1 by Nate Chapin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57841
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |