Skip to content
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

Support performing generic operations on DOM objects during document teardown #24483

Open
jdm opened this issue Oct 17, 2019 · 1 comment
Open

Support performing generic operations on DOM objects during document teardown #24483

jdm opened this issue Oct 17, 2019 · 1 comment

Comments

@jdm
Copy link
Member

@jdm jdm commented Oct 17, 2019

For problems like #23986 and #24052, we have code that currently runs in DOM objects' Drop method because that's the easiest way to deal with "something needs to be unregistered when the DOM object no longer exists". Destructors interact poorly with the GC and can operate in invalid or unexpected states, so we should add a way for DOM objects to opt-in to some kind of cleanup action when a global is being torn down (

pub fn clear_js_runtime(&self) {
). We could store a list of weak references and invoke a closure with the strong reference if it still exists. However, we will still want to support running the cleanup if the object is GCed before document teardown. It would be best to have a special DomCleanup type that can be stored inside an Option inside the object that requires cleanup and is responsible for tracking all important cleanup state and destroying itself when appropriate, so that there is no special Drop implementation needed for any DOM objects.

@gterzian
Copy link
Member

@gterzian gterzian commented Oct 18, 2019

Could you elaborate on how you envision the proposed DomCleanup object?


Personally I liked the pattern used in #23637 (and also starting to be applied in #24123) where one would:

  1. split up the DOM struct from a non-DOM implementation struct(especially relevant for objects that should be structured serializable or transferable, since it's much harder to do so with a singular DOM struct).
  2. Store(on the GlobalScope) a strong ref to the non-DOM implementation.
  3. Store only a weak ref to the DOM struct.
  4. Run a periodic check as part of the script event-loop, where you clean-up the non-DOM object if the weakref to the corresponding DOM struct has been GC'ed.
  5. When all objects have been cleaned-up, clean-up any "meta" objects used to manage the whole thing, which could include sending a message to other components to let them know you're not managing any of a type of object anymore(see for MessagePort, where a "router" is used by each global managing ports, and the constellation also keeps a reference to that router, hence when the global is done managing ports, it let's the constellation know to drop the corresponding sender(here).

The above might appear like overkill for most DOM objects(and there might be a more generic way to do this), and it might actually make sense for DOM objects that involve some communication with non-script components, including clean-ups.

For example in the case of XR, besides (un-)registering itself with the webvr_thread, it also communicates with the webxr registry for example at SupportsSession and RequestSession, each time using a IPC ROUTER, and using the globalscope(to access various senders to other components).

So one can imagine making XR a slimmer DOM struct, and having some sort of XrManager stored on the global, which would be responsible for the communication with non-script components, as well as keeping track of outstanding XR via weakrefs. Then clean-up could be done when the weakrefs are gone, or when the page unloads, or the pipeline exits.

Also note that the HTML spec has a hook to define specific clean-up steps triggered as part of document unloading, which for example the File API is using. So it might make sense to also define such unloading-document-cleanup-steps for WebXr, if is hasn't been done already.

@gterzian gterzian mentioned this issue Oct 20, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.