Closed
Bug 1287245
Opened 8 years ago
Closed 8 years ago
[e10s] browser.runtime.connect is entangled with unrelated ports (unexpected onMessage/onDisconnect)
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48+ verified, firefox49+ verified, firefox50 verified)
VERIFIED
FIXED
mozilla50
People
(Reporter: robwu, Assigned: robwu)
Details
(Whiteboard: triaged)
Attachments
(2 files)
1.45 KB,
application/zip
|
Details | |
58 bytes,
text/x-review-board-request
|
billm
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details |
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•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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•8 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•8 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+
Comment on attachment 8771622 [details] Bug 1287245 - Ensure globally unique internal port IDs https://reviewboard.mozilla.org/r/64724/#review62084
[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:
--- → ?
Comment 7•8 years ago
|
||
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
Comment 9•8 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•8 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•8 years ago
|
Flags: needinfo?(rob)
Assignee | ||
Comment 11•8 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•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8950de62b29
Status: ASSIGNED → RESOLVED
Closed: 8 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?
Comment 16•8 years ago
|
||
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 18•8 years ago
|
||
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d2b2b741d4e8 https://hg.mozilla.org/releases/mozilla-beta/rev/05853bb06a87
Comment 20•8 years ago
|
||
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
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•