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 upEnsure documents and fetch cancellers drop #24047
Conversation
highfive
commented
Aug 25, 2019
|
Heads up! This PR modifies the following files:
|
highfive
commented
Aug 25, 2019
|
|
|
@jdm @asajeffrey r? |
gterzian
force-pushed the
gterzian:ensure_documents_and_fetch_cancellers_drop
branch
from
aba3e07
to
cb64339
|
@bors-servo try=wpt |
…_drop, r=<try> Ensure documents and fetch cancellers drop <!-- Please describe your changes on the following line: --> Related to #23905 (comment) and #23905 (comment) --- <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24047) <!-- Reviewable:end -->
|
|
|
Opened new PR for upstreamable changes. Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#18661. |
gterzian
force-pushed the
gterzian:ensure_documents_and_fetch_cancellers_drop
branch
from
daa1bc3
to
c823074
|
No upstreamable changes; closed existing PR. Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#18661. |
|
@bors-servo try=wpt |
…_drop, r=<try> Ensure documents and fetch cancellers drop <!-- Please describe your changes on the following line: --> Related to #23905 (comment) and #23905 (comment) --- <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24047) <!-- Reviewable:end -->
|
|
gterzian
force-pushed the
gterzian:ensure_documents_and_fetch_cancellers_drop
branch
5 times, most recently
from
7381c21
to
d4b0541
|
@bors-servo try=wpt |
|
@bors-servo try=wpt (fixed the double borrow on |
…_drop, r=<try> Ensure documents and fetch cancellers drop <!-- Please describe your changes on the following line: --> Related to #23905 (comment) and #23905 (comment) --- <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24047) <!-- Reviewable:end -->
|
Ok so this one still crashes: and I think the right thing might be to mark as is a @jdm what do you think? |
|
That depends on what the rest of the crash output looks like. |
|
|
Ok, that test is quite new, and I don't see a clear relationship to these changes that would affect its behaviour. Changing its expected result to CRASH seems reasonable to me. |
Since without these changes we would leak the globalscope, I can imagine the crash is a result of the global scope dropping before the blob, due to the current changes. The test calls The fix for this is to run the clean-up steps for the blob not when it drops, but when the document is is associated is unloaded, which ensures the global is alive when the blob clean-up steps are run(and it the specced behavior). By the way I thought you had opened #24052 due to the test result in this PR. Did you see the crash elsewhere? In that case it might not be related... |
gterzian
force-pushed the
gterzian:ensure_documents_and_fetch_cancellers_drop
branch
from
1ec46e2
to
fe6148c
|
I may indeed have opened that issue due to seeing the test failure in this PR. |
gterzian
force-pushed the
gterzian:ensure_documents_and_fetch_cancellers_drop
branch
3 times, most recently
from
0f940a1
to
9009bef
|
I'm kind of queasy about this approach, we're adding a lot of potential panics, and I'm not sure what the semantics of zombie windows is meant to be. |
| /// A window without a browsing-context, | ||
| /// keeping only its associated document, since it should always be kept. | ||
| /// See https://html.spec.whatwg.org/multipage/#concept-document-window | ||
| Zombie(MutNullableDom<Document>), |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Aug 26, 2019
Member
Not sure if we need to keep the document, if this Window is unreachable from script.
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
•
Author
Member
I added the document to the zombie state, because without it this test started failing: /html/browsers/the-window-object/document-attribute.window.html. The reason for the failure is that if you do frame.remove(); the browsing-context is discarded, and in servo the pipeline is closed, however the window/document are still potentially reachable from script.
| Alive, | ||
| Zombie, // Pipeline is closed, but the window hasn't been GCed yet. | ||
| /// A window with a browsing-context. | ||
| Alive(WindowStateData), |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Aug 26, 2019
Member
Is there a spec name for this? Discarded? Perhaps Reachable vs Unreachable? Or ReachableFromJS?
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
•
Author
Member
We could go for BrowsingContextDiscarded and WithBrowsingContext, because the switch really occurs as part of https://html.spec.whatwg.org/multipage/window-object.html#garbage-collection-and-browsing-contexts and the window and document could still be reachable from script afterwards.
| @@ -137,10 +137,15 @@ use webrender_api::{DocumentId, ExternalScrollId, RenderApiSender}; | |||
| use webvr_traits::WebVRMsg; | |||
|
|
|||
| /// Current state of the window object | |||
| #[derive(Clone, Copy, Debug, JSTraceable, MallocSizeOf, PartialEq)] | |||
| #[derive(JSTraceable, MallocSizeOf)] | |||
| #[must_root] | |||
| enum WindowState { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -377,7 +408,12 @@ impl Window { | |||
|
|
|||
| #[allow(unsafe_code)] | |||
| pub fn get_cx(&self) -> JSContext { | |||
| unsafe { JSContext::from_ptr(self.js_runtime.borrow().as_ref().unwrap().cx()) } | |||
| match &*self.current_state.borrow_mut() { | |||
| WindowState::Zombie(_) => unreachable!("Window should be alive when get_cx is called"), | |||
This comment has been minimized.
This comment has been minimized.
asajeffrey
Aug 26, 2019
Member
unreachable! is for code that really really is unreachable, e.g. the method that triggers it is private. It's not intended for this sort of thing where it should really be a panic!
This comment has been minimized.
This comment has been minimized.
| // In the Zombie case, create an object for script, but don't store it. | ||
| // Usage of the object should throw security errors, | ||
| // since there is no associated document or browsing-context. | ||
| WindowState::Zombie(_) => History::new(self), |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Aug 26, 2019
Member
Not sure this is the right behaviour, aren't zombie windows meant to be unreachable from script?
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
Author
Member
They can be reachable, as exemplified by at least one test at /html/browsers/the-window-object/document-attribute.window.html.
The history interface throws a security error if used from a not fully active document(which would include one whose BC has been discarded), see https://html.spec.whatwg.org/multipage/history.html#the-history-interface:fully-active-2
| // In the Zombie case, create an object for script, but don't store it. | ||
| // Usage of the object should throw security errors, | ||
| // since there is no associated document or browsing-context. | ||
| WindowState::Zombie(_) => CustomElementRegistry::new(self), |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
•
Author
Member
The comment here looks wrong, since it seems the registry does not have a concept of throwing errors when used after the BC as been discarded. However it could still be reachable from script even in the zombie state.
The idea is that if we still make the interface available to script, but don't store it, we can be sure the Rust part isn't keeping the window alive for longer than it would be used from script, including via potential references to the window from these various interfaces.
Some of these interfaces take the discarding of browsing context into account, and some do not, and I wonder whether they all implicitly are only usable when the browsing-context has not been discarded.
| // In the Zombie case, create an object for script, but don't store it. | ||
| // Usage of the object should throw security errors, | ||
| // since there is no associated document or browsing-context. | ||
| WindowState::Zombie(_) => Location::new(self), |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
•
Author
Member
The location interface uses a concept of a relevant-document, and sometimes throws a security error or simply returns when the interface is used when the relevant document is null(and the "relevant document" is the active doc of the BC, so null in zombie state).
| nodes.push(Dom::from_ref(&*node)); | ||
| match &*self.current_state.borrow_mut() { | ||
| WindowState::Zombie(_) => { | ||
| unreachable!("Window should be alive when force_reflow is called") |
This comment has been minimized.
This comment has been minimized.
asajeffrey
Aug 26, 2019
Member
Are we really sure that there's no race condition between script, layout, the compositor, etc that can trigger this?
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
•
Author
Member
I'm not sure, and I think a panic is probably the right way to become aware of these. I do think any reflow after the browsing-context has been discarded, and the pipeline closed(and the layout thread exited), is likely to be wrong and would require fixing.
This comment has been minimized.
This comment has been minimized.
gterzian
Aug 27, 2019
•
Author
Member
Also, a reflow on a zombie window would already panic on the send to the layout chan above, since the pipeline would already have been closed.
Such a panic due to reflow on a closed pipeline has already been noted, for example in #19170
gterzian
force-pushed the
gterzian:ensure_documents_and_fetch_cancellers_drop
branch
from
9009bef
to
581a5a2
|
I'll see if I can find which |
|
@jdm @asajeffrey There is a minimalist alternative open at #24072 It does the trick, although I can't make out why |
Ensure documents drop when a pipeline exits <!-- Please describe your changes on the following line: --> minimalist companion/alternative to #24047 --- <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24072) <!-- Reviewable:end -->
Ensure documents drop when a pipeline exits <!-- Please describe your changes on the following line: --> minimalist companion/alternative to #24047 --- <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24072) <!-- Reviewable:end -->
Ensure documents drop when a pipeline exits <!-- Please describe your changes on the following line: --> minimalist companion/alternative to #24047 --- <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24072) <!-- Reviewable:end -->
|
|
|
In favor of #24072 |
gterzian commentedAug 25, 2019
•
edited by SimonSapin
Related to #23905 (comment) and #23905 (comment)
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is