DevTools: Implement waitForDebuggerOnStart for SharedWorker auto-attach [chromium/src : main]
Wang Neden (Gerrit)
Wang Neden voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
1 comment
Hi Alex,
Thanks for taking a look at this!
This CL fixes a race condition when using `Target.setAutoAttach` with `waitForDebuggerOnStart: true` for SharedWorkers. Currently, the CDP-injected script (e.g., from `Runtime.evaluate`) often executes *after* the worker's initial script, which is incorrect behavior.
The root cause is that, unlike `ServiceWorkerDevToolsManager`, the `SharedWorkerDevToolsManager` had no mechanism to notify the `BrowserAutoAttacher` about a new worker *before* it starts running. This prevented the attacher from setting the crucial `should_pause_on_start` flag in time.
To fix this, I've aligned the implementation with the established pattern in `ServiceWorkerDevToolsManager` by introducing a similar `Observer` interface. The `BrowserAutoAttacher` now observes `SharedWorkerDevToolsManager`, receives the `SharedWorkerCreated` notification, and can correctly set the pause flag to prevent the race condition.
Happy to address any feedback. Thanks
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
@dgo...@chromium.org Could you please review? I think I saw that you were working on a similar issue related to workers (or perhaps even the same underlying issue).
Related details
- Dmitry Gozman
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman added 3 comments![]( "Open in Gerrit")
Looks good overall, modulo legacy observers. +caseq@ FYI.
SharedWorkerDevToolsManager::GetInstance()->AddObserver(this);Let's use `ScopedObservation` instead. See https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/frame_auto_attacher.h;l=59-60;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 for an example.
base::ObserverList<Observer>::Unchecked observer_list_;Let's use `CheckedObserver` instead. See https://source.chromium.org/chromium/chromium/src/+/main:base/observer_list.h;l=29;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 for details.
Related details
- Andrey Kosyakov
- Wang Neden
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. |
Wang Neden (Gerrit)
Wang Neden voted and added 2 comments![]( "Open in Gerrit")
Votes added by Wang Neden
| Auto-Submit | +1 |
2 comments
SharedWorkerDevToolsManager::GetInstance()->AddObserver(this);Let's use `ScopedObservation` instead. See https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/frame_auto_attacher.h;l=59-60;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 for an example.
Hi Dmitry,
Thanks for the great suggestions! Using `ScopedObservation` and `CheckedObserver` is definitely a much cleaner and safer approach.
I've updated the patchset with these changes. The `BrowserAutoAttacher` now uses `ScopedObservation` to manage its observation of `SharedWorkerDevToolsManager`, and the observer itself is now a `CheckedObserver`.
While making these changes, I noticed that the existing observations for `ServiceWorkerDevToolsManager` and `DevToolsAgentHost` in the same class still use the manual `AddObserver`/`RemoveObserver` pattern. I'm happy to refactor them to also use `ScopedObservation` in a follow-up patch to improve consistency. I've kept this CL focused on the SharedWorker fix as discussed.
Please let me know what you think. Thanks
base::ObserverList<Observer>::Unchecked observer_list_;Let's use `CheckedObserver` instead. See https://source.chromium.org/chromium/chromium/src/+/main:base/observer_list.h;l=29;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 for details.
- Andrey Kosyakov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
SharedWorkerDevToolsManager::GetInstance()->AddObserver(this);Wang NedenLet's use `ScopedObservation` instead. See https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/frame_auto_attacher.h;l=59-60;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 for an example.
Hi Dmitry,
Thanks for the great suggestions! Using `ScopedObservation` and `CheckedObserver` is definitely a much cleaner and safer approach.
I've updated the patchset with these changes. The `BrowserAutoAttacher` now uses `ScopedObservation` to manage its observation of `SharedWorkerDevToolsManager`, and the observer itself is now a `CheckedObserver`.
While making these changes, I noticed that the existing observations for `ServiceWorkerDevToolsManager` and `DevToolsAgentHost` in the same class still use the manual `AddObserver`/`RemoveObserver` pattern. I'm happy to refactor them to also use `ScopedObservation` in a follow-up patch to improve consistency. I've kept this CL focused on the SharedWorker fix as discussed.
Please let me know what you think. Thanks
Hi Eden, I think it would be great to also change ServiceWorkerDevToolsManager and DevToolsAgentHost to also use ScopedObservation in a follow-up CL if you are interested in doing this.
Related details
- Andrey Kosyakov
- Dmitry Gozman
- Wang Neden
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang Neden (Gerrit)
Wang Neden added 1 comment![]( "Open in Gerrit")
SharedWorkerDevToolsManager::GetInstance()->AddObserver(this);Wang NedenLet's use `ScopedObservation` instead. See https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/frame_auto_attacher.h;l=59-60;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 for an example.
Alex RudenkoHi Dmitry,
Thanks for the great suggestions! Using `ScopedObservation` and `CheckedObserver` is definitely a much cleaner and safer approach.
I've updated the patchset with these changes. The `BrowserAutoAttacher` now uses `ScopedObservation` to manage its observation of `SharedWorkerDevToolsManager`, and the observer itself is now a `CheckedObserver`.
While making these changes, I noticed that the existing observations for `ServiceWorkerDevToolsManager` and `DevToolsAgentHost` in the same class still use the manual `AddObserver`/`RemoveObserver` pattern. I'm happy to refactor them to also use `ScopedObservation` in a follow-up patch to improve consistency. I've kept this CL focused on the SharedWorker fix as discussed.
Please let me know what you think. Thanks
Hi Eden, I think it would be great to also change ServiceWorkerDevToolsManager and DevToolsAgentHost to also use ScopedObservation in a follow-up CL if you are interested in doing this.
Great! I'll create the follow-up CL for the refactoring once this one lands.
Thanks!
Related details
- Andrey Kosyakov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |