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

Improve spec compliance of discarding BCs #24143

Merged
merged 1 commit into from Sep 27, 2019

Conversation

@gterzian
Copy link
Member

gterzian commented Sep 5, 2019

Came-up at #23637 (comment)


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive
Copy link

highfive commented Sep 5, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmliframeelement.rs, components/script/dom/window.rs
  • @KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/window.rs
@highfive
Copy link

highfive commented Sep 5, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian gterzian mentioned this pull request Sep 5, 2019
0 of 5 tasks complete
@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch 2 times, most recently from aa84725 to 5810f18 Sep 5, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 5, 2019

@jdm @asajeffrey r?

The problem is still running tasks after either a frame has been removed from the tree, or an auxiliary closed, in both cases the BC is discarded "sync"(as per the spec), however tasks are only ignored later when the script-thread handles the "exit pipeline" message, which means that in the meantime the global can still be handling tasks. This became apparent in #23637 (comment)

@gterzian
Copy link
Member Author

gterzian commented Sep 5, 2019

For good measure I'm also preventing handling any user input via compositor events, since those result in firing events and potentially running JS code, if the BC is being discarded.

(I mentioned something about this not applying to iframe previously, that was wrong, and it is now also applied to them).

@gterzian
Copy link
Member Author

gterzian commented Sep 5, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

Trying commit c8b7cf0 with merge ab60ff1...

bors-servo added a commit that referenced this pull request Sep 5, 2019
Improve spec compliance of discarding BCs

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

Came-up at #23637 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24143)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

💔 Test failed - linux-rel-css

@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from c8b7cf0 to 52b5485 Sep 5, 2019
@highfive highfive removed the S-tests-failed label Sep 5, 2019
@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from 52b5485 to 557d5aa Sep 5, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 5, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

Trying commit 557d5aa with merge 441285b...

bors-servo added a commit that referenced this pull request Sep 5, 2019
Improve spec compliance of discarding BCs

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

Came-up at #23637 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24143)
<!-- Reviewable:end -->
@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from 557d5aa to e434652 Sep 5, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 5, 2019

well, look at that...

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "worker-src 'self' directive should disallow cross origin static import.", 
    "test": "/workers/modules/dedicated-worker-import-csp.html", 
    "line": 284199, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "script-src 'self' directive should disallow cross origin static import.", 
    "test": "/workers/modules/dedicated-worker-import-csp.html", 
    "line": 284201, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "worker-src 'self' directive should override script-src * directive and disallow cross origin static import.", 
    "test": "/workers/modules/dedicated-worker-import-csp.html", 
    "line": 284204, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "script-src 'self' directive should disallow cross origin dynamic import.", 
    "test": "/workers/modules/dedicated-worker-import-csp.html", 
    "line": 284205, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "OK", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/workers/modules/dedicated-worker-import-csp.html", 
    "line": 284211, 
    "action": "test_result", 
    "expected": "CRASH"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "promise_test: Unhandled rejection with value: \"FAIL - first document not exposed. navigated_back is undefined\"", 
    "stack": null, 
    "subtest": "Test that iframe navigations are not observable by the parent, even after history navigations by the parent", 
    "test": "/resource-timing/nested-context-navigations-iframe.html", 
    "line": 303458, 
    "action": "test_result", 
    "expected": "TIMEOUT"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "promise_test: Unhandled rejection with value: \"FAIL - first document not exposed. navigated_back is true\"", 
    "stack": null, 
    "subtest": "Test that crossorigin iframe navigations are not observable by the parent, even after history navigations by the parent", 
    "test": "/resource-timing/nested-context-navigations-iframe.html", 
    "line": 303459, 
    "action": "test_result", 
    "expected": "NOTRUN"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "promise_test: Unhandled rejection with value: \"FAIL - initial document should be observable\"", 
    "stack": null, 
    "subtest": "Test that iframe navigations are not observable by the parent", 
    "test": "/resource-timing/nested-context-navigations-iframe.html", 
    "line": 303460, 
    "action": "test_result", 
    "expected": "NOTRUN"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "promise_test: Unhandled rejection with value: \"FAIL - initial document should be observable\"", 
    "stack": null, 
    "subtest": "Test that crossorigin iframe navigations are not observable by the parent", 
    "test": "/resource-timing/nested-context-navigations-iframe.html", 
    "line": 303461, 
    "action": "test_result", 
    "expected": "NOTRUN"
}
{
    "status": "TIMEOUT", 
    "group": "default", 
    "message": "Test timed out", 
    "stack": null, 
    "subtest": "Test that iframe refreshes are not observable by the parent", 
    "test": "/resource-timing/nested-context-navigations-iframe.html", 
    "line": 303462, 
    "action": "test_result", 
    "expected": "NOTRUN"
}
{
    "status": "TIMEOUT", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/resource-timing/nested-context-navigations-iframe.html", 
    "line": 303464, 
    "action": "test_result", 
    "expected": "CRASH"
}

