Closed Bug 1655624 Opened 10 months ago Closed 10 months ago

Actor 'Conduits' destroyed before query 'RuntimeMessage' was resolved

Categories

(WebExtensions :: General, defect, P1)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox-esr68 unaffected, firefox-esr78 unaffected, firefox79 unaffected, firefox80 fixed, firefox81 fixed)

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- fixed
firefox81 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

conduit-query-messaging.zip
1.68 KB, application/zip
Details
Bug 1655624 - Improve reliability of onMessage's error handling
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review

After taking a very good look at bug 1651274, I figured that the test is failing intermittently because a message has two recipients, and that destroying one recipient results in the destruction of the whole message channel, even though the other recipient is eligible for sending a reply to the sender.

I created a deterministic test case, and confirmed that the error from that bug is observed.

STR:

  1. Load extension.
  2. Click the extension button.
  3. Wait a second for a dialog to appear.

Expected:

  • "Got result: BG"
    (Firefox 78.0.2, Firefox Beta 79.0 and Chromium 86.0.4215.0)

Actual:

  • "ERROR: Actor 'Conduits' destroyed before query 'RuntimeMessage' was resolved"
    ( Firefox Nightly 80.0a1, buildID 20200719090414 )

mozregression points to https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=145740ac5efede9a7bcf4c6e1db4fe07bc24ecaf&tochange=5907260fa4688e06eb4b07982ed23fc2d476d8e3

This regression is caused by bug 1649477, could you take a look?

Flags: needinfo?(nika)

The regression is unlikely to actually be caused by that code. The main change in that bug is that I changed the name of the exception to be nicer (which is why that error wouldn't appear in an earlier version - it would've instead appeared as a NS_ERROR_NOT_AVAILABLE). I don't believe I made any meaningful change to the behaviour there. I am doubtful that the new prettier exception message is the cause of the regression.

Flags: needinfo?(nika)

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

I am doubtful that the new prettier exception message is the cause of the regression.

You'd think that, but hey:
https://searchfox.org/mozilla-central/rev/cffd9b5302/toolkit/components/extensions/ConduitsParent.jsm#291-292

:crylaugh:

I'll start with this, by at least writing a unit test.

Assignee: nobody → rob
Severity: -- → S2
Status: NEW → ASSIGNED
Priority: -- → P1

Bug 1655624 happened because the format of an internal error changed,
which caused an internal error to be propagated unexpectedly.
This patch fixes the issue by only propagating errors that are known to
originate from extensions, plus a regression test.

This patch also fixes a few other issues:

  • Internal errors are redacted to "An unexpected error occurred",
    which partially fixes bug 1643176.

  • Fix minor regression in void rejections: Prior to bug 1583484, an
    onMessage handler that rejected with a void value would cause
    sendMessage to reject. Since bug 1583484 the promise is not rejected,
    as the error is inadvertently ignored due to a runtime error:
    "TypeError: can't access property "result", err is undefined".

  • Avoid type confusion of objects with the mozWebExtLocation member.

