Check that an iframe is in a document with a browsing context before processing src by asajeffrey · Pull Request #13965 · servo/servo

asajeffrey · 2016-10-28T18:11:03Z

Check that an iframe is in a document with a browsing context before processing src.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Check that an iframe is in a document with a browsing context before processing src #13964.
These changes do not require tests because this is already tested by https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/old-tests/submission/Opera/script_scheduling/034.html

This change is

highfive · 2016-10-28T18:11:07Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmliframeelement.rs, components/script/dom/node.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/node.rs

highfive · 2016-10-28T18:11:08Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2016-10-28T18:11:19Z

cc @Ms2ger

Ms2ger

r? @Ms2ger

Ms2ger · 2016-10-31T16:32:48Z

components/script/dom/node.rs

    }

+    // https://dom.spec.whatwg.org/#in-a-document-tree
+    pub fn is_in_a_document_tree(&self) -> bool {


This duplicates is_in_doc() very slowly; please remove.

The problem is that is_in_doc returns true if the node is in a document fragment.

Ms2ger · 2016-10-31T16:34:10Z

components/script/dom/node.rs

+    }
+
+    pub fn is_in_a_document_tree_with_a_browsing_context(&self) -> bool {
+        self.inclusive_ancestors().last()


This can be self.is_in_doc() && self.owner_doc().browsing_context().is_some()

Same problem.

asajeffrey · 2016-11-01T19:57:58Z

@Ms2ger should I fix is_in_doc so that it returns false when it's in a DocumentFragment?

Ms2ger · 2016-11-02T10:06:03Z

The problem is that is_in_doc returns true if the node is in a document fragment.

That sounds like a pretty serious bug. Are you sure?

CC @nox.

nox · 2016-11-02T13:34:37Z

The problem is that is_in_doc returns true if the node is in a document fragment.

That doesn't sound true to me. Where did you find that?

asajeffrey · 2016-11-02T14:21:21Z

OK, I'll go and check.

asajeffrey · 2016-11-02T15:16:30Z

Unsurprisingly, you are both right. Hmm. I could have sworn I was getting an error because of this. Oh well, I'll fix this.

asajeffrey · 2016-11-02T19:14:43Z

I remember what the issue was! It's that owner_doc is Some(document) even when the node is in a document fragment. is_in_doc is fine.

Ms2ger · 2016-11-03T14:10:57Z

components/script/dom/node.rs


    pub fn is_in_html_doc(&self) -> bool {
-        self.owner_doc().is_html_document()
+        self.is_in_doc() && self.owner_doc().is_html_document()


Unfortunately this needs to return true if is_in_doc() is false. Maybe you should just inline it into its only caller, to avoid confusion.

Ms2ger · 2016-11-03T15:05:00Z

r=me if you squash all commits

asajeffrey · 2016-11-03T15:07:22Z

@Ms2ger thanks! @bors-servo r=Ms2ger

bors-servo · 2016-11-03T15:07:24Z

📌 Commit 330953a has been approved by Ms2ger

bors-servo · 2016-11-03T15:12:05Z

⌛ Testing commit 330953a with merge 06e3ed9...

…ng-context, r=Ms2ger Check that an iframe is in a document with a browsing context before processing src  Check that an iframe is in a document with a browsing context before processing src. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13964. - [X] These changes do not require tests because this is already tested by https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/old-tests/submission/Opera/script_scheduling/034.html   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13965)

larsbergstrom · 2016-11-03T16:19:25Z

@bors-servo clean force retry

…processing src.

asajeffrey · 2016-11-03T17:52:12Z

Rebased to unstick homu.

KiChjang · 2016-11-03T17:52:49Z

@bors-servo r=Ms2ger

bors-servo · 2016-11-03T17:52:50Z

📌 Commit 1803a58 has been approved by Ms2ger

bors-servo · 2016-11-03T19:03:20Z

⌛ Testing commit 1803a58 with merge 4984a83...

…ng-context, r=Ms2ger Check that an iframe is in a document with a browsing context before processing src  Check that an iframe is in a document with a browsing context before processing src. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13964. - [X] These changes do not require tests because this is already tested by https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/old-tests/submission/Opera/script_scheduling/034.html   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13965)

bors-servo · 2016-11-03T20:04:41Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

asajeffrey added the A-content/script Related to the script thread label Oct 28, 2016

highfive assigned mbrubeck Oct 28, 2016

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

asajeffrey mentioned this pull request Oct 28, 2016

Script lookup iframes by frameid #13646

Merged

3 tasks

Ms2ger suggested changes Oct 31, 2016

View reviewed changes

Ms2ger assigned Ms2ger and unassigned mbrubeck Oct 31, 2016

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 31, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 2, 2016

asajeffrey added the S-needs-squash Some (or all) of the commits in the PR should be combined. label Nov 3, 2016

Ms2ger suggested changes Nov 3, 2016

View reviewed changes

asajeffrey force-pushed the script-iframe-check-document-browsing-context branch from 75845b2 to 5ea60c0 Compare November 3, 2016 15:03

Ms2ger approved these changes Nov 3, 2016

View reviewed changes

asajeffrey force-pushed the script-iframe-check-document-browsing-context branch from 5ea60c0 to 330953a Compare November 3, 2016 15:06

highfive removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 3, 2016

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 3, 2016

Check that an iframe is in a document with a browsing context before …

1803a58

…processing src.

asajeffrey force-pushed the script-iframe-check-document-browsing-context branch from 330953a to 1803a58 Compare November 3, 2016 17:51

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 3, 2016

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. labels Nov 3, 2016

bors-servo merged commit 1803a58 into servo:master Nov 3, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 3, 2016

bors-servo mentioned this pull request Nov 3, 2016

Script thread with no root document #14013

Merged

3 tasks

Uh oh!

Conversation

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

Uh oh!

Uh oh!

highfive commented Oct 28, 2016

Uh oh!

highfive commented Oct 28, 2016

Uh oh!

asajeffrey commented Oct 28, 2016

Uh oh!

Ms2ger left a comment

Choose a reason for hiding this comment

Uh oh!

Ms2ger Oct 31, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Oct 31, 2016

Choose a reason for hiding this comment

Uh oh!

Ms2ger Oct 31, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Oct 31, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey commented Nov 1, 2016

Uh oh!

Ms2ger commented Nov 2, 2016

Uh oh!

nox commented Nov 2, 2016

Uh oh!

asajeffrey commented Nov 2, 2016

Uh oh!

asajeffrey commented Nov 2, 2016

Uh oh!

asajeffrey commented Nov 2, 2016

Uh oh!

Ms2ger Nov 3, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Nov 3, 2016

Choose a reason for hiding this comment

Uh oh!

Ms2ger commented Nov 3, 2016

Uh oh!

asajeffrey commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

larsbergstrom commented Nov 3, 2016

Uh oh!

asajeffrey commented Nov 3, 2016

Uh oh!

KiChjang commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

asajeffrey commented Oct 28, 2016 •

edited by larsbergstrom

Loading