[Extensions] Make cannot_dispatch_callback invocation more robust [chromium/src : main]
Tal Keren (Gerrit)
Tal Keren added 1 comment![]( "Open in Gerrit")
Hey, the context for this fix:
https://issues.chromium.org/issues/467448815#comment34
Related details
- Andrea Orru
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru added 3 comments![]( "Open in Gerrit")
Hey, the context for this fix:
https://issues.chromium.org/issues/467448815#comment34
Thank you!
event_router()->DispatchEventToExtension(ext1, std::move(event));Uhm... If this case is not properly handling, do you know why [this test](https://crsrc.org/c/chrome/browser/extensions/api/web_request/web_request_apitest.cc;drc=2619b946b249d21cc1f2f1e08edcec3ecb8fdc4e;l=6927) is not failing?
Related details
- Devlin Cronin
- Tal Keren
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")
event_router()->DispatchEventToExtension(ext1, std::move(event));Uhm... If this case is not properly handling, do you know why [this test](https://crsrc.org/c/chrome/browser/extensions/api/web_request/web_request_apitest.cc;drc=2619b946b249d21cc1f2f1e08edcec3ecb8fdc4e;l=6927) is not failing?
Oh I see. In the case you're covering, there's no lazy listener either...?
Tal Keren (Gerrit)
Tal Keren added 1 comment![]( "Open in Gerrit")
event_router()->DispatchEventToExtension(ext1, std::move(event));Andrea OrruUhm... If this case is not properly handling, do you know why [this test](https://crsrc.org/c/chrome/browser/extensions/api/web_request/web_request_apitest.cc;drc=2619b946b249d21cc1f2f1e08edcec3ecb8fdc4e;l=6927) is not failing?
Oh I see. In the case you're covering, there's no lazy listener either...?
Yes, the case that I am covering is when webRequest doesn't have a listener or a lazy listener, but the caller of the API think that there might be (in this case WebRequestEventRouter). This inconsistency might be problematic on its own but is resolved by enabling WebRequestPersistFilteredEventsViaEventRouter. It is a bit hard to fully simulate with webRequest because it is a race (needs to unregister an event listener while profile is marked as shutting down). This issue happened to us frequently because how our service worker is constructed, so this was my solution for it, which I think that should be the expected behavior anyway for callers that call any api that uses DispatchEventImpl with cannot_dispatch_callback even if the specific issue with WebRequestEventRouter won't exists soon.
Related details
- Andrea Orru
- 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. |
Tal Keren (Gerrit)
Tal Keren added 1 comment![]( "Open in Gerrit")
event_router()->DispatchEventToExtension(ext1, std::move(event));Andrea OrruUhm... If this case is not properly handling, do you know why [this test](https://crsrc.org/c/chrome/browser/extensions/api/web_request/web_request_apitest.cc;drc=2619b946b249d21cc1f2f1e08edcec3ecb8fdc4e;l=6927) is not failing?
Tal KerenOh I see. In the case you're covering, there's no lazy listener either...?
Yes, the case that I am covering is when webRequest doesn't have a listener or a lazy listener, but the caller of the API think that there might be (in this case WebRequestEventRouter). This inconsistency might be problematic on its own but is resolved by enabling WebRequestPersistFilteredEventsViaEventRouter. It is a bit hard to fully simulate with webRequest because it is a race (needs to unregister an event listener while profile is marked as shutting down). This issue happened to us frequently because how our service worker is constructed, so this was my solution for it, which I think that should be the expected behavior anyway for callers that call any api that uses DispatchEventImpl with cannot_dispatch_callback even if the specific issue with WebRequestEventRouter won't exists soon.
It will behave more like EventRouter::DispatchPendingEvent and will call cannot_dispatch_callback for any failure reason.
Tal Keren (Gerrit)
Tal Keren added 1 comment![]( "Open in Gerrit")
Reference the bug.
`Bug: 467448815, 484218883`
Acknowledged
Related details
- Andrea Orru
- 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 6 comments![]( "Open in Gerrit")
Votes added by Andrea Orru
| Code-Review | +1 |
6 comments
Thank you Tal! This is a really good patch. Just a few comments.
cases where a dispatch is undeliverable for other reasons, such as whenCan you expand on what the reasons are? As far as I can tell:
- Events with zero or blocked listeners are silently swallowed.
- During broadcasts, failures in one extension can incorrectly abort successful WebRequest blocking requests from another.
WebRequestEventRouter was one manifestation of this problem.Can you explain the nature of the inconsistency?
undeliverable-dispatch fallback more robustPeriod at the end of the sentence.
event_router()->DispatchEventToExtension(ext1, std::move(event));Andrea OrruUhm... If this case is not properly handling, do you know why [this test](https://crsrc.org/c/chrome/browser/extensions/api/web_request/web_request_apitest.cc;drc=2619b946b249d21cc1f2f1e08edcec3ecb8fdc4e;l=6927) is not failing?
Tal KerenOh I see. In the case you're covering, there's no lazy listener either...?
Tal KerenYes, the case that I am covering is when webRequest doesn't have a listener or a lazy listener, but the caller of the API think that there might be (in this case WebRequestEventRouter). This inconsistency might be problematic on its own but is resolved by enabling WebRequestPersistFilteredEventsViaEventRouter. It is a bit hard to fully simulate with webRequest because it is a race (needs to unregister an event listener while profile is marked as shutting down). This issue happened to us frequently because how our service worker is constructed, so this was my solution for it, which I think that should be the expected behavior anyway for callers that call any api that uses DispatchEventImpl with cannot_dispatch_callback even if the specific issue with WebRequestEventRouter won't exists soon.
It will behave more like EventRouter::DispatchPendingEvent and will call cannot_dispatch_callback for any failure reason.
Acknowledged
Related details
- Devlin Cronin
- Tal Keren
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")
Moving Devlin to cc since he's traveling, adding Emilia.
Related details
- Devlin Cronin
- Emilia Paz
- Tal Keren
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. |
Tal Keren (Gerrit)
Tal Keren added 4 comments![]( "Open in Gerrit")
cases where a dispatch is undeliverable for other reasons, such as whenCan you expand on what the reasons are? As far as I can tell:
- Events with zero or blocked listeners are silently swallowed.
- During broadcasts, failures in one extension can incorrectly abort successful WebRequest blocking requests from another.
I have updated the message.
But about the second point - it is a good point, didn't think about it, but it is actually divergent from EventRouter::DispatchPendingEvent. With broadcast and cannot_dispatch_callback - if any context is inactive at time of dispatch, EventRouter::DispatchPendingEvent is not aware about other potential listeners.
so I don't think that this callback can work properly with broadcast anyway.
it also made me look up how this callback is documenteed:
// Called if the event cannot be dispatched to a lazy listener. This happens
// if e.g. the extension registers an event listener from a lazy context
// asynchronously, which results in the active listener not being registered
// at the time the lazy context is spun back up.
CannotDispatchCallback cannot_dispatch_callback;
I wonder if it should be updated in any way.
WebRequestEventRouter was one manifestation of this problem.Can you explain the nature of the inconsistency?
Done
Period at the end of the sentence.
Done
nit: newline above this return
- Andrea Orru
- Devlin Cronin
- Emilia Paz
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
| Code-Review | +1 |
2 comments
cases where a dispatch is undeliverable for other reasons, such as whenTal KerenCan you expand on what the reasons are? As far as I can tell:
- Events with zero or blocked listeners are silently swallowed.
- During broadcasts, failures in one extension can incorrectly abort successful WebRequest blocking requests from another.
I have updated the message.
But about the second point - it is a good point, didn't think about it, but it is actually divergent from EventRouter::DispatchPendingEvent. With broadcast and cannot_dispatch_callback - if any context is inactive at time of dispatch, EventRouter::DispatchPendingEvent is not aware about other potential listeners.
so I don't think that this callback can work properly with broadcast anyway.it also made me look up how this callback is documenteed:
// Called if the event cannot be dispatched to a lazy listener. This happens
// if e.g. the extension registers an event listener from a lazy context
// asynchronously, which results in the active listener not being registered
// at the time the lazy context is spun back up.
CannotDispatchCallback cannot_dispatch_callback;
I wonder if it should be updated in any way.
I was talking about the scenario you are testing in the second unit test.
return false;nit: newline above this return
- Devlin Cronin
- Emilia Paz
- Tal Keren
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. |
Tal Keren (Gerrit)
Tal Keren added 1 comment![]( "Open in Gerrit")
cases where a dispatch is undeliverable for other reasons, such as whenTal KerenCan you expand on what the reasons are? As far as I can tell:
- Events with zero or blocked listeners are silently swallowed.
- During broadcasts, failures in one extension can incorrectly abort successful WebRequest blocking requests from another.
Andrea OrruI have updated the message.
But about the second point - it is a good point, didn't think about it, but it is actually divergent from EventRouter::DispatchPendingEvent. With broadcast and cannot_dispatch_callback - if any context is inactive at time of dispatch, EventRouter::DispatchPendingEvent is not aware about other potential listeners.
so I don't think that this callback can work properly with broadcast anyway.it also made me look up how this callback is documenteed:
// Called if the event cannot be dispatched to a lazy listener. This happens
// if e.g. the extension registers an event listener from a lazy context
// asynchronously, which results in the active listener not being registered
// at the time the lazy context is spun back up.
CannotDispatchCallback cannot_dispatch_callback;
I wonder if it should be updated in any way.
I was talking about the scenario you are testing in the second unit test.
Oh, I thought about BroadastEvent for a moment, but the helper doesn't even have that functionality. so nvm.
About the second scenario, I think that "when all candidate listeners are filtered out by the target extension." should cover it.
Related details
- Andrea Orru
- Devlin Cronin
- Emilia Paz
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")
cases where a dispatch is undeliverable for other reasons, such as whenTal KerenCan you expand on what the reasons are? As far as I can tell:
- Events with zero or blocked listeners are silently swallowed.
- During broadcasts, failures in one extension can incorrectly abort successful WebRequest blocking requests from another.
Andrea OrruI have updated the message.
But about the second point - it is a good point, didn't think about it, but it is actually divergent from EventRouter::DispatchPendingEvent. With broadcast and cannot_dispatch_callback - if any context is inactive at time of dispatch, EventRouter::DispatchPendingEvent is not aware about other potential listeners.
so I don't think that this callback can work properly with broadcast anyway.it also made me look up how this callback is documenteed:
// Called if the event cannot be dispatched to a lazy listener. This happens
// if e.g. the extension registers an event listener from a lazy context
// asynchronously, which results in the active listener not being registered
// at the time the lazy context is spun back up.
CannotDispatchCallback cannot_dispatch_callback;
I wonder if it should be updated in any way.
Tal KerenI was talking about the scenario you are testing in the second unit test.
Oh, I thought about BroadastEvent for a moment, but the helper doesn't even have that functionality. so nvm.
About the second scenario, I think that "when all candidate listeners are filtered out by the target extension." should cover it.
- Devlin Cronin
- Emilia Paz
- Tal Keren
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)
```
CannotDispatchCallback cannot_dispatch_callback;
```
As you mentioned would be good to list the reasons that we know when this is called in the documentation comment.
Everything else looks good to me.
Emilia Paz (Gerrit)
Emilia Paz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
1 comment
Overall lgtm, thanks Tal! Leaving the open comments to Andrea
Related details
- Devlin Cronin
- Tal Keren
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. |
Tal Keren (Gerrit)
Tal Keren added 1 comment![]( "Open in Gerrit")
cases where a dispatch is undeliverable for other reasons, such as when- Andrea Orru
- Devlin Cronin
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru voted and added 1 comment![]( "Open in Gerrit")
- Devlin Cronin
- Emilia Paz
- Tal Keren
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |