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

Remove sync constellation -> embedder communication #22121

Merged
merged 4 commits into from Dec 26, 2018

Conversation

Projects
None yet
7 participants
@gterzian
Copy link
Collaborator

gterzian commented Nov 6, 2018


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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Nov 6, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: ports/servo/browser.rs, components/constellation/constellation.rs, components/compositing/windowing.rs, components/servo/lib.rs
  • @KiChjang: components/script_traits/lib.rs

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from 5029901 to e7484a8 Nov 6, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Nov 6, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 6, 2018

⌛️ Trying commit e7484a8 with merge 0f36863...

bors-servo added a commit that referenced this pull request Nov 6, 2018

Auto merge of #22121 - gterzian:remove_constellation_block_in_navigat…
…ion, r=<try>

Remove sync constellation -> embedder communication

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

---
<!-- 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 #22042 (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/22121)
<!-- Reviewable:end -->
@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Nov 6, 2018

@jdm this is the only case of recv() involving a channel send inside an EmbedderMsg I could find in the constellation(there are some other cases involving canvas and layout however).

In the case of the unloading of a document, or the opening of an auxiliary, it's the script-thread that sends the EmbedderMsg(via a forwarding by the constellation), and blocks waiting on the reply from the embedder...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 6, 2018

💔 Test failed - status-taskcluster

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from e7484a8 to bd55e0f Nov 6, 2018

@highfive highfive removed the S-tests-failed label Nov 6, 2018

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch 2 times, most recently from 3f39521 to 629fe8e Nov 6, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Nov 7, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 7, 2018

⌛️ Trying commit 629fe8e with merge 4bc5201...

bors-servo added a commit that referenced this pull request Nov 7, 2018

Auto merge of #22121 - gterzian:remove_constellation_block_in_navigat…
…ion, r=<try>

Remove sync constellation -> embedder communication

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

---
<!-- 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 #22042 (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/22121)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 7, 2018

💔 Test failed - status-taskcluster

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from 629fe8e to 035c4e1 Nov 7, 2018

@highfive highfive removed the S-tests-failed label Nov 7, 2018

@paulrouget paulrouget assigned paulrouget and unassigned ferjm Nov 7, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Nov 7, 2018

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/browsers/browsing-the-web/unloading-documents/unload/002.html", "line": 105047, "action": "test_result", "expected": "OK"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function.html", "line": 105249, "action": "test_result", "expected": "OK"}
{"status": "ERROR", "group": "default", "message": "Got results from /html/browsers/browsing-the-web/history-traversal/support/window-name-test.sub.html?shouldhavename=true&sendmessage=true, expected /html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html\nTraceback (most recent call last):\n  File \"/home/servo/buildbot/slave/linux-rel-css/build/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/base.py\", line 162, in run_test\n    result = self.do_test(test)\n  File \"/home/servo/buildbot/slave/linux-rel-css/build/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/executorservo.py\", line 110, in do_test\n    result = self.convert_result(test, self.result_data)\n  File \"/home/servo/buildbot/slave/linux-rel-css/build/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/base.py\", line 69, in __call__\n    (result_url, test.url))\nAssertionError: Got results from /html/browsers/browsing-the-web/history-traversal/support/window-name-test.sub.html?shouldhavename=true&sendmessage=true, expected /html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html\n", "stack": null, "subtest": null, "test": "/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html", "line": 105297, "action": "test_result", "expected": "OK"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/semantics/forms/form-submission-0/getactionurl.html", "line": 105537, "action": "test_result", "expected": "OK"}

Given all those timeouts, and more, it looks likes something is wrong...

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from 035c4e1 to 610bbe3 Nov 7, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Nov 7, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 7, 2018

⌛️ Trying commit 610bbe3 with merge a38dfc2...

bors-servo added a commit that referenced this pull request Nov 7, 2018

Auto merge of #22121 - gterzian:remove_constellation_block_in_navigat…
…ion, r=<try>

Remove sync constellation -> embedder communication

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

---
<!-- 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 #22042 (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/22121)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 7, 2018

💔 Test failed - linux-rel-css

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 23, 2018

No upstreamable changes; closed existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14658.

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from f638c84 to fbfaa16 Dec 23, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Dec 23, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 23, 2018

⌛️ Trying commit fbfaa16 with merge 9357720...

bors-servo added a commit that referenced this pull request Dec 23, 2018

Auto merge of #22121 - gterzian:remove_constellation_block_in_navigat…
…ion, r=<try>

Remove sync constellation -> embedder communication

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

---
<!-- 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 #22042 (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/22121)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 23, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Dec 23, 2018

Ok, this is what I have done:

  1. Turned the panic into a warning(which is consistent to what was already there): https://github.com/servo/servo/pull/22121/files#diff-55c92a6a5ba7654ce45fe6fc6c63740fR1062
  2. Only checking this "delaying load events mode" if there is actually a browsing context, avoiding the panic: https://github.com/servo/servo/pull/22121/files#diff-30a18e04d7e0b66aafdf192e416cad44R1914
  3. Added a new commit for something I came across while debugging the latest, seems like a straightforward change based on the spec. See step4 of https://html.spec.whatwg.org/multipage/parsing.html#abort-a-parser

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from fbfaa16 to 5dca870 Dec 23, 2018

@gterzian gterzian force-pushed the gterzian:remove_constellation_block_in_navigation branch from 5dca870 to cca354b Dec 23, 2018

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Dec 23, 2018

Added this check for consistency(even though it hadn't crashed at that point yet for lack of a browsing context, like it did in maybe_queue_document_load) eb82e78#diff-1879ac6bd5d567e2aa43529d33474677R2090

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Dec 26, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 26, 2018

📌 Commit cca354b has been approved by paulrouget

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 26, 2018

⌛️ Testing commit cca354b with merge 66223ee...

bors-servo added a commit that referenced this pull request Dec 26, 2018

Auto merge of #22121 - gterzian:remove_constellation_block_in_navigat…
…ion, r=paulrouget

Remove sync constellation -> embedder communication

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

---
<!-- 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 #22042 (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/22121)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 26, 2018

💔 Test failed - linux-rel-wpt

@gterzian

This comment has been minimized.

Copy link
Collaborator

gterzian commented Dec 26, 2018

@bors-servo retry

#22559 (this one is related, yet really intermittent, cannot reproduce)
#22560

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 26, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 26, 2018

@bors-servo bors-servo merged commit cca354b into servo:master Dec 26, 2018

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

@gterzian gterzian deleted the gterzian:remove_constellation_block_in_navigation branch Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment