Closed Bug 1392067 Opened 7 years ago Closed 6 years ago

onDisconnect not fired when extension tab containing other end gets closed

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified

People

(Reporter: nmaier, Assigned: robwu)

References

Details

Attachments

(3 files)

STR:
0. Firefox Release and Nightly.
1. From somewhere (e.g. the background page) register a runtime.onConnect listener that
  1.1. logs each new connection (port)
  1.2. for each new connection registers a port.onDisconnect listener that logs the disconnection
2. Open a new extension tab with windows.create() and an extension URL
3. runtime.connect() from that extension tab.
4. Observe the connection gets logged via 1.1.
5. Close the tab

Expected:
There should be a log from 1.2 indicating disconnection of the other end was observed.

Actual:
No log -> the onDisconnect listener was not called

---

STR2:
Explicitly disconnect() while handling the "unload" event in the extension tab.

Actual2:
There should be a log from 1.2.

Expected2:
No log -> the onDisconnect listener was not called

---

Related to STR2, any port.postMessage()s/runtime.sendMessage() from "unload" won't get delivered either.

This is a nasty issue, as I am trying to track (in the background page) some state for each |Port| that is connected and since I - due to this bug - have no reliable way to determine if a port was actually closed, my code leaks memory in case onDisconnect was lost.

What's more, not only is no onDisconnect event dispatched, port.postMessage() will continue to be successful even tho the other end is dead, only logging messages like the following, but not throwing an error:
No matching message handler for the given recipient. MessageChannel.jsm:634

Seems to me there is some internal queue for messages that does not get processed to the end, failing to ensure all (IPC) messages are sent in this scenario.
Somewhat messy work-around:

If the port.sender has a Tab with tab.id, then listening on tabs.onRemoved may also show when a tab goes down; the port can be manually marked disconnected from there and any cleanup performed. This is what I am doing right now.
Priority: -- → P3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Not a duplicate, this bug even occurs when an extension is not uninstalled.
Assignee: nobody → rob
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
STR:
1. Load add-on at about:debugging
2. Click on the extension button to open a new window.
3. Close the window.
4. Look at the global browser console (Ctrl-Shift-J).

Expected:
- onConnect log after step 2, onDisconnect log at step 3.

Actual:
- onConnect log after step 2, but no onDisconnect.

Reproduced on:
Firefox 55.0.3 on Linux.
Firefox 57.0a1 (2017-09-13) on Linux.
I debugged a little bit.

The Port logic in ExtensionChild.jsm attempts to send Extension:Port:Disconnect to the "parent", but the message is never sent, because target.sendAsyncMessage throws NS_ERROR_ILLEGAL_VALUE at https://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/toolkit/components/extensions/MessageChannel.jsm#594 . This has nothing to do with the message itself (a simple target.sendAsyncMessage("foo", "bar") also results in the same error).

