-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(scopes): make page a scope #4300
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, it seems like the PR is actually about a convenient parent
for each object, not about the scopes. What if we actually send whatever parent we want, but only dispose the BrowserContext and Browser explicitly?
I see three issues with this particular implementation:
- the "no channels in initializers" check (see the separate comment);
- all frames will outlive the page and won't be disposed - that means we don't actually clean up any resources on page close;
- methods like
Download.path
will return a wrong errorTarget browser or context has been closed
when in fact it was the page that was closed, while the context is still alive.
if (!payload) | ||
return payload; | ||
if (payload instanceof Dispatcher) { | ||
if (!allowDispatchers) | ||
throw new Error(`Channels are not allowed in the scope's initialzier`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular check was a result of the following problem:
BrowserContext had pages
in the initializer, and since we send channels from the initializer before the object, these pages were created in the parent scope (the Browser), rather than BrowserContext's scope. This lead to leaks where pages were not cleaned up upon BrowserContext's dispose. I am afraid this could happen again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an edge case, let's add a test instead?
My understanding is that in this case we clean up:
|
That's what happens today as well, so it isn't changing. Stale pointers already have undefined behavior anyways. |
b541e50
to
61ccde3
Compare
61ccde3
to
d7df332
Compare
Although we end up with a structure where Frame is on the same level as the page, it is very convenient to attribute console messages, dialogs, web sockets to the page rather than the browser context. We get a lot of
page()
methods for free on those.