Allow windows to share browsing contexts. by asajeffrey · Pull Request #15120 · servo/servo

asajeffrey · 2017-01-19T22:18:43Z

This PR allows different Window objects in the same browsing context to share a BrowsingContext object.

SpiderMonkey requires a WindowProxy object to be in the same compartment as its Window, so when a WindowProxy changes Window, we have to brain-transplant it. In turn this requires the reflector of a BrowsingContext to be mutable.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Transplant WindowProxy objects #13608 and Don't create a new BrowsingContext object for each Window #14843
These changes do not require tests because an existing test catches this (/html/browsers/the-window-object/Window-document.html is now PASS)

This change is

highfive · 2017-01-19T22:18:46Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/browsingcontext.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/xmldocument.rs, components/script/dom/servoparser/mod.rs, components/script/script_thread.rs, components/script/dom/domparser.rs, components/script/dom/window.rs, components/script/dom/domimplementation.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/browsingcontext.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/xmldocument.rs, components/script/dom/servoparser/mod.rs, components/script/script_thread.rs, components/script/dom/domparser.rs, components/script/dom/window.rs, components/script/dom/domimplementation.rs

highfive · 2017-01-19T22:18:47Z

Warning

These commits modify unsafe code. Please review it carefully!

asajeffrey · 2017-01-19T22:19:15Z

r? @jdm

asajeffrey · 2017-01-19T22:19:38Z

cc @Ms2ger @cbrewster

jdm · 2017-01-19T22:30:32Z

I think my changes in #15118 may make some of the mutable Reflector changes unnecessary (particularly if we modify rust-mozjs to make Heap::set not require &mut self).

asajeffrey · 2017-01-19T22:34:51Z

You're right, we could use reflector.rootable() to set the reflector.

asajeffrey · 2017-01-20T15:54:07Z

@jdm: I used reflector.rootable() to set the reflector, rather than having the reflector inside a RefCell.

asajeffrey · 2017-01-20T16:00:38Z

@cbrewster does this PR fix the problems you were having with webstorage tests? http://logs.glob.uno/?c=mozilla%23servo&s=11+Jan+2017&e=11+Jan+2017#c591494

asajeffrey · 2017-01-20T16:01:53Z

For the record, here's the IRC conversation I had with @bzbarsky about brain transplanting window proxy objects: http://logs.glob.uno/?c=mozilla%23jsapi&s=19+Jan+2017&e=19+Jan+2017#c819204

bors-servo · 2017-01-23T12:11:35Z

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

bors-servo · 2017-01-24T16:59:32Z

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

jdm · 2017-01-26T23:28:31Z

components/script/dom/browsingcontext.rs

+            // Set the window proxy.
+            SetWindowProxy(cx, window_jsobject, new_window_proxy.handle());
+
+            // Create a new reflector.


Update the reflector.

Oops. Done.

jdm · 2017-01-26T23:34:05Z

components/script/dom/browsingcontext.rs

+            // Transfer ownership of this browsing context from the old window proxy to the new one.
+            SetProxyExtra(new_window_proxy.get(), 0, &PrivateValue(self as *const _ as *const _));
+
+            // Set the window proxy.


This comment is redundant given the function name. How about Notify the JS engine that the window proxy has changed.?

jdm · 2017-01-26T23:34:36Z

components/script/dom/document.rs

