-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement browsing context discarding #14559
Implement browsing context discarding #14559
Conversation
Heads up! This PR modifies the following files:
|
This should help with the crash we were seeing in #13996 (comment) (though that crash has mysteriously disappeared). cc @Ms2ger @jdm |
Also see #14434 |
☔ The latest upstream changes (presumably #14557) made this pull request unmergeable. Please resolve the merge conflicts. |
9d314d3
to
7f5f9d6
Compare
☔ The latest upstream changes (presumably #14412) made this pull request unmergeable. Please resolve the merge conflicts. |
7f5f9d6
to
8be8c54
Compare
☔ The latest upstream changes (presumably #14260) made this pull request unmergeable. Please resolve the merge conflicts. |
8be8c54
to
c4d1813
Compare
Rebased. r? @ConnorGBrewster |
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.
Looks good! Although it looks like removing the reference to the active document in the browsing context caused a couple of regressions. I think we should find a way to fix those before merging.
Also since the BS doesn't store the active document, we will need to find a way to determine if the document is active
vs fully_active
.
@@ -3190,8 +3187,8 @@ impl DocumentMethods for Document { | |||
|
|||
// Step 2. | |||
// TODO: handle throw-on-dynamic-markup-insertion counter. | |||
|
|||
if !self.is_active() { | |||
// FIXME: this should check for being active rather than fully active |
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.
Should we implement is_active
again before merging this?
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.
The problem is that we're currently not sending enough information from the constellation to the script thread to determine if a document is active. We're sending freeze/thaw messages to determine if a document is fully active, but not any messages about being active. I think this is a spec issue, the spec should really say "fully active" here.
@@ -2304,7 +2295,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF> | |||
} | |||
} | |||
for entry in evicted_pipelines { | |||
self.close_pipeline(entry.pipeline_id, ExitPipelineMode::Normal); | |||
self.close_pipeline(entry.pipeline_id, DiscardBrowsingContext::No, ExitPipelineMode::Normal); |
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.
Why do we not discard the Browsing Context here?
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 code is used when a browsing context navigates, we don't want to close the BC in this case. Nested BCs will get closed, when the recursive call is made it's with DiscardBrowsingContext::Yes
.
} | ||
|
||
pub fn active_document(&self) -> Root<Document> { |
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.
What are the reasons behind not storing a reference to the Document in the BC?
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.
We never needed it, so I removed it! SpiderMonkey does the dance to resolve WindowProxy
to Window
, which tracks the current document.
} | ||
// TODO Step 3. |
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.
I think we should find a way to do this, and other cases that may have regressed with this PR before merging.
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.
I think it's broken in the same way as it was before. Did this cause a regression?
@@ -1345,6 +1357,13 @@ impl Window { | |||
unsafe { SetWindowProxy(cx, window, browsing_context.reflector().get_jsobject()); } | |||
} | |||
|
|||
#[allow(unsafe_code)] |
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.
Is this needed?
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.
Good catch.
@@ -183,6 +183,15 @@ pub struct NewLayoutInfo { | |||
pub layout_threads: usize, | |||
} | |||
|
|||
/// When a pipeline is closed, should its browsing context be discarded too? |
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.
Maybe add a spec link here that discussing when to discard browsing contexts?
☔ The latest upstream changes (presumably #14623) made this pull request unmergeable. Please resolve the merge conflicts. |
e3788d8
to
7c2de62
Compare
Rebased. |
@bors-servo try |
…ding, r=<try> Implement browsing context discarding <!-- Please describe your changes on the following line: --> Implement browsing context discarding (https://html.spec.whatwg.org/multipage/browsers.html#discard-a-document). * When a pipeline is closed, inform the script thread whether the browsing context is to be discarded. * In script threads, synchronously discard any similar-origin documents and browsing contexts. * When a browsing context is discarded, it loses the reference to the active document, but the window keeps it, so we need to move the `Document` pointer from `BrowsingContext` to `Window`. * Fix the webIDL for Window to make parent and top optional (the spec says they can return null when the browsing context is discarded). --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14411 - [X] There are tests for these changes <!-- 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/14559) <!-- Reviewable:end -->
💔 Test failed - linux-rel-wpt |
@bors-servo: retry |
…ding, r=<try> Implement browsing context discarding <!-- Please describe your changes on the following line: --> Implement browsing context discarding (https://html.spec.whatwg.org/multipage/browsers.html#discard-a-document). * When a pipeline is closed, inform the script thread whether the browsing context is to be discarded. * In script threads, synchronously discard any similar-origin documents and browsing contexts. * When a browsing context is discarded, it loses the reference to the active document, but the window keeps it, so we need to move the `Document` pointer from `BrowsingContext` to `Window`. * Fix the webIDL for Window to make parent and top optional (the spec says they can return null when the browsing context is discarded). --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14411 - [X] There are tests for these changes <!-- 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/14559) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
@bors-servo r+ retry |
📌 Commit 7c2de62 has been approved by |
…ding, r=cbrewster Implement browsing context discarding <!-- Please describe your changes on the following line: --> Implement browsing context discarding (https://html.spec.whatwg.org/multipage/browsers.html#discard-a-document). * When a pipeline is closed, inform the script thread whether the browsing context is to be discarded. * In script threads, synchronously discard any similar-origin documents and browsing contexts. * When a browsing context is discarded, it loses the reference to the active document, but the window keeps it, so we need to move the `Document` pointer from `BrowsingContext` to `Window`. * Fix the webIDL for Window to make parent and top optional (the spec says they can return null when the browsing context is discarded). --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14411 - [X] There are tests for these changes <!-- 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/14559) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
Implement browsing context discarding (https://html.spec.whatwg.org/multipage/browsers.html#discard-a-document).
Document
pointer fromBrowsingContext
toWindow
../mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is