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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

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

Comment 1

[Rob Wu [:robwu]]

Attachment: connect-bidirectionally.zip

Comment 2

[Rob Wu [:robwu]]

Attachment: 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/

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Rob Wu [:robwu]]

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/

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

[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.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c026e26932d
Ensure globally unique internal port IDs r=billm

Comment 9

[Iris Hsiao [:ihsiao]]

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

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a10527539339
Backed out changeset 7c026e26932d for Mochitest test_ext_background_runtime_connect_params.html failure

Comment 11

[Rob Wu [:robwu]]

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/

Comment 12

[Rob Wu [:robwu]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8950de62b29
Ensure globally unique internal port IDs r=billm

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

This is super late in the 48 cycle. 
I would rather let it ride the train from 49. wdyt?

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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 18

[Sylvestre Ledru [:Sylvestre]]

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

ok, too bad it didn't arrive in beta 9...

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2b2b741d4e8
https://hg.mozilla.org/releases/mozilla-beta/rev/05853bb06a87

Comment 20

[Vasilica Tamas, QA [:vtamas]]

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.