Skip to content
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

Add ImmutableOrigin to allow for serializing origins #15438

Merged
merged 1 commit into from Feb 22, 2017

Conversation

Projects
None yet
10 participants
@asajeffrey
Copy link
Member

commented Feb 8, 2017

This PR adds a serializable type ImmutableOrigin and a non-serializable type MutableOrigin. The immutable type represents an origin with null domain, and the mutable type represents an origin with a mutable domain. This separation is needed for implementing setting document.domain.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14892.
  • These changes do not require tests because it's a refactoring which will enable other features.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 8, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/xmldocument.rs, components/script/dom/domparser.rs, components/script/dom/bindings/trace.rs, components/script/lib.rs, components/script/script_thread.rs, components/script/dom/servoparser/mod.rs, components/script/dom/window.rs, components/script/origin.rs, components/script/dom/domimplementation.rs
  • @KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/net/http_loader.rs, components/script/dom/node.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/xmldocument.rs, components/net_traits/request.rs, components/net_traits/request.rs, components/script/dom/domparser.rs, components/script/dom/bindings/trace.rs, components/script/lib.rs, components/script/script_thread.rs, components/script/dom/servoparser/mod.rs, components/script/dom/window.rs, components/script/origin.rs, components/script/dom/domimplementation.rs
@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

The original work on this was by @cbrewster. r? @jdm

@highfive highfive assigned jdm and unassigned metajack Feb 8, 2017

@asajeffrey asajeffrey referenced this pull request Feb 9, 2017

Closed

Added some same-origin-domain checks #15478

