[e10s] browser.runtime.connect is entangled with unrelated ports (unexpected onMessage/onDisconnect)

VERIFIED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox48+ verified, firefox49+ verified, firefox50 verified)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
1. Ensure that e10s is enabled (https://wiki.mozilla.org/Electrolysis#Testing).
2. Load attached addon.
3. Visit example.com
4. Open the global JS console (Ctrl-Shift-J).
5. Click on the extension button - the extension will insert a content script.
6. Look at the console, you will see:

Port not found, going to inject script.
contentscript.js ran
BG: onConnect port-from-cs-Fri Jul 15 2016 21:49:03 GMT-0700 (PDT)
CS2: onConnect port-from-bg-Fri Jul 15 2016 21:49:03 GMT-0700 (PDT)

7. Click on the the button that was inserted in the tab.
8. Look at the console.

Result (Firefox 47.0.1, Firefox Nightly (50, built today):
BG: onMessage (port-from-cs-Fri Jul 15 2016 21:49:03 GMT-0700 (PDT)) = Sent from CS2, for BG2 only: Fri Jul 15 2016 21:49:06 GMT-0700 (PDT)
BG2: onMessage (port-from-bg-Fri Jul 15 2016 21:49:03 GMT-0700 (PDT)) = Sent from CS2, for BG2 only: Fri Jul 15 2016 21:49:06 GMT-0700 (PDT)

EXPECTED result:
The first line shouldn't be there, I should only see the line starting with "BG2". This error shows that a completely unrelated extension port received a message that was meant for a different port.

It is caused by the use of internal port IDs that are only unique within a process: http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/toolkit/components/extensions/ExtensionUtils.jsm#1167
(Assignee)

Comment 1

2 years ago
Created attachment 8771621 [details]
connect-bidirectionally.zip
(Assignee)

Comment 2

2 years ago
Created attachment 8771622 [details]
Bug 1287245 - Ensure globally unique internal port IDs

Review commit: https://reviewboard.mozilla.org/r/64724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64724/
Attachment #8771622 - Flags: review?(wmccloskey)

Updated

2 years ago
Whiteboard: triaged
Comment on attachment 8771622 [details]
Bug 1287245 - Ensure globally unique internal port IDs

https://reviewboard.mozilla.org/r/64724/#review62066

This looks good with the change I've requested.

::: toolkit/components/extensions/ExtensionUtils.jsm:1163
(Diff revision 1)
>    connect(messageManager, name, recipient) {
> -    let portId = nextPortId++;
> +    // TODO(robwu): This should include the ID of the process, to be globally
> +    // unique and avoid conflicts between messages sent by one end to another.
> +    // Using Math.random() as a quick hack, this should be changed to something
> +    // that is guaranteed to be globally unique.
> +    let portId = `${gNextPortId++}${Math.random()}`;

For Firefox 48, I think the simplest fix here is to combine gNextPortId with Services.appInfo.processType. If we're in a content process, Services.appInfo.processType will be different than in the parent process. I think that should solve this problem.

In the long term, it probably makes sense to expose a process ID via Services.cpmm.processId. Rob, can you file a bug for that? I can show you how to implement it.
Attachment #8771622 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

2 years ago
Comment on attachment 8771622 [details]
Bug 1287245 - Ensure globally unique internal port IDs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64724/diff/1-2/
Attachment #8771622 - Flags: review?(wmccloskey)
Attachment #8771622 - Flags: review?(wmccloskey) → review+
[Tracking Requested - why for this release]: This is a significant bug in WebExtensions and we want to fix it in 48. It can cause messages to go to the wrong destination.
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
Bill, please land that as possible and fill the uplift request.
It will be in RC.
Tracking as it seems pretty significant and we will probably release 10s soon.
tracking-firefox48: ? → +
tracking-firefox49: ? → +

Comment 8

2 years ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c026e26932d
Ensure globally unique internal port IDs r=billm

Comment 9

2 years ago
Hi Bill, sorry had to back out due to Mochitest failure, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=764231&repo=autoland#L4314
Flags: needinfo?(wmccloskey)

Comment 10

2 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a10527539339
Backed out changeset 7c026e26932d for Mochitest test_ext_background_runtime_connect_params.html failure

Updated

2 years ago
Flags: needinfo?(rob)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8771622 [details]
Bug 1287245 - Ensure globally unique internal port IDs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64724/diff/2-3/
(Assignee)

Comment 12

2 years ago
Muphry's law in action. While pointing out the wrong spelling of Services.appinfo in bug 1287631, I of course misspelled it in my patch... Please try to land it again. All mochitest pass using the latest try job.
Flags: needinfo?(rob)

Comment 13

2 years ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8950de62b29
Ensure globally unique internal port IDs r=billm
Flags: needinfo?(wmccloskey)

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a8950de62b29
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8771622 [details]
Bug 1287245 - Ensure globally unique internal port IDs

Approval Request Comment
[Feature/regressing bug #]: WebExtensions
[User impact if declined]: Extensions may see messages arrive on unexpected ports. This could cause all sorts of problems.
[Describe test coverage new/current, TreeHerder]: On m-c.
[Risks and why]: Very low risk. The patch is quite simple and restricted to WebExtension ports.
[String/UUID change made/needed]: None.
Attachment #8771622 - Flags: approval-mozilla-beta?
Attachment #8771622 - Flags: approval-mozilla-aurora?
This is super late in the 48 cycle. 
I would rather let it ride the train from 49. wdyt?
(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> This is super late in the 48 cycle. 
> I would rather let it ride the train from 49. wdyt?

Yeah, I know. I still think we should take it. It's a very low risk patch and the bug is pretty insidious. It affects WebExtensions only when e10s is enabled. Both of those are new in 48, so they'll be getting lots of attention.

I double checked everything in the patch to try to anticipate what might go wrong and I'm just not seeing anything.
Comment on attachment 8771622 [details]
Bug 1287245 - Ensure globally unique internal port IDs

ok, too bad it didn't arrive in beta 9...
Attachment #8771622 - Flags: approval-mozilla-beta?
Attachment #8771622 - Flags: approval-mozilla-beta+
Attachment #8771622 - Flags: approval-mozilla-aurora?
Attachment #8771622 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox48: affected → fixed
status-firefox49: affected → fixed
I was able to reproduce the initial issue on Firefox 50.0a1 (2016-07-15) under Windows 10 64-bit.

Verified as fixed on Firefox 51.0a1 (2016-09-04), Firefox 50.0a2 (2016-09-04), Firefox 49.0b8 (20160829102229) and Firefox 48.0.2 (20160823121617) under Windows 10 64-bit.
The first line, which starts with BG is no longer thrown in Browser Console.
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.