Closed Bug 1299480 Opened 3 years ago Closed 3 years ago

regression: right click context menu broken via cross containers

Categories

(Core :: DOM: Security, defect)

51 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- verified

People

(Reporter: jkt, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(2 files)

Attached image 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.
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
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?
Flags: needinfo?(allstars.chh)
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
Summary: Cross containers displays the previous tabs context menu → regression: right click context menu broken via cross containers
Whiteboard: [userContextId][domsecurity-backlog]
Version: unspecified → 51 Branch
forward to baku as I am not familiar with nsContextMenu, and from the console error it's casued by bug 1257455.
Flags: needinfo?(allstars.chh) → needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch context.patchSplinter Review
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.
Attachment #8788252 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8788252 - Flags: review?(gijskruitbosch+bugs) → review+
> event.target.dataset.usercontextid

I cannot. dataset is undefined.
(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!
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb0f5bef1a6
Fix regression in the context menu when containers are in used, r=gijs
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.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9d3e0f0b20
Fix regression in the context menu when containers are in used, r=gijs
(In reply to Pulsebot from comment #10)
> Pushed by amarchesini@mozilla.com:
> 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?
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2374e3ecf070
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?

It's not needed. the reason why the test was broken is because I left a usercontextid in the browser-context.inc.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/2374e3ecf070
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Looks like this bug caused the regression in bug#1302088.
Depends on: 1302088
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.