Skip to content

Webmessaging: add more source info to message#27171

Closed
gterzian wants to merge 2 commits intoservo:mainfrom
gterzian:fix_postmessage
Closed

Webmessaging: add more source info to message#27171
gterzian wants to merge 2 commits intoservo:mainfrom
gterzian:fix_postmessage

Conversation

@gterzian
Copy link
Copy Markdown
Member

@gterzian gterzian commented Jul 4, 2020

FIX #27146
FIX #24066
FIX #22647
FIX #22154

  • ./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 ___

@highfive
Copy link
Copy Markdown

highfive commented Jul 4, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/script_thread.rs, components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @KiChjang: components/script/script_thread.rs, components/script_traits/lib.rs

@highfive
Copy link
Copy Markdown

highfive commented Jul 4, 2020

warning Warning warning

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 4, 2020
@gterzian gterzian changed the title send all required info as part of postmessage post-message: remove unnecessary call to constellation Jul 4, 2020
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Jul 4, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 4, 2020

⌛ Trying commit beffc43 with merge 422199c...

bors-servo added a commit that referenced this pull request Jul 4, 2020
@bors-servo
Auto merge of #27171 - gterzian:fix_postmessage, r=<try>
422199c 
post-message: remove unnecessary call to constellation

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

FIX #27146
FIX #24066
FIX #22647
FIX #22154

---
<!-- 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. -->
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Jul 4, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 4, 2020

⌛ Trying commit f3b5143 with merge 9f7730c...

bors-servo added a commit that referenced this pull request Jul 4, 2020
@bors-servo
Auto merge of #27171 - gterzian:fix_postmessage, r=<try>
9f7730c 
post-message: remove unnecessary call to constellation

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

FIX #27146
FIX #24066
FIX #22647
FIX #22154

---
<!-- 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. -->
@gterzian gterzian changed the title post-message: remove unnecessary call to constellation Webmessaging: put all required info in the message Jul 4, 2020
@gterzian gterzian force-pushed the fix_postmessage branch 3 times, most recently from 04b409e to 4d2f33c Compare July 4, 2020 17:18
@gterzian gterzian changed the title Webmessaging: put all required info in the message Webmessaging: add more source info to message Jul 4, 2020
@gterzian gterzian marked this pull request as draft July 5, 2020 06:09
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Jul 5, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Jul 5, 2020
@bors-servo
Auto merge of #27171 - gterzian:fix_postmessage, r=<try>
11f3ca3 
Webmessaging: add more source info to message

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

FIX #27146
FIX #24066
FIX #22647
FIX #22154

---
<!-- 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. -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 5, 2020

⌛ Trying commit ada2f9c with merge 11f3ca3...

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 5, 2020

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 5, 2020
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 5, 2020
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Jul 5, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Jul 5, 2020
@bors-servo
Auto merge of #27171 - gterzian:fix_postmessage, r=<try>
af9d9a1 
Webmessaging: add more source info to message

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

FIX #27146
FIX #24066
FIX #22647
FIX #22154

---
<!-- 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. -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 5, 2020

⌛ Trying commit b968142 with merge af9d9a1...

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 12, 2020

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2020
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2020
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Jul 12, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Jul 12, 2020
@bors-servo
Auto merge of #27171 - gterzian:fix_postmessage, r=<try>
97a0551 
Webmessaging: add more source info to message

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

FIX #27146
FIX #24066
FIX #22647
FIX #22154

---
<!-- 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. -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 12, 2020

⌛ Trying commit 23fee4f with merge 97a0551...

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 12, 2020

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2020
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2020
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Jul 12, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Jul 12, 2020
@bors-servo
Auto merge of #27171 - gterzian:fix_postmessage, r=<try>
234583e 
Webmessaging: add more source info to message

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

FIX #27146
FIX #24066
FIX #22647
FIX #22154

---
<!-- 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. -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 12, 2020

⌛ Trying commit 68e5426 with merge 234583e...

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 12, 2020

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2020
@gterzian gterzian 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 Jul 16, 2020
@gterzian gterzian added the S-needs-new-owner The PR has been abandoned by the original author. label Jul 28, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented May 8, 2023

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

@mrobinson mrobinson mentioned this pull request Jul 11, 2023
Merged
3 tasks
@gterzian
Copy link
Copy Markdown
Member Author

gterzian commented Sep 25, 2023

@mrobinson Looking into this again, while adding more info to the message may still be relevant, I think I missed the core issue at hand: the script-thread crashes on an error on the recv on the IPC when trying to get the BC info, indicated the constellation has already exited by then.

So I'd say the numerous crashes noted in the linked-to issues is probably related to to a hole in the clean-shutdown procedure.

Requires further investigation, but some initial notes:

  1. Pipeline doesn't wait on an ACK from script when sending the ExitPipeline msg.
  2. Neither does an EventLoop wait on an ACK from script when sending the ExitScriptThread msg
  3. There is some workflow with the BHM involving a closing boolean, which is useful if the script-thread is blocking inside a script execution, however it's not good enough to ensure clean shutdown, because a script-thread could easily be already handling a message by the time it is set. The constellation does wait on an ACK from the BHM, which implies that the closing flag has been set, however that doesn't prevent script from handling one last message if past the point of checking the flag(it may be that the constellation sends it's reply to the IPC, and then shutdowns before script is scheduled to receive it).

Any way, requires more investigation, but looking at the panic in #24066 it seems clear that the problem is not that the IPC returns None, it's that it returns an error because the sender has already been dropped(such as Io(Custom { kind: ConnectionReset, error: "No senders exist for this port." }) or in Disconnected in #27146).

The last issue linked above #22154 appears unrelated actually(a comment in the issue seems to correctly identify a separate problem with trying to send a message layout after it has shutdown).

mrobinson added a commit to mrobinson/servo that referenced this pull request Oct 4, 2023
@mrobinson
Bootstrap pkg-config and cmake on MacOS
e4b1c34 
These need to be installed in order to build so we can install them via
Homebrew. Do this by simply restoring the Homebrew bootstrapping logic
we had in place previously.

Fixes servo#27171.
@mrobinson mrobinson mentioned this pull request Oct 4, 2023
Merged
3 tasks
github-merge-queue bot pushed a commit that referenced this pull request Oct 5, 2023
@mrobinson
Bootstrap pkg-config and cmake on MacOS (#30497)
aadf48b 
These need to be installed in order to build so we can install them via
Homebrew. Do this by simply restoring the Homebrew bootstrapping logic
we had in place previously.

Fixes #27171.
@gterzian gterzian mentioned this pull request Dec 11, 2023
Closed
@gterzian gterzian closed this Jul 7, 2025
@gterzian gterzian deleted the fix_postmessage branch July 7, 2025 12:57
@mrobinson mrobinson mentioned this pull request Oct 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@asajeffrey asajeffrey asajeffrey left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@nox nox

Labels

S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-new-owner The PR has been abandoned by the original author. S-tests-failed The changes caused existing tests to fail.

Projects

None yet

Milestone

No milestone

5 participants

@gterzian @highfive @bors-servo @asajeffrey @nox