mirror of
https://github.com/lightpanda-io/browser.git
synced 2026-03-21 20:24:42 +00:00
Remove redundant CDP v8 shutdown
https://github.com/lightpanda-io/browser/pull/1614 improved our shutdown behavior so that microtasks associated with a context wouldn't fire after the context was disposed of. This involved having context-specific microtasks, pumping the message loop, and prevent re-entry. The shutdown code in CDP already had much of this behavior built-in, but it has now become redundant. Most importantly the CDP shutdown logic did not prevent re-entry. Removing this code fixes a flaky WPT crash. I didn't seem to be tied to a specific test, but rather a cross-context/page use-after-free that was saw prior to 1614. I could reproduce it reliably by running `/wasm/core/`. I'll be honest, it isn't clear to me why _removing_ the CDP cleanup helps. Running the message loop and microtask _before_ our normal shutdown might be unnecessary, but why would it crash? I don't know, but the CDP path is slightly different in that it also involves Inspector shutdown. So there's still something about this flow I don't quite understand. And, at least for this case the current flow seems "correct".
This commit is contained in:
@@ -438,15 +438,10 @@ pub fn BrowserContext(comptime CDP_T: type) type {
|
||||
const browser = &self.cdp.browser;
|
||||
const env = &browser.env;
|
||||
|
||||
// Drain microtasks makes sure we don't have inspector's callback
|
||||
// in progress before deinit.
|
||||
env.runMicrotasks();
|
||||
|
||||
// resetContextGroup detach the inspector from all contexts.
|
||||
// It append async tasks, so we make sure we run the message loop
|
||||
// It appends async tasks, so we make sure we run the message loop
|
||||
// before deinit it.
|
||||
env.inspector.?.resetContextGroup();
|
||||
_ = env.pumpMessageLoop();
|
||||
env.inspector.?.stopSession();
|
||||
|
||||
// abort all intercepted requests before closing the sesion/page
|
||||
|
||||
Reference in New Issue
Block a user