(In reply to Tomislav Jovanovic :zombie from comment #3)

You'd think that, but hey:
https://searchfox.org/mozilla-central/rev/cffd9b5302/toolkit/components/extensions/ConduitsParent.jsm#291-292

Oh no! I suppose it is kinda my fault :-S
Thanks for finding that issue.

Rob is this ready to land? Is it safe enough to uplift?

Flags: needinfo?(rob)

I pushed to try and there are some crashes of the test file on debug builds. I'm investigating why. It could be a pre-existing issue, but I want to make sure before landing.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c8863e162a6dc6d7040dd4884d83250904c6c8

Flags: needinfo?(rob)

I'm going to land the first patch that fixes the regression. The second patch is just extra test coverage, and it has uncovered a pre-existing issue in unrelated code. That will be part of a new bug.

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/46abf66e3b20
Improve reliability of onMessage's error handling r=zombie

Backed out changeset 46abf66e3b20 (bug 1655624) for test_ext_error_location.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Ak3R5uxnQEiDy7BoMRvJ7A.0&searchStr=xpcshell&fromchange=8eb8e0e50e9358a9efb4f47f90769a5d95a81f2e&tochange=f770506ac16c019d1c3efb0459fd93724e02b744

Backout link: https://hg.mozilla.org/integration/autoland/rev/f770506ac16c019d1c3efb0459fd93724e02b744

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312826951&repo=autoland&lineNumber=8271

[task 2020-08-12T13:58:45.330Z] 13:58:45     INFO -  TEST-START | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js
[task 2020-08-12T13:58:45.985Z] 13:58:45  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js | xpcshell return code: 0
[task 2020-08-12T13:58:45.985Z] 13:58:45     INFO -  TEST-INFO took 653ms
[task 2020-08-12T13:58:45.985Z] 13:58:45     INFO -  >>>>>>>
[task 2020-08-12T13:58:45.985Z] 13:58:45     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-08-12T13:58:45.985Z] 13:58:45     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2020-08-12T13:58:45.985Z] 13:58:45     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-08-12T13:58:45.985Z] 13:58:45     INFO -  running event loop
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js | Starting check_remote
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  (xpcshell/head.js) | test check_remote pending (2)
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js | check_remote - [check_remote : 1] useRemoteWebExtensions matches - true == true
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js | check_remote - [check_remote : 1] testing from extension process - false == false
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  (xpcshell/head.js) | test check_remote finished (2)
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js | Starting test_error_location
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  (xpcshell/head.js) | test test_error_location pending (2)
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  "Extension attached"
[task 2020-08-12T13:58:45.986Z] 13:58:45     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2020-08-12T13:58:45.986Z] 13:58:45  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_error_location.js | test_error_location - [test_error_location : 472] Promise rejected, expecting rejection to match <unknown>, got "Could not establish connection. Receiving end does not exist."null - false == true
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  resource://testing-common/ExtensionXPCShellUtils.jsm:handleResult:472
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  resource://gre/modules/ExtensionCommon.jsm:emit:326
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  resource://gre/modules/Extension.jsm:receiveMessage:2001
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  Z:\task_1597239307\build\tests\xpcshell\head.js:_do_main:248
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  Z:\task_1597239307\build\tests\xpcshell\head.js:_execute_test:577
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  -e:null:1
[task 2020-08-12T13:58:45.987Z] 13:58:45     INFO -  exiting test
Flags: needinfo?(rob)
Flags: needinfo?(rob)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/b6178ee3cc2d
Improve reliability of onMessage's error handling r=zombie

https://hg.mozilla.org/mozilla-central/rev/b6178ee3cc2d

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
status-firefox81: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
See Also: → 1658827
Blocks: 1659074

Comment on attachment 9167434 [details]
Bug 1655624 - Regression tests for undefined/uncloneable error in onMessage

Revision D85644 was moved to bug 1659074. Setting attachment 9167434 [details] to obsolete.

Attachment #9167434 - Attachment is obsolete: true

Comment on attachment 9167433 [details]
Bug 1655624 - Improve reliability of onMessage's error handling

Beta/Release Uplift Approval Request

  • User impact if declined: Extension messaging doesn't work as expected. Add-ons can break.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Change is limited to extension code and covered by unit tests.
  • String changes made/needed:
Attachment #9167433 - Flags: approval-mozilla-beta?

Comment on attachment 9167433 [details]
Bug 1655624 - Improve reliability of onMessage's error handling

approved for 80 rc1

Attachment #9167433 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1657384

Hi Rob,
Not sure if this error happens due to the same root issue, but I can reproduce this on the latest Nightly 82.0a1 if I have Ghostery and Facebook Container addons installed/enabled and navigate to Youtube for example.

Promise rejected after context unloaded: Actor 'Conduits' destroyed before query 'RuntimeMessage' was resolved
content_script_bundle.js:110
action moz-extension://75e48aad-6ab0-4ec2-9704-0893fd8f19ce/dist/content_script_bundle.js:110

Should I submit a new issue?

Flags: needinfo?(rob)

Please do file a new issue with STR, and also a screenshot of the console.

Flags: needinfo?(rob)

Thank you! Submitted Bug 1665380.

Blocks: 1643176
You need to log in before you can comment on or make changes to this bug.