Skip to content

/ servo

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

Leak message ports in Dom, until they are closed #25771

Merged
merged 1 commit into from Feb 24, 2020
Merged

Leak message ports in Dom, until they are closed #25771

merged 1 commit into from Feb 24, 2020

Conversation

@gterzian
Copy link
Member

gterzian commented Feb 15, 2020 •
edited

FIX #25461
FIX #24810
FIX #24488

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

highfive commented Feb 15, 2020

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented Feb 15, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from 16c63b0 to cbae73f Feb 15, 2020
@gterzian gterzian mentioned this pull request Feb 15, 2020
Open
@gterzian
Copy link
Member Author

gterzian commented Feb 15, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Feb 15, 2020

⌛ Trying commit cbae73f with merge af3af1d...
bors-servo added a commit that referenced this pull request Feb 15, 2020
@bors-servo
Auto merge of #25771 - gterzian:leak_message_ports, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
af3af1d 
[WIP Leak message ports in Dom, until they are closed

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

FIX #25461
FIX #24810

---
<!-- 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
Member Author

gterzian commented Feb 15, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Feb 15, 2020

⌛ Trying commit 3b5f41c with merge a2f2886...
bors-servo added a commit that referenced this pull request Feb 15, 2020
@bors-servo
Auto merge of #25771 - gterzian:leak_message_ports, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a2f2886 
[WIP Leak message ports in Dom, until they are closed

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

FIX #25461
FIX #24810

---
<!-- 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
Contributor

bors-servo commented Feb 15, 2020

💔 Test failed - status-taskcluster
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from 3b5f41c to cfff43b Feb 15, 2020
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from cfff43b to 607b409 Feb 15, 2020
@gterzian
Copy link
Member Author

gterzian commented Feb 15, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Feb 15, 2020

⌛ Trying commit 607b409 with merge 0ee3792...
bors-servo added a commit that referenced this pull request Feb 15, 2020
@bors-servo
Auto merge of #25771 - gterzian:leak_message_ports, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0ee3792 
[WIP Leak message ports in Dom, until they are closed

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

FIX #25461
FIX #24810

---
<!-- 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 [WIP Leak message ports in Dom, until they are closed [WIP] Leak message ports in Dom, until they are closed Feb 15, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Feb 15, 2020

💔 Test failed - status-taskcluster
@gterzian gterzian changed the title [WIP] Leak message ports in Dom, until they are closed Leak message ports in Dom, until they are closed Feb 15, 2020
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from 607b409 to df16e42 Feb 15, 2020
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from df16e42 to f256aa2 Feb 15, 2020
@gterzian gterzian mentioned this pull request Feb 15, 2020
Open
@gterzian
Copy link
Member Author

gterzian commented Feb 15, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Feb 15, 2020

⌛ Trying commit f256aa2 with merge ecf2988...
@gterzian
Copy link
Member Author

gterzian commented Feb 20, 2020

@jdm r?
@jdm
Copy link
Member

jdm commented Feb 20, 2020

This is in my queue to review. Apologies about the wait.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2020

☔ The latest upstream changes (presumably #25815) made this pull request unmergeable. Please resolve the merge conflicts.
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from b146b68 to 1368c9e Feb 21, 2020
@jdm
Copy link
Member

jdm commented Feb 21, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

📌 Commit 1368c9e has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

⌛ Testing commit 1368c9e with merge 7e43fdb...
bors-servo added a commit that referenced this pull request Feb 21, 2020
@bors-servo
Auto merge of #25771 - gterzian:leak_message_ports, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7e43fdb 
Leak message ports in Dom, until they are closed

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

FIX #25461
FIX #24810
FIX #24488

---
<!-- 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
Contributor

bors-servo commented Feb 21, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Feb 21, 2020 

  ▶ CRASH [expected OK] /html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events.dedicatedworker.html
  │ 
  │ _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
  │ 
  │ 
  │ 
  │ remove_message_port called on a global not managing the port. (thread WebWorker for http://web-platform.test:8000/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/support/promise-rejection-events.js, at components/script/dom/globalscope.rs:666)
  │ stack backtrace:
  │    0: <servo::backtrace::Print as core::fmt::Debug>::fmt
  │    1: core::fmt::write
  │    2: std::io::Write::write_fmt
  │    3: servo::backtrace::print
  │    4: servo::main::{{closure}}
  │    5: std::panicking::rust_panic_with_hook
  │    6: std::panicking::begin_panic
  │    7: script::dom::globalscope::GlobalScope::remove_message_port
  │    8: <script::dom::globalscope::MessageListener::notify::process_remove_message_port<F> as script::task::TaskOnce>::run_once
  │    9: <T as script::task::TaskBox>::run_box
  │   10: script::dom::workerglobalscope::WorkerGlobalScope::process_event
  │   11: <script::dom::dedicatedworkerglobalscope::DedicatedWorkerGlobalScope as script::dom::abstractworkerglobalscope::WorkerEventLoopMethods>::handle_event
  │   12: script::dom::abstractworkerglobalscope::run_worker_event_loop
  │   13: profile_traits::mem::ProfilerChan::run_with_memory_reporting
  │   14: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   15: _ZN3std9panicking3try7do_call17hd915b3dca089cc12E.llvm.17344723410305141537
  │   16: __rust_maybe_catch_panic
  │   17: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   18: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │   19: std::sys::unix::thread::Thread::new::thread_start
  │   20: _pthread_body
  │   21: _pthread_start
  │ [2020-02-21T16:24:16Z ERROR servo] remove_message_port called on a global not managing the port.
  └ Pipeline failed in hard-fail mode.  Crashing!
@gterzian
leak message ports in dom, until they are closed
Loading status checks…
291ab74
@gterzian gterzian force-pushed the gterzian:leak_message_ports branch from 1368c9e to 291ab74 Feb 22, 2020
@gterzian
Copy link
Member Author

gterzian commented Feb 22, 2020

Ah yes, if the test closes the port, then we can't be sure it's still there when this message is handled. I turned this into a no-op, see https://github.com/servo/servo/pull/25771/files#diff-59d233642d0ce6d687484bdd009e1017R661
@jdm
Copy link
Member

jdm commented Feb 23, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Feb 23, 2020

📌 Commit 291ab74 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

⌛ Testing commit 291ab74 with merge 0ae17e6...
@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 0ae17e6 to master...
@bors-servo bors-servo merged commit 0ae17e6 into servo:master Feb 24, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:leak_message_ports branch Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Intermittent failure in /fetch/security/embedded-credentials.tentative.sub.html
Fix messageport "GC'ed too early"
open.spotify.com panics with 'post_messageport_msg called on an unknown port'
4 participants
@gterzian @highfive @bors-servo @jdm
You can’t perform that action at this time.