@@ -184,12 +184,12 @@ pub struct Document {
    node: Node,
    window: JS<Window>,
    /// https://html.spec.whatwg.org/multipage/#concept-document-bc


This comment doesn't belong any more.

Oops. Done.

jdm · 2017-01-26T23:35:49Z

components/script/dom/document.rs


    pub fn new(window: &Window,
-               browsing_context: Option<&BrowsingContext>,
+               has_browsing_context: bool,


I lean towards a self-documenting enum here, given that every caller passes a boolean literal.

jdm · 2017-01-26T23:37:15Z

components/script/script_thread.rs

    /// The documents for pipelines managed by this thread
    documents: DOMRefCell<Documents>,
+    /// The browsing contexts known by this thread
+    /// TODO: this map grows, but never shrinks.


We should file an issue about this and mention it here.

Done. It's #15258.

bors-servo · 2017-01-27T20:58:41Z

⌛ Testing commit 2e863a9 with merge 4f1d6ba...

bors-servo · 2017-01-27T21:08:02Z

💔 Test failed - mac-dev-unit

asajeffrey · 2017-01-27T21:10:53Z

That would be caused by it picking up the old version of rust-mozjs. Wait for servo/rust-mozjs#329 to land...

bors-servo · 2017-01-28T01:27:52Z

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

asajeffrey · 2017-01-29T03:58:25Z

servo/rust-mozjs#329 has landed. Rebased. @bors-servo: r=jdm

bors-servo · 2017-01-29T03:58:25Z

📌 Commit 403499a has been approved by jdm

bors-servo · 2017-01-29T04:35:41Z

⌛ Testing commit 403499a with merge 67c1826...

…xts, r=jdm Allow windows to share browsing contexts.  This PR allows different `Window` objects in the same browsing context to share a `BrowsingContext` object. SpiderMonkey requires a `WindowProxy` object to be in the same compartment as its `Window`, so when a `WindowProxy` changes `Window`, we have to brain-transplant it. In turn this requires the reflector of a `BrowsingContext` to be mutable. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13608 and #14843 - [X] These changes do not require tests because an existing test catches this (`/html/browsers/the-window-object/Window-document.html` is now `PASS`)   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15120)

bors-servo · 2017-01-29T05:24:00Z

☀️ 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 Jan 19, 2017

highfive assigned emilio Jan 19, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 19, 2017

highfive assigned jdm and unassigned emilio Jan 19, 2017

asajeffrey force-pushed the script-windows-share-browsing-contexts branch from 98a3843 to ba006bc Compare January 19, 2017 22:22

asajeffrey changed the title ~~Allow windows to share browsing contextw.~~ Allow windows to share browsing contexts. Jan 19, 2017

asajeffrey force-pushed the script-windows-share-browsing-contexts branch from ba006bc to 7129af4 Compare January 20, 2017 15:52

highfive added the S-needs-rebase There are merge conflict errors. label Jan 23, 2017

asajeffrey force-pushed the script-windows-share-browsing-contexts branch 3 times, most recently from 7129af4 to dee9594 Compare January 23, 2017 19:19

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

asajeffrey mentioned this pull request Jan 24, 2017

WindowProxy for browsing contexts whose active document is in another script thread #15180

Closed

asajeffrey force-pushed the script-windows-share-browsing-contexts branch from dee9594 to e5254db Compare January 24, 2017 18:17

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

jdm mentioned this pull request Jan 25, 2017

Make BrowserContext spec-compliant #5238

Closed

jdm requested changes Jan 26, 2017

View reviewed changes

jdm added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Jan 26, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-squash Some (or all) of the commits in the PR should be combined. labels Jan 27, 2017

asajeffrey removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Jan 27, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 27, 2017

asajeffrey added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Jan 27, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Jan 28, 2017

Alan Jeffrey added 2 commits January 28, 2017 21:48

Remove warnings about unnecessary mutability.

fc67878

Allow windows to share browsing contexts.

403499a

asajeffrey force-pushed the script-windows-share-browsing-contexts branch from 2e863a9 to 403499a Compare January 29, 2017 03:57

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 29, 2017

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-needs-rebase There are merge conflict errors. labels Jan 29, 2017

asajeffrey removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Jan 29, 2017

bors-servo merged commit 403499a into servo:master Jan 29, 2017

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

cbrewster mentioned this pull request Jan 30, 2017

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

Closed

5 tasks

asajeffrey mentioned this pull request Mar 2, 2017

Don't create a new BrowsingContext object for each Window #14843

Closed

Uh oh!

Conversation

asajeffrey commented Jan 19, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jan 19, 2017

Uh oh!

highfive commented Jan 19, 2017

Uh oh!

asajeffrey commented Jan 19, 2017

Uh oh!

asajeffrey commented Jan 19, 2017

Uh oh!

jdm commented Jan 19, 2017

Uh oh!

asajeffrey commented Jan 19, 2017

Uh oh!

asajeffrey commented Jan 20, 2017

Uh oh!

asajeffrey commented Jan 20, 2017

Uh oh!

asajeffrey commented Jan 20, 2017

Uh oh!

bors-servo commented Jan 23, 2017

Uh oh!

bors-servo commented Jan 24, 2017

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 Jan 27, 2017

Uh oh!

bors-servo commented Jan 27, 2017

Uh oh!

asajeffrey commented Jan 27, 2017

Uh oh!

bors-servo commented Jan 28, 2017

Uh oh!

asajeffrey commented Jan 29, 2017

Uh oh!

bors-servo commented Jan 29, 2017

Uh oh!

bors-servo commented Jan 29, 2017

Uh oh!

bors-servo commented Jan 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asajeffrey commented Jan 19, 2017 •

edited by larsbergstrom

Loading