Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix messageport "GC'ed too early" #25461
Closed
Labels
Comments
|
See also #24810 (comment)
This might be an alternative to simply leaking the port inside a |
This was referenced Feb 15, 2020
bors-servo
added a commit
that referenced
this issue
Feb 15, 2020
[WIP Leak message ports in Dom, until they are closed <!-- Please describe your changes on the following line: --> FIX #25461 FIX #24810 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Feb 15, 2020
[WIP Leak message ports in Dom, until they are closed <!-- Please describe your changes on the following line: --> FIX #25461 FIX #24810 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Feb 15, 2020
[WIP Leak message ports in Dom, until they are closed <!-- Please describe your changes on the following line: --> FIX #25461 FIX #24810 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Feb 15, 2020
Leak message ports in Dom, until they are closed <!-- Please describe your changes on the following line: --> FIX #25461 FIX #24810 FIX #24488 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Feb 21, 2020
Leak message ports in Dom, until they are closed <!-- Please describe your changes on the following line: --> FIX #25461 FIX #24810 FIX #24488 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Part of #25208, with regards to
I'm thinking we might want to switch to the "leak memory" strategy, where we would hold a
Dom<MessagePort>instead of aWeakinsideManagedMessagePortservo/components/script/dom/globalscope.rs
Line 256 in 709e069
Then we could remove those when
closeis called on a port, as opposed to rely on the GC mechanism of Spidermonkey.I haven't been able to diagnose what exactly triggers the "early GC" in the two issues mentioned, and I think for now we can simply assume that we cannot rely on SM to fully manage the lifetimes of ports.
Instead, so that it at least "works" when for example browsing Spotify, we could just leak them inside a
Dom, and only drop them if the JS actually callsClose, or if the page is navigated away from.Then later we could try to come-up with our own heuristics to GC ports that are logically unreachable.
The spec has a note on the topic: https://html.spec.whatwg.org/multipage/#ports-and-garbage-collection