And also

{
    "status": "CRASH", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/html/browsers/the-window-object/close-method.window.html", 
    "line": 172617, 
    "action": "test_result", 
    "expected": "OK"
}
@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from 878e5ba to 5b0980c Sep 5, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 5, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

Trying commit 5b0980c with merge 86f0b2b...

bors-servo added a commit that referenced this pull request Sep 5, 2019
Improve spec compliance of discarding BCs

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

Came-up at #23637 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24143)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 5, 2019

💔 Test failed - linux-rel-wpt

@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from 5b0980c to 89ccec2 Sep 5, 2019
@highfive highfive removed the S-tests-failed label Sep 5, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 10, 2019

@bors-servo try=wpt (compilation error, fixed, this time with a local check first)

@bors-servo
Copy link
Contributor

bors-servo commented Sep 10, 2019

Trying commit 6b499ce with merge 97ee008...

bors-servo added a commit that referenced this pull request Sep 10, 2019
Improve spec compliance of discarding BCs

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

Came-up at #23637 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24143)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 10, 2019

💔 Test failed - linux-rel-wpt

@gterzian
Copy link
Member Author

gterzian commented Sep 11, 2019

{
    "status": "FAIL", 
    "group": "default", 
    "message": "/css/css-values/vh-support-transform-translate.html b66c48d9abd17b3672f590a293821101a1847cb9\n/css/css-values/reference/all-green.html ca9048d25067fe2666ee98ffdaee4d41e63aa283", 
    "stack": null, 
    "subtest": null, 
    "test": "/css/css-values/vh-support-transform-translate.html", 
    "line": 38036, 
    "action": "test_result", 
    "expected": "PASS"
}

I think this one might be intermittent

@gterzian
Copy link
Member Author

gterzian commented Sep 11, 2019

@asajeffrey ready for another round, see the new commits, to be squashed.

@CYBAI
Copy link
Collaborator

CYBAI commented Sep 11, 2019

I think this one might be intermittent

Yes, it's #23290.

@gterzian gterzian requested a review from asajeffrey Sep 18, 2019
@gterzian gterzian assigned asajeffrey and unassigned nox Sep 18, 2019
@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from 75104ff to 37ac288 Sep 19, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Sep 20, 2019

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

do not handle compositor input events when BC is being discarded

prevent firing of timers for discarded BCs

return null for opener is BC has been discarded

bundle discard BC steps into window method

return null in window.opener, if BC has already been discarded

move the window closed check pre-event to script-thread
@gterzian gterzian force-pushed the gterzian:improve_spec_comp_bc_discarding branch from 37ac288 to 45ec250 Sep 22, 2019
@gterzian
Copy link
Member Author

gterzian commented Sep 25, 2019

@asajeffrey This one is ready for another review.

@asajeffrey
Copy link
Member

asajeffrey commented Sep 27, 2019

Yay, this looks like it's quite the improvement! @bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

📌 Commit 45ec250 has been approved by asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

Testing commit 45ec250 with merge 75c201f...

bors-servo added a commit that referenced this pull request Sep 27, 2019
…ajeffrey

Improve spec compliance of discarding BCs

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

Came-up at #23637 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/24143)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing 75c201f to master...

@bors-servo bors-servo merged commit 45ec250 into servo:master Sep 27, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:improve_spec_comp_bc_discarding branch Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.