[Extensions] Resolve per-context webRequest blocking dispatch targets [chromium/src : main]
Andrea Orru (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 11 comments![]( "Open in Gerrit")
"webRequestInternal.eventHandled". Each response is clamped to thenitty nit: maybe add "as they do today" to indicate this flow isn't new.
content::BrowserContext* browser_context,same comments re ref, ChildProcessId
int render_process_id,nit: ChildProcessId?
content::BrowserContext* browser_context,nit: pass by ref
!key.IsLazy() && key.web_view_instance_id != 0;is there such a thing as a lazy webview listener? I feel like there shouldn't be. Can we make this a CHECK somewhere (or multiple places)?
if (!responding_target) {when do we expect this? (Add a comment)
// If no exact match exists, check if this is a newly started context
// operating under a concrete identity for an event dispatched under its lazy
// key. In that case, fall back to matching the existing lazy target for the
// same browser context, extension, and webview instance.
return std::ranges::find_if(
blocked_request.pending_targets, [&key](const auto& entry) {
const DispatchTargetKey& candidate_key = entry.first;
return candidate_key.IsLazy() &&
candidate_key.listener_context_id == key.listener_context_id &&
candidate_key.extension_id == key.extension_id &&
candidate_key.web_view_instance_id == key.web_view_instance_id;
});hmm... is this / should this be possible?
A lazy context starts. There are only lazy listeners.
- Context adds listeners before the event gets there. This means we'll have gone through the IPC to add the event listener, which should happen on the same associated message channel and thus the listener should be active (right?)
- Or, context doesn't add listeners in time (unsupported async registration). The listeners aren't registered, so they can't get the event.
Is there a scenario in which the listener gets the event, but we haven't received the message that it's been added?
return RespondingTarget{blocked_request, it};passing iterators into maps from methods is, to me, a bit of a code smell -- it's very easy to accidentally have the iterator become stale without easily noticing it.
It also doesn't look like we ever use the returned iterator _as an iterator_. Could we instead have this return the blocked request and extra info flags?
// completion signal that arrived before this deferred task executed).if the event couldn't dispatch, how could we get a completion signal?
Or should this instead be something like the process going away? (I guess in some ways that's a "completion signal", but isn't what I'd normally think of)
// Requests may be missing if already completed or canceled, or blocked on
// a different event stage if the request advanced past `event_name` before
// a delayed response arrived.how would this happen?
Related details
- Andrea Orru
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. |
Andrea Orru (Gerrit)
Andrea Orru added 4 comments![]( "Open in Gerrit")
Answering a few of the core questions before I make more code changes.
!key.IsLazy() && key.web_view_instance_id != 0;is there such a thing as a lazy webview listener? I feel like there shouldn't be. Can we make this a CHECK somewhere (or multiple places)?
Acknowledged. There's no such thing as a lazy webview listener and we can add CHECKs.
// If no exact match exists, check if this is a newly started context
// operating under a concrete identity for an event dispatched under its lazy
// key. In that case, fall back to matching the existing lazy target for the
// same browser context, extension, and webview instance.
return std::ranges::find_if(
blocked_request.pending_targets, [&key](const auto& entry) {
const DispatchTargetKey& candidate_key = entry.first;
return candidate_key.IsLazy() &&
candidate_key.listener_context_id == key.listener_context_id &&
candidate_key.extension_id == key.extension_id &&
candidate_key.web_view_instance_id == key.web_view_instance_id;
});hmm... is this / should this be possible?
A lazy context starts. There are only lazy listeners.
- Context adds listeners before the event gets there. This means we'll have gone through the IPC to add the event listener, which should happen on the same associated message channel and thus the listener should be active (right?)
- Or, context doesn't add listeners in time (unsupported async registration). The listeners aren't registered, so they can't get the event.
Is there a scenario in which the listener gets the event, but we haven't received the message that it's been added?
To answer your question first... There's no scenario where the listener gets the event before we've received the registration. That's not why this code is here.
This is actually the "normal" path, not an edge case. Probably the comments don't make it clear.
The pending target is written at dispatch time using the lazy key, because a sleeping context has no other identity at that point. But once awake, the context always answers under its concrete identity (actual process ID, worker version etc.). So for any event dispatched while the context was sleeping, the answer's key can never equal the lazy key in the map.
// completion signal that arrived before this deferred task executed).if the event couldn't dispatch, how could we get a completion signal?
Or should this instead be something like the process going away? (I guess in some ways that's a "completion signal", but isn't what I'd normally think of)
The "process going away" case is not handled in the current CL, but in a follow-up (it's mentioned in the commit description, although perhaps just in passing).
A process/worker can be torn down, in which case a real `eventHandlingDone` cannot be issued, and the browser needs to manage the equivalent of a "completion signal".
Related details
- Devlin Cronin
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. |
Andrea Orru (Gerrit)
Andrea Orru voted and added 4 comments![]( "Open in Gerrit")
Votes added by Andrea Orru
| Commit-Queue | +1 |
4 comments
"webRequestInternal.eventHandled". Each response is clamped to thenitty nit: maybe add "as they do today" to indicate this flow isn't new.
Done
same comments re ref, ChildProcessId
Done
int render_process_id,Andrea Orrunit: ChildProcessId?
Done
content::BrowserContext* browser_context,Andrea Orrunit: pass by ref
Done
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
// If no exact match exists, check if this is a newly started context
// operating under a concrete identity for an event dispatched under its lazy
// key. In that case, fall back to matching the existing lazy target for the
// same browser context, extension, and webview instance.
return std::ranges::find_if(
blocked_request.pending_targets, [&key](const auto& entry) {
const DispatchTargetKey& candidate_key = entry.first;
return candidate_key.IsLazy() &&
candidate_key.listener_context_id == key.listener_context_id &&
candidate_key.extension_id == key.extension_id &&
candidate_key.web_view_instance_id == key.web_view_instance_id;
});Andrea Orruhmm... is this / should this be possible?
A lazy context starts. There are only lazy listeners.
- Context adds listeners before the event gets there. This means we'll have gone through the IPC to add the event listener, which should happen on the same associated message channel and thus the listener should be active (right?)
- Or, context doesn't add listeners in time (unsupported async registration). The listeners aren't registered, so they can't get the event.
Is there a scenario in which the listener gets the event, but we haven't received the message that it's been added?
To answer your question first... There's no scenario where the listener gets the event before we've received the registration. That's not why this code is here.
This is actually the "normal" path, not an edge case. Probably the comments don't make it clear.
The pending target is written at dispatch time using the lazy key, because a sleeping context has no other identity at that point. But once awake, the context always answers under its concrete identity (actual process ID, worker version etc.). So for any event dispatched while the context was sleeping, the answer's key can never equal the lazy key in the map.
Got it; thanks for the details. This makes sense.
It's probably worth updating the comment to explain this a bit more thoroughly, but the general code flow makes sense.
// completion signal that arrived before this deferred task executed).Andrea Orruif the event couldn't dispatch, how could we get a completion signal?
Or should this instead be something like the process going away? (I guess in some ways that's a "completion signal", but isn't what I'd normally think of)
The "process going away" case is not handled in the current CL, but in a follow-up (it's mentioned in the commit description, although perhaps just in passing).
A process/worker can be torn down, in which case a real `eventHandlingDone` cannot be issued, and the browser needs to manage the equivalent of a "completion signal".
got it -- but then we should still document when another completion signal could arrive : )
Related details
- Andrea Orru
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. |
Andrea Orru (Gerrit)
Andrea Orru added 1 comment![]( "Open in Gerrit")
passing iterators into maps from methods is, to me, a bit of a code smell -- it's very easy to accidentally have the iterator become stale without easily noticing it.
It also doesn't look like we ever use the returned iterator _as an iterator_. Could we instead have this return the blocked request and extra info flags?
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. |
Andrea Orru (Gerrit)
Andrea Orru added 5 comments![]( "Open in Gerrit")
Andrea Orruis there such a thing as a lazy webview listener? I feel like there shouldn't be. Can we make this a CHECK somewhere (or multiple places)?
Acknowledged. There's no such thing as a lazy webview listener and we can add CHECKs.
Done
when do we expect this? (Add a comment)
Done
// If no exact match exists, check if this is a newly started context
// operating under a concrete identity for an event dispatched under its lazy
// key. In that case, fall back to matching the existing lazy target for the
// same browser context, extension, and webview instance.
return std::ranges::find_if(
blocked_request.pending_targets, [&key](const auto& entry) {
const DispatchTargetKey& candidate_key = entry.first;
return candidate_key.IsLazy() &&
candidate_key.listener_context_id == key.listener_context_id &&
candidate_key.extension_id == key.extension_id &&
candidate_key.web_view_instance_id == key.web_view_instance_id;
});Andrea Orruhmm... is this / should this be possible?
A lazy context starts. There are only lazy listeners.
- Context adds listeners before the event gets there. This means we'll have gone through the IPC to add the event listener, which should happen on the same associated message channel and thus the listener should be active (right?)
- Or, context doesn't add listeners in time (unsupported async registration). The listeners aren't registered, so they can't get the event.
Is there a scenario in which the listener gets the event, but we haven't received the message that it's been added?
Devlin CroninTo answer your question first... There's no scenario where the listener gets the event before we've received the registration. That's not why this code is here.
This is actually the "normal" path, not an edge case. Probably the comments don't make it clear.
The pending target is written at dispatch time using the lazy key, because a sleeping context has no other identity at that point. But once awake, the context always answers under its concrete identity (actual process ID, worker version etc.). So for any event dispatched while the context was sleeping, the answer's key can never equal the lazy key in the map.
Got it; thanks for the details. This makes sense.
It's probably worth updating the comment to explain this a bit more thoroughly, but the general code flow makes sense.
Done
// completion signal that arrived before this deferred task executed).Andrea Orruif the event couldn't dispatch, how could we get a completion signal?
Or should this instead be something like the process going away? (I guess in some ways that's a "completion signal", but isn't what I'd normally think of)
Devlin CroninThe "process going away" case is not handled in the current CL, but in a follow-up (it's mentioned in the commit description, although perhaps just in passing).
A process/worker can be torn down, in which case a real `eventHandlingDone` cannot be issued, and the browser needs to manage the equivalent of a "completion signal".
got it -- but then we should still document when another completion signal could arrive : )
Done
// Requests may be missing if already completed or canceled, or blocked on
// a different event stage if the request advanced past `event_name` before
// a delayed response arrived.Andrea Orruhow would this happen?
Done
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. |
Andrea Orru (Gerrit)
Andrea Orru voted Commit-Queue+1![]( "Open in Gerrit")
- Devlin Cronin
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. |
Andrea Orru (Gerrit)
Andrea Orru added 1 comment![]( "Open in Gerrit")
Answering a few of the core questions before I make more code changes.
Done
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
3 comments
GroupListenersByDispatchTarget(const RawListeners& listeners);Is the only reason this is a static method (as opposed to an anonymous namespace method in the .cc file) because it returns a DispatchTargetKey, which is a private type? If so, maybe put a comment? (In most cases, private statics are kind of a code smell)
if (blocked_request.pending_targets.erase(target_key) == 0) {
// The target was already resolved. Several independent triggers can
// resolve a pending target, in any order:
// - Renderer completion signals: renderer-controlled IPCs, which a
// misbehaving renderer can send unsolicited.
// - `OnTargetCannotDispatch()`, which runs from a posted task.
// - Teardown of the target's renderer context.
// A trigger that runs after the target is resolved must do nothing; in
// particular, it must not decrement the block count again.
return;
}this... still seems like we should be able to be stricter here (e.g. maybe renderer kills).
1) Misbehaving renderer: should terminate the renderer
2) "CannotDispatch". This should only happen as a "second" signal if the renderer context was eliminated. We should never get a "cannot dispatch" for a live renderer as the second signal, since if we can't dispatch, we shouldn't get that first signal.
3) Renderer teardown should probably only enter here if the renderer is being tracked as a pending target.
This is probably okay for this CL, but I wonder if we should add a TODO to tighten it up? WDYT?
Related details
- Andrea Orru
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. |
Andrea Orru (Gerrit)
Andrea Orru voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrea Orru
| Commit-Queue | +2 |
2 comments
GroupListenersByDispatchTarget(const RawListeners& listeners);Is the only reason this is a static method (as opposed to an anonymous namespace method in the .cc file) because it returns a DispatchTargetKey, which is a private type? If so, maybe put a comment? (In most cases, private statics are kind of a code smell)
Done
if (blocked_request.pending_targets.erase(target_key) == 0) {
// The target was already resolved. Several independent triggers can
// resolve a pending target, in any order:
// - Renderer completion signals: renderer-controlled IPCs, which a
// misbehaving renderer can send unsolicited.
// - `OnTargetCannotDispatch()`, which runs from a posted task.
// - Teardown of the target's renderer context.
// A trigger that runs after the target is resolved must do nothing; in
// particular, it must not decrement the block count again.
return;
}this... still seems like we should be able to be stricter here (e.g. maybe renderer kills).
1) Misbehaving renderer: should terminate the renderer
2) "CannotDispatch". This should only happen as a "second" signal if the renderer context was eliminated. We should never get a "cannot dispatch" for a live renderer as the second signal, since if we can't dispatch, we shouldn't get that first signal.
3) Renderer teardown should probably only enter here if the renderer is being tracked as a pending target.This is probably okay for this CL, but I wonder if we should add a TODO to tighten it up? WDYT?
Sounds good. I added a TODO.
Related details
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: extensions/browser/api/web_request/extension_web_request_event_router.h
Insertions: 3, Deletions: 0.
@@ -752,6 +752,9 @@
// Groups the matched `listeners` by their renderer dispatch target,
// producing exactly one entry per context.
+ //
+ // Static member rather than anonymous namespace helper because the signature
+ // names private nested types (`RawListeners`, `DispatchTargetKey`).
static std::map<DispatchTargetKey, RawListeners>
GroupListenersByDispatchTarget(const RawListeners& listeners);
```
```
The name of the file: extensions/browser/api/web_request/extension_web_request_event_router.cc
Insertions: 6, Deletions: 0.
@@ -2083,6 +2083,12 @@
// - Teardown of the target's renderer context.
// A trigger that runs after the target is resolved must do nothing; in
// particular, it must not decrement the block count again.
+ //
+ // TODO(crbug.com/494684626): CHECK the erase on the completion signal
+ // path. `OnEventHandlingDone()` resolves only targets it just found, and
+ // stale renderer signals fail that lookup. Only `OnTargetCannotDispatch()`
+ // needs this return. Killing renderers that send unsolicited signals would
+ // require some additional tracking on the browser side.
return;
}
DecrementBlockCount(browser_context, extension_id, event_name, request_id,
```
Change information
[Extensions] Resolve per-context webRequest blocking dispatch targets
Implements response resolution for per-context webRequest event
dispatch, completing the lifecycle for PendingTarget entries recorded on
a BlockedRequest.
During event handling, the renderer delivers responses in two stages:
1. Individual blocking listener responses arrive via
"webRequestInternal.eventHandled" as they do today. Each response is
clamped to the target's allowed extraInfoSpec bitmask and recorded as
an event delta.
2. Once all matching listeners in the context finish executing, a
single "webRequestInternal.eventHandlingDone" IPC resolves the
pending target, applies the collected deltas, and unblocks the
network request if no other blocking sources remain.
The renderer owns the completion signal by reporting "eventHandlingDone"
for every awaited dispatch it receives. The browser resolves a pending
target without applying response deltas only when the event cannot be
delivered at dispatch time (Event::cannot_dispatch_callback). A
follow-up will add resolution for targets whose renderer context is torn
down.
Additionally, refactors response delta recording into a reusable
AppendResponseDelta() helper shared between the legacy per-listener and
new per-context dispatch paths.
Note: This code is currently inactive until the per-context event
dispatch feature flag is enabled.
Design doc: go/no-webrequest-subevent
- M extensions/browser/api/web_request/extension_web_request_event_router.cc
- M extensions/browser/api/web_request/extension_web_request_event_router.h
- M extensions/browser/api/web_request/web_request_api.cc
Code-Review: +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |