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

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 #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:34:22Z

components/script/dom/node.rs

@@ -771,6 +771,20 @@ impl Node {
        self.owner_doc().is_html_document()
    }

+    // 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:22Z

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:11:01Z

components/script/dom/node.rs

@@ -768,7 +768,11 @@ impl Node {
    }

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


        Auto merge of #13965 - asajeffrey:script-iframe-check-document-browsi…

…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


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

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


        Auto merge of #13965 - asajeffrey:script-iframe-check-document-browsi…

…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 label Oct 28, 2016

highfive assigned mbrubeck Oct 28, 2016

highfive added the S-awaiting-review label Oct 28, 2016

asajeffrey mentioned this pull request Oct 28, 2016

Script lookup iframes by frameid #13646

Merged

3 of 3 tasks complete

Ms2ger requested changes Oct 31, 2016

View changes

Ms2ger assigned Ms2ger and unassigned mbrubeck Oct 31, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 2, 2016

asajeffrey added the S-needs-squash label Nov 3, 2016

Ms2ger requested changes Nov 3, 2016

View changes

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

Ms2ger approved these changes Nov 3, 2016

View changes

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

highfive removed S-awaiting-review S-needs-squash labels Nov 3, 2016

highfive added the S-awaiting-merge label Nov 3, 2016

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

Loading status checks…

1803a58

…processing src.

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

highfive added S-awaiting-review and removed S-awaiting-merge labels Nov 3, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 3, 2016

bors-servo merged commit 1803a58 into servo:master Nov 3, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 3, 2016

bors-servo mentioned this pull request Nov 3, 2016

Script thread with no root document #14013

Merged

3 of 3 tasks complete

servo / servo

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

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

asajeffrey commented Oct 28, 2016 •

edited by larsbergstrom

highfive commented Oct 28, 2016

highfive commented Oct 28, 2016

asajeffrey commented Oct 28, 2016

Ms2ger left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Nov 1, 2016

Ms2ger commented Nov 2, 2016

nox commented Nov 2, 2016

asajeffrey commented Nov 2, 2016

asajeffrey commented Nov 2, 2016

asajeffrey commented Nov 2, 2016

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

larsbergstrom commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

KiChjang commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

servo / servo

Join GitHub today

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

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

Conversation

asajeffrey commented Oct 28, 2016 • edited by larsbergstrom

highfive commented Oct 28, 2016

highfive commented Oct 28, 2016

asajeffrey commented Oct 28, 2016

Ms2ger left a comment

This comment has been minimized.

Ms2ger Oct 31, 2016

This comment has been minimized.

asajeffrey Oct 31, 2016

This comment has been minimized.

Ms2ger Oct 31, 2016

This comment has been minimized.

asajeffrey Oct 31, 2016

asajeffrey commented Nov 1, 2016

Ms2ger commented Nov 2, 2016

nox commented Nov 2, 2016

asajeffrey commented Nov 2, 2016

asajeffrey commented Nov 2, 2016

asajeffrey commented Nov 2, 2016

This comment has been minimized.

Ms2ger Nov 3, 2016

This comment has been minimized.

asajeffrey Nov 3, 2016

Ms2ger commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

larsbergstrom commented Nov 3, 2016

asajeffrey commented Nov 3, 2016

KiChjang commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

asajeffrey commented Oct 28, 2016 •

edited by larsbergstrom