To fix this error, the port lifetime management should be done in the parent process. That also makes the port logic resilient against crashing content processes (currently when a content process crashes, the ports are kept alive).
(In reply to Nils Maier [:nmaier] from comment #1)
> Somewhat messy work-around:
> 
> If the port.sender has a Tab with tab.id, then listening on tabs.onRemoved
> may also show when a tab goes down; the port can be manually marked
> disconnected from there and any cleanup performed. This is what I am doing
> right now.

It can't solve this issue in every cases because tabs.onRemoved has also some issue.
For example, tabs.onRemoved is not fired for browserAction in Firefox Android 59 (https://bugzilla.mozilla.org/show_bug.cgi?id=1418799). I also faced some cases where tabs.onRemoved was not fired on window close in Firefox Quantum 57.
The best solution I have found is to both listen to Port.onDisconnect and call Port.postMessage inside a try catch.
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review240370

This is a very confusing patch.  It is using and referring to concepts without defining them, and has a complex ad-hock data structure of nested Maps and Sets that may technically do the right thing, but is very hard to reason about.  Perhaps turning these concepts into simple, concrete and explicitly named classes would help.

Tests have another set of issues, and seem to fail on Try.

::: browser/components/extensions/test/browser/browser-common.ini:84
(Diff revision 1)
>  [browser_ext_contextMenus_onclick.js]
>  [browser_ext_contextMenus_radioGroups.js]
>  [browser_ext_contextMenus_uninstall.js]
>  [browser_ext_contextMenus_urlPatterns.js]
> +[browser_ext_crash_disconnect_port.js]
> +skip-if = !e10s

Please add a comment explaining why this is skipped.

And this needs to be a browser test because you want to crash the tab?  I guess that's fine, but it would be good to have a regular mochitest (if possible) under `toolkit/` to also test this on android.

It can simply close the tab/window, which was original STR for this bug anyway.

::: browser/components/extensions/test/browser/browser_ext_crash_disconnect_port.js:11
(Diff revision 1)
> +  return ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"],
> +      "content_scripts": [{
> +        "js": ["contentscript.js"],
> +        // Note: Not "http://mochi.test:8888/" because of bugzil.la/1362809 .

I believe you can use `example.com`, without the need for port.

::: browser/components/extensions/test/browser/browser_ext_crash_disconnect_port.js:33
(Diff revision 1)
> +          // port.onDisconnect should fire. To avoid inexplicable test time-outs if this does not
> +          // happen, force the test to resume (and fail) if port.onDisconnect is not called soon.
> +          // eslint-disable-next-line mozilla/no-arbitrary-setTimeout

Are you expecting higher-than-usual test timeouts here?  If so, why?

Otherwise, we don't do this.  Virtually every test depends on multiple events firing, so the code would be littered with `setTimeout()`s if we did.

If you are worried about debugging such timeouts on CI, you can insert additional logging statements where appropriate.

::: browser/components/extensions/test/browser/browser_ext_crash_disconnect_port.js:67
(Diff revision 1)
> +          });
> +        }
> +        browser.test.onMessage.addListener(msg => {
> +          if (msg === "connect_from_tab_to_bg") {
> +            let port = browser.runtime.connect({name: "tab_to_bg"});
> +            addOnDisconnectListener(port);

What is this for?  When would this do anything if you are killing the tab process?

::: browser/components/extensions/test/browser/browser_ext_crash_disconnect_port.js:68
(Diff revision 1)
> +        }
> +        browser.test.onMessage.addListener(msg => {
> +          if (msg === "connect_from_tab_to_bg") {
> +            let port = browser.runtime.connect({name: "tab_to_bg"});
> +            addOnDisconnectListener(port);
> +          } else if (msg === "ping") {

This is unused.

::: browser/components/extensions/test/browser/browser_ext_crash_disconnect_port.js:99
(Diff revision 1)
> +  is(page, "background", "expected onDisconnect in background after tab crash");
> +  await extension.unload();
> +
> +  BrowserTestUtils.removeTab(tab);
> +});
> +

For something this simple, these two tests seem very hard to follow, probably because the same extension is used in both.

It might be slightly longer code if you separated them, bit would probably be easier to understand.

::: toolkit/components/extensions/ExtensionParent.jsm:208
(Diff revision 1)
> -    this.ports = new DefaultMap(() => new Map());
> +    this.ports = new DefaultMap(() => new Set());
> +    this.portsById = new Map();

Please document for structure/purpose of these maps (partly per-existing, I know).

::: toolkit/components/extensions/ExtensionParent.jsm:352
(Diff revision 1)
>  
>    /**
>     * @param {object} recipient An object that was passed to
>     *     `MessageChannel.sendMessage`.
>     * @param {Extension} extension
> -   * @returns {object|null} The message manager matching the recipient if found.
> +   * @returns {Array} A pair of two optional values:

Why the array?  Please return an object instead, and probably rename the method as appropriate.

::: toolkit/components/extensions/ExtensionParent.jsm:403
(Diff revision 1)
> +  },
> +
> +  // Register an extension port, so that the port is always disconnected when a
> +  // message manager is closed, even if the content side does not send an
> +  // explicit Extension:Port:Disconnect message.
> +  setupExtensionPortProxy(portId, senderElem, senderMM, receiverElem, receiverMM) {

What's a PortProxy?  How is it different from a PortDescriptor?  And how is that different from Port?  You didn't document either, and seem to use them interchangeably, it's really confusing.

::: toolkit/components/extensions/ExtensionParent.jsm:405
(Diff revision 1)
> +      // No need to register the proxy, because if the sender's message manager
> +      // disconnects, then so does the recipient's message manager.

I don't understand why this is not needed.  Without registering the port, none of the event handlers (message-manager-disconnect, SwapDocShell) can do anything.

::: toolkit/components/extensions/ExtensionParent.jsm:414
(Diff revision 1)
> +    if (this.portsById.has(portId)) {
> +      throw new Error(`Extension port IDs may not be re-used: ${portId}`);
> +    }
> +    // The source of Extension:Connect is always inside a <browser>, whereas
> +    // the recipient can be a process (and not be associated with a <browser>).
> +    if (receiverElem) {

Can the Elem be anything other than a browser?  Please rename if not.

::: toolkit/components/extensions/ExtensionParent.jsm:433
(Diff revision 1)
> +
> +  disposeExtensionPortProxy(portId) {
> +    let portDescriptor = this.portsById.get(portId);
> +    if (portDescriptor) {
> +      this.deletePortDescriptor(portDescriptor.senderMM, portDescriptor);
> +      this.deletePortDescriptor(portDescriptor.receiverMM, portDescriptor);

Why do we need to track by receiverMM as well?
Attachment #8965812 - Flags: review?(tomica) → review-
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review243894
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review240370

> What's a PortProxy?  How is it different from a PortDescriptor?  And how is that different from Port?  You didn't document either, and seem to use them interchangeably, it's really confusing.

`PortDescriptor` was the data structure that described the port.
"ExtensionPortProxy" registers events and the data structure to support proxying extension Ports (of `ExtensionChild.jsm`).

I've now created a separate `ExtensionPortProxy` class that contains the members of `ExtensionPortProxy`, and moved most logic from `ProxyMessenger` to instance methods of `ExtensionPortProxy`. The `setupExtensionPortProxy` method was removed and inlined in the only caller (`receiveMessage` in `ProxyMessenger`).

> I don't understand why this is not needed.  Without registering the port, none of the event handlers (message-manager-disconnect, SwapDocShell) can do anything.

I have completely rewritten the comment (now located after the `if (messageManager === "Extension:Connect") {` line).

When the message manager disconnects, then one of the message managers should receive the `Extension:Port:Disconnect` message.
If the message managers are identical, then there is no message manager that could receive this message, so there is no point in tracking them.

> Why do we need to track by receiverMM as well?

`receiverMM` can also be swapped, e.g. when a port is connected to a tab (via `browser.tabs.connect`), and the tab is detached.

I wanted to add a test for scenario too, similar to the one for https://bugzil.la/1448674 , but when I manually tried to kill an extension process, the background page did not restart. So I think that killed extension processes is not really a supported use case yet, and therefore I did not add that test in the end.

I wouldn't mind filing a bug and adding such a test if you believe that this scenario is important.
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review244564

::: browser/components/extensions/test/browser/browser-common.ini:85
(Diff revision 2)
>  [browser_ext_contextMenus_onclick.js]
>  [browser_ext_contextMenus_radioGroups.js]
>  [browser_ext_contextMenus_uninstall.js]
>  [browser_ext_contextMenus_urlPatterns.js]
> +[browser_ext_crash_disconnect_port.js]
> +skip-if = !e10s # the tab's process is killed during the test. Without e10s the parent process would die too.

Please add a test that simply closes the tab, and tests on android as well.

::: browser/components/extensions/test/browser/browser_ext_crash_disconnect_port.js:91
(Diff revision 2)
> +  // chance to send an "Extension:Port:Disconnect" message.
> +  await BrowserTestUtils.crashBrowser(tab.linkedBrowser);
> +  await extension.awaitMessage("port_disconnected");
> +  BrowserTestUtils.removeTab(tab);
> +  await extension.unload();
> +});

Nice, the code turned out both simpler _and_ shorter.

::: toolkit/components/extensions/ExtensionChild.jsm:132
(Diff revision 2)
>   * @param {BaseContext} context The context that owns this port.
>   * @param {nsIMessageSender} senderMM The message manager to send messages to.
>   * @param {Array<nsIMessageListenerManager>} receiverMMs Message managers to
>   *     listen on.
>   * @param {string} name Arbitrary port name as defined by the addon.
> - * @param {string} id An ID that uniquely identifies this port's channel.
> + * @param {integer} id An ID that uniquely identifies this port's channel.

nit: JSDocs (and typescript, as I'm finding out these days) only know about `{number}`.

::: toolkit/components/extensions/ExtensionParent.jsm:180
(Diff revision 2)
>        }
>      }
>    }
>  }();
>  
> +// A proxy for extension ports between two DISTINCT message managers.

Is the "distinct" thing here a corectnes requirement, or an optimization?  What are the cases when we have the same messeangers anyway?

::: toolkit/components/extensions/ExtensionParent.jsm:212
(Diff revision 2)
> +    this._unregisterFromMessageManager(this.receiverMM);
> +  }
> +
> +  _unregisterFromMessageManager(messageManager) {
> +    let ports = ProxyMessenger.ports.get(messageManager);
> +    ports.delete(messageManager);

This needs to be `ports.delete(this)`.

::: toolkit/components/extensions/ExtensionParent.jsm:295
(Diff revision 2)
>        if (this.ports.has(subject)) {
>          let ports = this.ports.get(subject);
>          this.ports.delete(subject);
> -
> -        for (let [portId, {sender, recipient, receiverMM}] of ports.entries()) {
> -          recipient.portId = portId;
> +        for (let port of ports) {
> +          MessageChannel.sendMessage(port.getOtherMessageManager(subject), "Extension:Port:Disconnect", null, {
> +            // The handler of Extension:Port:Disconnect uses the sender

nit: a comment this long is either "code smell" or not really needed, could probably go with something simple like "we only need identity of the recipient to handle this particular message on the other end".
Attachment #8965812 - Flags: review?(tomica)
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review244564

> Please add a test that simply closes the tab, and tests on android as well.

That is of no use. Firstly, the bug only happens upon closing a window, not a tab (I checked on Android too, which does not support windows in the first place).
Secondly, the bug happens when the disconnection message is not delivered via the message manager. Killing the content process seems the most reliable way to simulate this scenario.

(Closing a window is currently also a way to reproduce the bug, but I don't regard that as a deterministic way of reproduction)

> Is the "distinct" thing here a corectnes requirement, or an optimization?  What are the cases when we have the same messeangers anyway?

Optimization. I've rephrased this comment to make that more obvious. The optimization was already explained at the line in `receiveMessage` with `if (messageName == "Extension:Connect") {`.

> This needs to be `ports.delete(this)`.

Woops, thanks for the catch.

> nit: a comment this long is either "code smell" or not really needed, could probably go with something simple like "we only need identity of the recipient to handle this particular message on the other end".

I have omitted some details and shortened the comment by three lines.
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review244564

> That is of no use. Firstly, the bug only happens upon closing a window, not a tab (I checked on Android too, which does not support windows in the first place).
> Secondly, the bug happens when the disconnection message is not delivered via the message manager. Killing the content process seems the most reliable way to simulate this scenario.
> 
> (Closing a window is currently also a way to reproduce the bug, but I don't regard that as a deterministic way of reproduction)

The original STR mentions a tab closing, not a window.

But in any case, if this _is_ reproducible by closing a window, even intermittently, that's IMO a valid test case because it exercises a different situation (even if the current implementation code treats them exactly the same).  Killing the process might be the most reliable way to make that happen, but it's obviously not the only STR that makes it happen.
Attachment #8965812 - Flags: review?(tomica)
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review244564

> The original STR mentions a tab closing, not a window.
> 
> But in any case, if this _is_ reproducible by closing a window, even intermittently, that's IMO a valid test case because it exercises a different situation (even if the current implementation code treats them exactly the same).  Killing the process might be the most reliable way to make that happen, but it's obviously not the only STR that makes it happen.

The original STR mentions closing a tab, but that tab is the only tab in the new window, so effectively it is closing a window.

I have added the window test as requested (still not an Android test since browser.windows.create does not exist on Android, and the bug cannot be reproduced on Android).

> Optimization. I've rephrased this comment to make that more obvious. The optimization was already explained at the line in `receiveMessage` with `if (messageName == "Extension:Connect") {`.

Actually both. I said "optimization" because not creating an `ExtensionPortProxy` when the message managers are identical is an optimization.

But if I allow the instantiation of `ExtensionPortProxy` with identical message managers, then I have to update the `replaceMessageManager` method and the `message-manager-disconnect` observer to avoid using stale message managers (for correctness).

Either way I have to check for message manager equality, and ensuring distinctness is conceptually easier to understand and maintain.
Comment on attachment 8965812 [details]
Bug 1392067 - Disconnect open extension ports when the message manager goes away

https://reviewboard.mozilla.org/r/234654/#review245326

Thanks.

::: browser/components/extensions/test/browser/browser-common.ini:215
(Diff revisions 3 - 4)
>  [browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js]
>  [browser_ext_webNavigation_urlbar_transitions.js]
>  [browser_ext_windows.js]
>  [browser_ext_windows_create.js]
>  tags = fullscreen
> +[browser_ext_windows_create_and_remove.js]

nit: I would probably group the two tests in this bug under related names, something like "browser_ext_port_disconnect_crash" and "_window_close" or something.
Attachment #8965812 - Flags: review?(tomica) → review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/9d8b5d05ff0e
Disconnect open extension ports when the message manager goes away r=zombie
Backed out changeset 9d8b5d05ff0e (bug 1392067) for Browser chrome failure on /builds/worker/workspace/build/src/obj-firefox/dist/include/js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=175561370&repo=autoland&lineNumber=2661

INFO - TEST-OK | browser/components/extensions/test/browser/browser_ext_popup_shutdown.js | took 2200ms
[task 2018-04-25T16:54:16.715Z] 16:54:16     INFO - checking window state
[task 2018-04-25T16:54:16.812Z] 16:54:16     INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_port_disconnect_on_crash.js
[task 2018-04-25T16:54:17.783Z] 16:54:17     INFO - GECKO(1048) | Et tu, Brute?
[task 2018-04-25T16:54:17.785Z] 16:54:17     INFO - GECKO(1048) | AddressSanitizer:DEADLYSIGNAL
[task 2018-04-25T16:54:17.785Z] 16:54:17     INFO - GECKO(1048) | =================================================================
[task 2018-04-25T16:54:17.787Z] 16:54:17    ERROR - GECKO(1048) | ==1101==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f5013c2753d bp 0x7ffe41a28600 sp 0x7ffe41a27c60 T0)
[task 2018-04-25T16:54:17.790Z] 16:54:17     INFO - GECKO(1048) | ==1101==The signal is caused by a READ memory access.
[task 2018-04-25T16:54:17.790Z] 16:54:17     INFO - GECKO(1048) | ==1101==Hint: address points to the zero page.
[task 2018-04-25T16:54:19.166Z] 16:54:19     INFO - GECKO(1048) |     #0 0x7f5013c2753c in setDouble /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:447:15
[task 2018-04-25T16:54:19.167Z] 16:54:19     INFO - GECKO(1048) |     #1 0x7f5013c2753c in setDouble /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:1342
[task 2018-04-25T16:54:19.167Z] 16:54:19     INFO - GECKO(1048) |     #2 0x7f5013c2753c in js::ctypes::ConvertToJS(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, void*, bool, bool, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/ctypes/CTypes.cpp:3315
[task 2018-04-25T16:54:19.167Z] 16:54:19     INFO - GECKO(1048) |     #3 0x7f5013c25623 in js::ctypes::PointerType::ContentsGetter(JSContext*, JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/ctypes/CTypes.cpp:5446:8
[task 2018-04-25T16:54:19.168Z] 16:54:19     INFO - GECKO(1048) |     #4 0x7f5013c8744c in CallNonGenericMethod<&js::ctypes::PointerType::IsPointer, &js::ctypes::PointerType::ContentsGetter> /builds/worker/workspace/build/src/obj-firefox/dist/include/js/CallNonGenericMethod.h:100:16
[task 2018-04-25T16:54:19.168Z] 16:54:19     INFO - GECKO(1048) |     #5 0x7f5013c8744c in js::ctypes::Property<&js::ctypes::PointerType::IsPointer, &js::ctypes::PointerType::ContentsGetter>::Fun(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/ctypes/CTypes.cpp:227
[task 2018-04-25T16:54:19.198Z] 16:54:19     INFO - GECKO(1048) |     #6 0x7f5013cfd367 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:280:15
[task 2018-04-25T16:54:19.199Z] 16:54:19     INFO - GECKO(1048) |     #7 0x7f5013cfd367 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
[task 2018-04-25T16:54:19.200Z] 16:54:19     INFO - GECKO(1048) |     #8 0x7f5013cff843 in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:516:12
[task 2018-04-25T16:54:19.201Z] 16:54:19     INFO - GECKO(1048) |     #9 0x7f5013cff843 in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
[task 2018-04-25T16:54:19.201Z] 16:54:19     INFO - GECKO(1048) |     #10 0x7f5013cff843 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:650
[task 2018-04-25T16:54:19.279Z] 16:54:19     INFO - GECKO(1048) |     #11 0x7f5014c68ee0 in CallGetter /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2176:16
[task 2018-04-25T16:54:19.279Z] 16:54:19     INFO - GECKO(1048) |     #12 0x7f5014c68ee0 in GetExistingProperty<js::AllowGC::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2229
[task 2018-04-25T16:54:19.279Z] 16:54:19     INFO - GECKO(1048) |     #13 0x7f5014c68ee0 in NativeGetPropertyInline<js::AllowGC::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2442
[task 2018-04-25T16:54:19.280Z] 16:54:19     INFO - GECKO(1048) |     #14 0x7f5014c68ee0 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2478
[task 2018-04-25T16:54:19.283Z] 16:54:19     INFO - GECKO(1048) |     #15 0x7f50148dc2ce in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1640:12
[task 2018-04-25T16:54:19.284Z] 16:54:19     INFO - GECKO(1048) |     #16 0x7f50148dc2ce in js::ForwardingProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:154
[task 2018-04-25T16:54:19.301Z] 16:54:19     INFO - GECKO(1048) |     #17 0x7f50148a03e0 in js::CrossCompartmentWrapper::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:225:23
[task 2018-04-25T16:54:19.304Z] 16:54:19     INFO - GECKO(1048) |     #18 0x7f50148b2734 in getInternal /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:351:21
[task 2018-04-25T16:54:19.305Z] 16:54:19     INFO - GECKO(1048) |     #19 0x7f50148b2734 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:361
[task 2018-04-25T16:54:19.305Z] 16:54:19     INFO - GECKO(1048) |     #20 0x7f5013d08d52 in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1639:16
[task 2018-04-25T16:54:19.305Z] 16:54:19     INFO - GECKO(1048) |     #21 0x7f5013d08d52 in GetProperty /builds/worker/workspace/build/src/js/src/vm/JSObject.h:800
[task 2018-04-25T16:54:19.306Z] 16:54:19     INFO - GECKO(1048) |     #22 0x7f5013d08d52 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4395
[task 2018-04-25T16:54:19.311Z] 16:54:19     INFO - GECKO(1048) |     #23 0x7f5013ceaedc in GetPropertyOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:213:12
[task 2018-04-25T16:54:19.311Z] 16:54:19     INFO - GECKO(1048) |     #24 0x7f5013ceaedc in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2803
[task 2018-04-25T16:54:19.315Z] 16:54:19     INFO - GECKO(1048) |     #25 0x7f5013cce557 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:417:12
[task 2018-04-25T16:54:19.315Z] 16:54:19     INFO - GECKO(1048) |     #26 0x7f5013cfd0e5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:489:15
[task 2018-04-25T16:54:19.316Z] 16:54:19     INFO - GECKO(1048) |     #27 0x7f5013ce7e58 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:522:12
[task 2018-04-25T16:54:19.316Z] 16:54:19     INFO - GECKO(1048) |     #28 0x7f5013ce7e58 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084
[task 2018-04-25T16:54:19.317Z] 16:54:19     INFO - GECKO(1048) |     #29 0x7f5013cce557 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:417:12
[task 2018-04-25T16:54:19.319Z] 16:54:19     INFO - GECKO(1048) |     #30 0x7f5013d00804 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:700:15
[task 2018-04-25T16:54:19.340Z] 16:54:19     INFO - GECKO(1048) |     #31 0x7f5013d755ef in ExecuteInExtensibleLexicalEnvironment(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/src/builtin/Eval.cpp:460:12
[task 2018-04-25T16:54:19.342Z] 16:54:19     INFO - GECKO(1048) |     #32 0x7f5013d74fe9 in js::ExecuteInGlobalAndReturnScope(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JSObject*>) /builds/worker/workspace/build/src/js/src/builtin/Eval.cpp:478:10
[task 2018-04-25T16:54:19.350Z] 16:54:19     INFO - GECKO(1048) |     #33 0x7f500acc37ae in nsMessageManagerScriptExecutor::LoadScriptInternal(JS::Handle<JSObject*>, nsTSubstring<char16_t> const&, bool) /builds/worker/workspace/build/src/dom/base/nsFrameMessageManager.cpp:1329:17
[task 2018-04-25T16:54:19.412Z] 16:54:19     INFO - GECKO(1048) |     #34 0x7f500f08807b in RecvLoadRemoteScript /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:2298:3
[task 2018-04-25T16:54:19.412Z] 16:54:19     INFO - GECKO(1048) |     #35 0x7f500f08807b in non-virtual thunk to mozilla::dom::TabChild::RecvLoadRemoteScript(nsTString<char16_t> const&, bool const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp
[task 2018-04-25T16:54:19.463Z] 16:54:19     INFO - GECKO(1048) |     #36 0x7f50094fd689 in mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBrowserChild.cpp:4374:20
[task 2018-04-25T16:54:19.524Z] 16:54:19     INFO - GECKO(1048) |     #37 0x7f5008f1aae6 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:5080:28
[task 2018-04-25T16:54:19.541Z] 16:54:19     INFO - GECKO(1048) |     #38 0x7f5008dbcace in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2141:25
[task 2018-04-25T16:54:19.542Z] 16:54:19     INFO - GECKO(1048) |     #39 0x7f5008db9a96 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2071:17
[task 2018-04-25T16:54:19.542Z] 16:54:19     INFO - GECKO(1048) |     #40 0x7f5008dbb24c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1917:5
[task 2018-04-25T16:54:19.542Z] 16:54:19     INFO - GECKO(1048) |     #41 0x7f5008dbb8a8 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1950:15
[task 2018-04-25T16:54:19.558Z] 16:54:19     INFO - GECKO(1048) |     #42 0x7f5007eadb21 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:337:32
[task 2018-04-25T16:54:19.558Z] 16:54:19     INFO - GECKO(1048) |     #43 0x7f5007ecc919 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1090:14
[task 2018-04-25T16:54:19.560Z] 16:54:19     INFO - GECKO(1048) |     #44 0x7f5007ee8350 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
[task 2018-04-25T16:54:19.561Z] 16:54:19     INFO - GECKO(1048) |     #45 0x7f5008dc4626 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:125:5
[task 2018-04-25T16:54:19.577Z] 16:54:19     INFO - GECKO(1048) |     #46 0x7f5008d18e59 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
[task 2018-04-25T16:54:19.578Z] 16:54:19     INFO - GECKO(1048) |     #47 0x7f5008d18e59 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
[task 2018-04-25T16:54:19.579Z] 16:54:19     INFO - GECKO(1048) |     #48 0x7f5008d18e59 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
[task 2018-04-25T16:54:19.587Z] 16:54:19     INFO - GECKO(1048) |     #49 0x7f500f7c0d6a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
[task 2018-04-25T16:54:19.589Z] 16:54:19     INFO - GECKO(1048) |     #50 0x7f5013a3404b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22
[task 2018-04-25T16:54:19.589Z] 16:54:19     INFO - GECKO(1048) |     #51 0x7f5008d18e59 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
[task 2018-04-25T16:54:19.592Z] 16:54:19     INFO - GECKO(1048) |     #52 0x7f5008d18e59 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
[task 2018-04-25T16:54:19.592Z] 16:54:19     INFO - GECKO(1048) |     #53 0x7f5008d18e59 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
[task 2018-04-25T16:54:19.593Z] 16:54:19     INFO - GECKO(1048) |     #54 0x7f5013a33a10 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:719:34
[task 2018-04-25T16:54:19.614Z] 16:54:19     INFO - GECKO(1048) |     #55 0x4f1875 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
[task 2018-04-25T16:54:19.620Z] 16:54:19     INFO - GECKO(1048) |     #56 0x4f1875 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
[task 2018-04-25T16:54:19.692Z] 16:54:19     INFO - GECKO(1048) |     #57 0x7f502837e82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
[task 2018-04-25T16:54:19.692Z] 16:54:19     INFO - GECKO(1048) |     #58 0x420f48 in _start (/builds/worker/workspace/build/application/firefox/firefox+0x420f48)
[task 2018-04-25T16:54:19.692Z] 16:54:19     INFO - GECKO(1048) | AddressSanitizer can not provide additional info.
[task 2018-04-25T16:54:19.692Z] 16:54:19     INFO - GECKO(1048) | SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:447:15 in setDouble
[task 2018-04-25T16:54:19.692Z] 16:54:19     INFO - GECKO(1048) | ==1101==ABORTING
[task 2018-04-25T16:54:19.770Z] 16:54:19     INFO - GECKO(1048) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x16007F,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
[task 2018-04-25T16:54:19.772Z] 16:54:19     INFO - GECKO(1048) | Crash cleaned up
[task 2018-04-25T16:54:20.078Z] 16:54:20     INFO - GECKO(1048) | about:tabcrashed loaded and ready
[task 2018-04-25T16:54:22.705Z] 16:54:22     INFO - GECKO(1048) | Et tu, Brute?
[task 2018-04-25T16:54:22.705Z] 16:54:22     INFO - GECKO(1048) | AddressSanitizer:DEADLYSIGNAL
[task 2018-04-25T16:54:22.705Z] 16:54:22     INFO - GECKO(1048) | =================================================================
[task 2018-04-25T16:54:22.708Z] 16:54:22    ERROR - GECKO(1048) | ==1481==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f1e2e02753d bp 0x7ffeb9572360 sp 0x7ffeb95719c0 T0)

Push with the failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9d8b5d05ff0e6c25cd6911b9d44ea0c8499cd4f8

Backout:
https://hg.mozilla.org/integration/autoland/rev/63ecf1e96a20c56625258dfa5136dc18e016c100
Flags: needinfo?(rob)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a808d4f39df
Disconnect open extension ports when the message manager goes away
I added || !crashreporter to skip the test on ASAN builds, because the crash is an expected part of the test.
Flags: needinfo?(rob)
https://hg.mozilla.org/mozilla-central/rev/9a808d4f39df
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Rob Wu [:robwu] from comment #25)
> I added || !crashreporter to skip the test on ASAN builds, because the crash
> is an expected part of the test.

PS. I used this because I saw it being used at https://hg.mozilla.org/mozilla-central/rev/ce3fea0cb25b and explained at https://bugzilla.mozilla.org/show_bug.cgi?id=1126089#c48 .
Attached image on disconnect.gif
Reproduced on Firefox 60.0a1 (20180303220113) in Windows 10 64Bit.
Retested and verified on Firefox 61.0a1 (20180503100146) on Windows 10 x64 and MacOS 10.13.3
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.