3 of 3 tasks complete
@@ -2154,7 +2153,7 @@ impl Document {
HasBrowsingContext::No,
None,
// https://github.com/whatwg/html/issues/2109
Origin::opaque_identifier(),
self.origin.clone(),

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 10, 2017

Contributor

What's up here?

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 12, 2017

Author Member

Well the latest opinion on that discussion was @smaug---- saying whatwg/html#2109 (comment) which looks like it's coming down on the side of a template inheriting its origin, rather than generating a new opaque origin. Can the template document cause script to run? If so, we may need to make sure it runs in the right script thread.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 21, 2017

Author Member

Is this the only remaining issue? I can revert it if we want to keep the old behaviour.

This comment has been minimized.

Copy link
@jdm

jdm Feb 21, 2017

Member

Let's leave this as it is until that issue resolves one way or another.

* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

// #[cfg(feature = "servo")] use heapsize::HeapSizeOf;

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 10, 2017

Contributor

Uh...

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 12, 2017

Author Member

Oops. Fixed.

@@ -80,7 +80,7 @@ impl DOMImplementationMethods for DOMImplementation {
let doc = XMLDocument::new(win,
HasBrowsingContext::No,
None,
self.document.origin().alias(),

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 10, 2017

Contributor

I feel we should keep the alias/copy distinction.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 12, 2017

Author Member

The Rust naming contention seems to be that clone copies the reference to Rc.

This comment has been minimized.

Copy link
@jdm

jdm Feb 21, 2017

Member

Since the spec does not talk about aliasing any more there is no need for a special name here.

@@ -2410,14 +2411,18 @@ impl DocumentMethods for Document {
// https://html.spec.whatwg.org/multipage/#relaxing-the-same-origin-restriction
fn Domain(&self) -> DOMString {
// Step 1.
if self.browsing_context().is_none() {
if self.has_browsing_context {

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 10, 2017

Member

Missing !?

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 11, 2017

Author Member

Oops.

@asajeffrey asajeffrey referenced this pull request Feb 13, 2017

Merged

Implement setter for document.domain #15536

4 of 4 tasks complete
@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2017

@jdm ping?

@cbrewster cbrewster referenced this pull request Feb 17, 2017

Closed

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

3 of 5 tasks complete

@asajeffrey asajeffrey force-pushed the asajeffrey:url-serializable-origin branch from b07059c to 3087ea2 Feb 17, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

Clearly the original origin code is older than March 2016, and I've somehow missed that whatwg/html@438155d happened in the meantime.

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2017

Yes, the "alias" concept is gone, we may as well use the Rust naming convention for clone.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:url-serializable-origin branch from 3087ea2 to b7055cb Feb 20, 2017

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2017

This is blocked because the Serde implementation for Host doesn't exist any more.

IRC conversation with @SimonSapin and @nox at http://logs.glob.uno/?c=mozilla%23servo&s=20+Feb+2017&e=20+Feb+2017#c615535

@SimonSapin

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

Should be unblocked now: servo/rust-url#280 (comment)

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2017

Thanks!

@asajeffrey asajeffrey force-pushed the asajeffrey:url-serializable-origin branch from b7055cb to 5d703b9 Feb 20, 2017

url = "1.2"
url_serde = {version = "0.1", optional = true}
servo_rand = {path = "../rand"}

This comment has been minimized.

Copy link
@nox

nox Feb 21, 2017

Member

Alphabetical order.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 21, 2017

Author Member

Fixed.

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

I intend to complete me review today.

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2017

@jdm: thanks!

@asajeffrey asajeffrey referenced this pull request Feb 21, 2017

Merged

Implement cross-thread postMessage #15679

3 of 3 tasks complete
@jdm
Copy link
Member

left a comment

Only a couple small comments. Sorry about the delay!

@@ -2154,7 +2153,7 @@ impl Document {
HasBrowsingContext::No,
None,
// https://github.com/whatwg/html/issues/2109
Origin::opaque_identifier(),
self.origin.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Feb 21, 2017

Member

Let's leave this as it is until that issue resolves one way or another.

@@ -80,7 +80,7 @@ impl DOMImplementationMethods for DOMImplementation {
let doc = XMLDocument::new(win,
HasBrowsingContext::No,
None,
self.document.origin().alias(),

This comment has been minimized.

Copy link
@jdm

jdm Feb 21, 2017

Member

Since the spec does not talk about aliasing any more there is no need for a special name here.

@@ -155,7 +154,8 @@ struct InProgressLoad {
is_visible: bool,
/// The requested URL of the load.
url: ServoUrl,
origin: Origin,
/// The origin for the document
origin: MutableOrigin,

This comment has been minimized.

Copy link
@jdm

jdm Feb 21, 2017

Member

I think it might be clearer if we left this as an immutable one, since the domain of the origin can't change while the document is being fetch.

The one case I'm unsure about is if this can represent the origin of an about:blank loaded in a document that has set document.domain. If we need to provide a MutableOrigin for that, then I guess this needs to stay as-is.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 22, 2017

Author Member

Indeed, that is the one case.

@asajeffrey asajeffrey force-pushed the asajeffrey:url-serializable-origin branch from 1c8c7af to 66049a0 Feb 22, 2017

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2017

@nox suggests a forced push then an r+, so...

@bors-servo: r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

📌 Commit 66049a0 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

⌛️ Testing commit 66049a0 with merge 2f73c51...

bors-servo added a commit that referenced this pull request Feb 22, 2017

Auto merge of #15438 - asajeffrey:url-serializable-origin, r=jdm
Add ImmutableOrigin to allow for serializing origins

<!-- Please describe your changes on the following line: -->

This PR adds a serializable type `ImmutableOrigin` and a non-serializable type `MutableOrigin`. The immutable type represents an origin with `null` domain, and the mutable type represents an origin with a mutable domain. This separation is needed for implementing setting `document.domain`.

---
<!-- 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 #14892.
- [X] These changes do not require tests because it's a refactoring which will enable other features.

<!-- 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/15438)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

💔 Test failed - mac-dev-unit

@asajeffrey asajeffrey force-pushed the asajeffrey:url-serializable-origin branch from 66049a0 to bfd7b95 Feb 22, 2017

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2017

Oh yes ./mach build-geckolib.

@asajeffrey

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2017

@bors-servo r=jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

📌 Commit bfd7b95 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

⌛️ Testing commit bfd7b95 with merge 78e8c31...

bors-servo added a commit that referenced this pull request Feb 22, 2017

Auto merge of #15438 - asajeffrey:url-serializable-origin, r=jdm
Add ImmutableOrigin to allow for serializing origins

<!-- Please describe your changes on the following line: -->

This PR adds a serializable type `ImmutableOrigin` and a non-serializable type `MutableOrigin`. The immutable type represents an origin with `null` domain, and the mutable type represents an origin with a mutable domain. This separation is needed for implementing setting `document.domain`.

---
<!-- 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 #14892.
- [X] These changes do not require tests because it's a refactoring which will enable other features.

<!-- 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/15438)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

@bors-servo bors-servo merged commit bfd7b95 into servo:master Feb 22, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors-servo added a commit that referenced this pull request Mar 15, 2017

Auto merge of #15536 - asajeffrey:script-document-domain-setter, r=<try>
Implement setter for document.domain

<!-- Please describe your changes on the following line: -->

This PR implements the setter for `document.domain`.

It builds on #15438 and #15478, only the last commit is part of this PR.

It includes tests for similar-origin security checks.

---
<!-- 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 #934.
- [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/15536)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Mar 15, 2017

Auto merge of #15536 - asajeffrey:script-document-domain-setter, r=nox
Implement setter for document.domain

<!-- Please describe your changes on the following line: -->

This PR implements the setter for `document.domain`.

It builds on #15438 and #15478, only the last commit is part of this PR.

It includes tests for similar-origin security checks.

---
<!-- 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 #934.
- [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/15536)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Mar 15, 2017

Auto merge of #15679 - asajeffrey:script-postMessage-xorigin, r=emilio
Implement cross-thread postMessage

<!-- Please describe your changes on the following line: -->

This PR implements cross-thread postMessage,

It builds on #15438 and #15478, only the last commit is part of this PR.

---
<!-- 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] 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/15679)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.