1299480 - regression: right click context menu broken via cross containers

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Jonathan Kingston [:jkt] he/him]

Attachment: Selection_301.png

STR:
1. In a non container select some text and right click
2. Notice the correct text show in: 'Search [Search engine] for "[text selected]"'
3. In a container tab select some text and right click
4. Notice the previous text show in: 'Search [Search engine] for "[text selected]"'

I was testing across work, none and personal. It doesn't always seem to reproduce but it has been very regular.

This may relate to sites that touches the clipboard, which I think https://dxr.mozilla.org does using window.getSelection().

This is reproducible without selecting text in the second tab, but still displays the context menu as if text were selected.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

I've managed to reproduce this a few times with the latest m-c [1] but I'm still trying to find reliable STR. Once I get some STR, I'll try getting a regression range as well.

This looks like a pretty bad bug. When it does occur, the right click context menu completely breaks for that specific tab. Whenever you select any of the menu items, you'll receive the error messages I've added below in the browser console. The only menu item that still works when the context menu is in the broken state is 'Search [Search engine] for "[text selected]"'.

* video of the issue: https://youtu.be/9BuCR431hXU

Browser Console errors:

TypeError: gContextMenu is null browser.xul:1:1
onpopupshowingchrome://browser/content/browser.xul:1:1

NS_ERROR_ILLEGAL_VALUE: nsContextMenu.js:152
CM_initOpenItemschrome://browser/content/nsContextMenu.js:152:7
CM_initItemschrome://browser/content/nsContextMenu.js:106:5
CM_initMenuchrome://browser/content/nsContextMenu.js:85:5
nsContextMenuchrome://browser/content/nsContextMenu.js:21:3
onpopupshowingchrome://browser/content/browser.xul:1:119
openPopupAtScreenchrome://global/content/bindings/popup.xml:67:15
receiveMessagechrome://browser/content/tabbrowser.xml:4355:15

[1] fx51.0a1, buildId: 20160831030224, changeset: 506facea6316

Comment 2

[Kamil Jozwiak [:kjozwiak]]

STR:

* launch the latest version of m-c
* open a personal container via "file -> new container tab"
* load facebook.com within the personal container
* open a shopping container via "file -> new container tab"
* switch back into the personal container where FB has already been loaded
* select the "Create a Page" link at the bottom of FB page and right click so you get the context menu
* without closing the context menu, click on the shopping container tab
** the first click will close the currently opened context menu, the second click will switch into the shopping container tab
* in the shopping container, load ebay.com
* right click on any link and you'll notice that the context menu is broken with the errors appearing under the browser console

Let me know if these STR might be confusing, I can always create a quick video.

Regression range:

15:13.50 INFO: Last good revision: 37014a3e11f39652f24df6b489e2be42fbaeea53
15:13.50 INFO: First bad revision: 702ab78cea269af5f9c3494eec4b101fb6b47d5b
15:13.50 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37014a3e11f39652f24df6b489e2be42fbaeea53&tochange=702ab78cea269af5f9c3494eec4b101fb6b47d5b

15:14.46 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1257455

Yoshi, would you mind taking a look?

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

forward to baku as I am not familiar with nsContextMenu, and from the console error it's casued by bug 1257455.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: context.patch

The reason why we have this issue is because |item.setAttribute("usercontextid", userContextId);| throws an exception. This exception happens because we don't support the changing of usercontextid attribute in XULElements for security reasons.

This patch renames usercontextid attribute to data-usercontextid for the contextmenu.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8788252 [details] [diff] [review]
context.patch

Review of attachment 8788252 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1007,5 @@
>      }
>  
>      let params = {
>        allowMixedContent: persistAllowMixedContentInChildTab,
> +      userContextId: parseInt(event.target.getAttribute('data-usercontextid'))

Here you can use

event.target.dataset.usercontextid

Nit: please also add a trailing comma.

Comment 6

[Andrea Marchesini [:baku]]

> event.target.dataset.usercontextid

I cannot. dataset is undefined.

Comment 7

[:Gijs (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #6)
> > event.target.dataset.usercontextid
> 
> I cannot. dataset is undefined.

Oh, blah, I forgot this is XUL. Ignore me!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb0f5bef1a6
Fix regression in the context menu when containers are in used, r=gijs

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5a61f9f8334b for failures in browser_referrer_open_link_in_tab.js, https://treeherder.mozilla.org/logviewer.html#?job_id=35362730&repo=mozilla-inbound

Mac and Windows only, because the test is disabled on Linux.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9d3e0f0b20
Fix regression in the context menu when containers are in used, r=gijs

Comment 11

[:Gijs (he/him)]

(In reply to Pulsebot from comment #10)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/de9d3e0f0b20
> Fix regression in the context menu when containers are in used, r=gijs

Why was the ternary necessary when it wasn't necessary before with the different attribute name?

Comment 12

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

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35390295&repo=mozilla-inbound

So far they've all been on windows, but that might not mean anything.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d94e8ab0f01e

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2374e3ecf070
Fix regression in the context menu when containers are in used, r=gijs

Comment 14

[Andrea Marchesini [:baku]]

> Why was the ternary necessary when it wasn't necessary before with the
> different attribute name?

It's not needed. the reason why the test was broken is because I left a usercontextid in the browser-context.inc.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/2374e3ecf070

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Looks like this bug caused the regression in bug#1302088.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/09/2016-09-12-03-04-21-mozilla-central/
* https://archive.mozilla.org/pub/firefox/nightly/2016/09/2016-09-13-00-40-05-mozilla-aurora/

Platforms:
* macOS 10.11.6 x64 -> PASSED (using both m-c & m-a)
* Windows 10 x64 -> PASSED (using both m-c & m-a)
* Ubuntu 16.04.1 LTS x64 VM -> PASSED (using both m-c & m-a)

Test Cases:

* went through the original STR from comment#2 using the following method of opening containers:
** File -> New Container Tab (under OSX, also opened containers when there's no other windows opened)
** Hamburger Menu -> Open Container Tab
** down arrow (when there's many tabs opened) -> New Container Tab
* went through the original STR from comment#2 using regular e10s tabs rather than containers
* went through the original STR from comment#2 using PB rather than containers
* ensured that all the context menu items are working correctly after going through the STR from comment#2
* ensured the browser console isn't displaying an error messages relating to the context menu
* highlighted a text link within two different containers and switched back and forth opening context menus