Implement browsing context discarding by asajeffrey · Pull Request #14559 · servo/servo

asajeffrey · 2016-12-12T22:16:15Z

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).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Discard documents and browsing contexts synchronously #14411
There are tests for these changes

This change is

highfive · 2016-12-12T22:16:18Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/dom/browsingcontext.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script_traits/lib.rs, components/script_traits/lib.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/dom/browsingcontext.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script_traits/lib.rs, components/script_traits/lib.rs

asajeffrey · 2016-12-12T22:17:38Z

This should help with the crash we were seeing in #13996 (comment) (though that crash has mysteriously disappeared). cc @Ms2ger @jdm

asajeffrey · 2016-12-12T22:19:04Z

Also see #14434

bors-servo · 2016-12-13T06:09:40Z

☔ The latest upstream changes (presumably #14557) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-12-15T03:42:58Z

☔ The latest upstream changes (presumably #14412) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-12-15T23:56:13Z

☔ The latest upstream changes (presumably #14260) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2016-12-23T15:10:13Z

Rebased. r? @ConnorGBrewster

cbrewster

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.

cbrewster · 2016-12-23T16:20:24Z

components/script/dom/document.rs

        // TODO: handle throw-on-dynamic-markup-insertion counter.
-
-        if !self.is_active() {
+        // FIXME: this should check for being active rather than fully active


Should we implement is_active again before merging this?

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.

cbrewster · 2016-12-23T17:10:11Z

components/constellation/constellation.rs

        }
        for entry in evicted_pipelines {
-            self.close_pipeline(entry.pipeline_id, ExitPipelineMode::Normal);
+            self.close_pipeline(entry.pipeline_id, DiscardBrowsingContext::No, ExitPipelineMode::Normal);


Why do we not discard the Browsing Context here?

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.

cbrewster · 2016-12-23T17:23:21Z

components/script/dom/browsingcontext.rs

+        self.discarded.set(true);
    }

-    pub fn active_document(&self) -> Root<Document> {


What are the reasons behind not storing a reference to the Document in the BC?

We never needed it, so I removed it! SpiderMonkey does the dance to resolve WindowProxy to Window, which tracks the current document.

cbrewster · 2016-12-23T17:25:53Z

components/script/dom/document.rs

+        if self.window().parent_info().is_none() && self.is_fully_active() {
+            return true;
        }
+        // TODO Step 3.


I think we should find a way to do this, and other cases that may have regressed with this PR before merging.

I think it's broken in the same way as it was before. Did this cause a regression?

cbrewster · 2016-12-23T17:27:12Z

components/script/dom/window.rs

        unsafe { SetWindowProxy(cx, window, browsing_context.reflector().get_jsobject()); }
    }

+    #[allow(unsafe_code)]


Is this needed?

Good catch.

cbrewster · 2016-12-23T17:28:21Z

components/script_traits/lib.rs

    pub layout_threads: usize,
 }

+/// When a pipeline is closed, should its browsing context be discarded too?


Maybe add a spec link here that discussing when to discard browsing contexts?

bors-servo · 2016-12-24T12:38:46Z

☔ The latest upstream changes (presumably #14623) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2017-01-05T21:13:58Z

Rebased.

asajeffrey · 2017-01-05T21:14:53Z

@bors-servo try

bors-servo · 2017-01-05T21:14:58Z

⌛ Trying commit 7c2de62 with merge c0706a2...

…ding, r=<try> Implement browsing context discarding  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). ---  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14559)

bors-servo · 2017-01-05T22:06:42Z

💔 Test failed - linux-rel-wpt

jdm · 2017-01-06T00:03:54Z

@bors-servo: retry

Intermittent crash ("failed to fill whole buffer") in /navigation-timing/nav2_test_open_data_uri.html, /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-components.html, and /html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/allow-scripts-flag-changing-2.html, /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html #14323

bors-servo · 2017-01-06T00:03:58Z

⌛ Trying commit 7c2de62 with merge 1d5503b...

…ding, r=<try> Implement browsing context discarding  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). ---  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14559)

bors-servo · 2017-01-06T02:05:25Z

☀️ 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

cbrewster · 2017-01-06T02:20:10Z

@bors-servo r+ retry

bors-servo · 2017-01-06T02:20:10Z

📌 Commit 7c2de62 has been approved by cbrewster

bors-servo · 2017-01-06T02:20:13Z

⌛ Testing commit 7c2de62 with merge 8b274f2...

…ding, r=cbrewster Implement browsing context discarding  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). ---  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14559)

bors-servo · 2017-01-06T03:59:14Z

☀️ 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

asajeffrey added the A-content/script Related to the script thread label Dec 12, 2016

highfive assigned ghost Dec 12, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 12, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Dec 13, 2016

asajeffrey force-pushed the script-track-document-and-bc-discarding branch from 9d314d3 to 7f5f9d6 Compare December 13, 2016 15:10

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Dec 13, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Dec 15, 2016

asajeffrey force-pushed the script-track-document-and-bc-discarding branch from 7f5f9d6 to 8be8c54 Compare December 15, 2016 04:27

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Dec 15, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Dec 15, 2016

asajeffrey force-pushed the script-track-document-and-bc-discarding branch from 8be8c54 to c4d1813 Compare December 23, 2016 15:09

highfive assigned cbrewster and unassigned ghost Dec 23, 2016

cbrewster suggested changes Dec 23, 2016

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Dec 24, 2016

Implement browsing context discarding.

7c2de62

asajeffrey force-pushed the script-track-document-and-bc-discarding branch from e3788d8 to 7c2de62 Compare January 5, 2017 21:13

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Jan 5, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 5, 2017

cbrewster approved these changes Jan 6, 2017

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-tests-failed The changes caused existing tests to fail. labels Jan 6, 2017

cbrewster mentioned this pull request Jan 6, 2017

Determine if a Document is active #14876

Closed

bors-servo merged commit 7c2de62 into servo:master Jan 6, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 6, 2017

bors-servo mentioned this pull request Jan 6, 2017

[WIP] Make subsequent about:blank loads async #14764

Closed

5 tasks

Uh oh!

Conversation

asajeffrey commented Dec 12, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Dec 12, 2016

Uh oh!

asajeffrey commented Dec 12, 2016

Uh oh!

asajeffrey commented Dec 12, 2016

Uh oh!

bors-servo commented Dec 13, 2016

Uh oh!

bors-servo commented Dec 15, 2016

Uh oh!

bors-servo commented Dec 15, 2016

Uh oh!

asajeffrey commented Dec 23, 2016

Uh oh!

cbrewster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Dec 24, 2016

Uh oh!

asajeffrey commented Jan 5, 2017

Uh oh!

asajeffrey commented Jan 5, 2017

Uh oh!

bors-servo commented Jan 5, 2017

Uh oh!

bors-servo commented Jan 5, 2017

Uh oh!

jdm commented Jan 6, 2017

Uh oh!

bors-servo commented Jan 6, 2017

Uh oh!

bors-servo commented Jan 6, 2017

Uh oh!

cbrewster commented Jan 6, 2017

Uh oh!

bors-servo commented Jan 6, 2017

Uh oh!

bors-servo commented Jan 6, 2017

Uh oh!

bors-servo commented Jan 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asajeffrey commented Dec 12, 2016 •

edited by larsbergstrom

Loading