Closed Bug 1287245 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

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

VERIFIED FIXED
mozilla50
Tracking Status
firefox48 + verified
firefox49 + verified
firefox50 --- verified

People

(Reporter: robwu, Assigned: robwu)

Details

(Whiteboard: triaged)

Attachments

(2 files)

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
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)
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.
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.
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c026e26932d
Ensure globally unique internal port IDs r=billm
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)
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
Flags: needinfo?(rob)
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/
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)
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8950de62b29
Ensure globally unique internal port IDs r=billm
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/a8950de62b29
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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+
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.