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

open.spotify.com panics with 'post_messageport_msg called on an unknown port' #24810

Closed
ferjm opened this issue Nov 21, 2019 · 5 comments
Closed

open.spotify.com panics with 'post_messageport_msg called on an unknown port' #24810

ferjm opened this issue Nov 21, 2019 · 5 comments

Comments

@ferjm
Copy link
Member

@ferjm ferjm commented Nov 21, 2019

https://open.spotify.com

post_messageport_msg called on an unknown port. (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at components/script/dom/globalscope.rs:462)
stack backtrace:
   0: backtrace::backtrace::trace_unsynchronized
   1: backtrace::backtrace::trace
   2: backtrace::capture::Backtrace::create
   3: backtrace::capture::Backtrace::new
   4: servo::main::{{closure}}
   5: std::panicking::rust_panic_with_hook
   6: std::panicking::begin_panic
   7: script::dom::globalscope::GlobalScope::post_messageport_msg
   8: script::dom::messageport::MessagePort::post_message_impl
   9: <script::dom::messageport::MessagePort as script::dom::bindings::codegen::Bindings::MessagePortBinding::MessagePortBinding::MessagePortMethods>::PostMessage_
  10: script::dom::bindings::codegen::Bindings::MessagePortBinding::MessagePortBinding::postMessage::{{closure}}
  11: core::ops::function::FnOnce::call_once
  12: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  13: std::panicking::try::do_call
  14: __rust_maybe_catch_panic
  15: std::panicking::try
  16: std::panic::catch_unwind
  17: mozjs::panic::wrap_panic
  18: script::dom::bindings::codegen::Bindings::MessagePortBinding::MessagePortBinding::postMessage
  19: CallJitMethodOp
  20: script::dom::bindings::utils::generic_call
  21: script::dom::bindings::utils::generic_method
  22: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  23: _ZL9InterpretP9JSContextRN2js8RunStateE
  24: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
  25: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  26: _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EE
  27: _Z20JS_CallFunctionValueP9JSContextN2JS6HandleIP8JSObjectEENS2_INS1_5ValueEEERKNS1_16HandleValueArrayENS1_13MutableHandleIS6_EE
  28: mozjs::rust::wrappers::JS_CallFunctionValue
  29: script::dom::bindings::codegen::Bindings::WindowBinding::FrameRequestCallback::Call
  30: script::dom::bindings::codegen::Bindings::WindowBinding::FrameRequestCallback::Call__
  31: script::dom::document::AnimationFrameCallback::call
  32: script::dom::document::Document::run_the_animation_frame_callbacks
  33: script::script_thread::ScriptThread::handle_tick_all_animations
  34: script::script_thread::ScriptThread::handle_msg_from_constellation
  35: script::script_thread::ScriptThread::handle_msgs::{{closure}}
  36: script::script_thread::ScriptThread::profile_event
  37: script::script_thread::ScriptThread::handle_msgs
  38: script::script_thread::ScriptThread::start
  39: <script::script_thread::ScriptThread as script_traits::ScriptThreadFactory>::create::{{closure}}::{{closure}}
  40: profile_traits::mem::ProfilerChan::run_with_memory_reporting
  41: <script::script_thread::ScriptThread as script_traits::ScriptThreadFactory>::create::{{closure}}
  42: std::sys_common::backtrace::__rust_begin_short_backtrace
  43: std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}
  44: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  45: std::panicking::try::do_call
  46: __rust_maybe_catch_panic
  47: std::panicking::try
  48: std::panic::catch_unwind
  49: std::thread::Builder::spawn_unchecked::{{closure}}
  50: core::ops::function::FnOnce::call_once{{vtable.shim}}
  51: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  52: std::sys::unix::thread::Thread::new::thread_start
  53: _pthread_body
  54: _pthread_start
@ferjm ferjm added the I-panic label Nov 21, 2019
@gterzian
Copy link
Member

@gterzian gterzian commented Nov 21, 2019

Inside an animation frame callback. I wonder if this is similar to #24591
It would be useful to know at which point in the lifecycle of the page the animation frame callback is requested.

@gterzian
Copy link
Member

@gterzian gterzian commented Nov 21, 2019

Can this be reproduced by running the following script(I'm currently building another branch)?

<script>
  var chan = new MessageChannel();
  window.addEventListener("unload", () => {
    window.requestAnimationFrame(() => {
      chan.port1.postMessage("Test");
    });
  });
  window.location.assign("about:blank");
</script>
@gterzian
Copy link
Member

@gterzian gterzian commented Nov 21, 2019

Ok I don't think it's related to #24591, because it still happens on a branch that fixes that problem. I ran the above script on that branch too, and there were no problems, but that seems to be unrelated.

@gterzian
Copy link
Member

@gterzian gterzian commented Nov 22, 2019

So what's happening if the port is Gc'ed, although it has been started before(so an event-handler has been set on it). It's hard to tell what is exactly happening without looking at the code.

I would say a port to which an event-handler as been set should not be GC'ed, but it's hard to tell exactly why it is GC'ed. It's not normally GC'ed in such a case, otherwise all tests would fail.

There doesn't seem to be any navigation going-on, or a transfer of the port. The only notable thing is the animation frame callback.

Also to be clear, the port that is GC'ed is the one entangled with the one on which postMessage is called inside the frame callback.

So what happens is:

  1. Port 1 is GC'ed for unknown reason.
  2. Port 2, entangled with it, is therefore also removed, although the JS might still hold on to it(this is somewhat questionable, we could leave it managed until the JS drops it, or at least not panic when the JS tries to still use the port whose entangled has been GC'ed).
  3. Port 2 tries to message with port 1.

So if we don't remove the port at 2, we avoid the panic, however the message is not going to be received either. So the issues is really that the first port is GC'ed quite early it seems.

In any case, it's probably a good idea to not remove the port just because the entangled one has been GC'ed, since the JS could still keep a reference to it and use it(or we should remove the panics upon use).

However I don't think the messages that are sent are intended to be lost, so the why of the GC of the entangled is still an open question.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jan 9, 2020

Ah, so I came across something similar when deciding how i would implement the webxr reset event.

Firefox has a special KeepAliveIfEventListnersFor helper that's used for this kind of thing. We should build that.

See immersive-web/webxr#845 (comment)

bors-servo added a commit that referenced this issue Feb 15, 2020
[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 added a commit that referenced this issue Feb 15, 2020
[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 added a commit that referenced this issue Feb 15, 2020
[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 added a commit that referenced this issue Feb 15, 2020
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 added a commit that referenced this issue Feb 21, 2020
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. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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