1244887 - incorrect usercontext label being used under the awesomebar

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: m-c.png

Looks like the awesomebar is using the incorrect usercontext labels. It seems like it's always using the "default" case [1] however the current id's are definitely in the pre-defined range. Attached screenshots to illustrate the issue.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/UserContextUI.jsm#29

STR:

* install either m-c or m-a
* once installed, privacy.userContext.enabled;true under about:config
* open new containers via File -> New Container Tab

Reproduced on both OSX 10.11.3 x64 and Win 10 x64 Pro VM using the following builds:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-01-03-02-41-mozilla-central/
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-01-00-40-07-mozilla-aurora/

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Attachment: m-a.png

Comment 2

[Steven Englehardt [:englehardt]]

Attachment: 1244887.patch

This was introduced in Bug 1239606. It stems from the fact that the ID is passed around and a string in the UI, but an int in the rest of the browser. The shopping and banking text was also swapped in 1239606.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e625cb1faf3

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8714515 [details] [diff] [review]
1244887.patch

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

::: browser/modules/UserContextUI.jsm
@@ +16,5 @@
>  this.UserContextUI = {
>    getUserContextLabel(userContextId) {
>      switch (userContextId) {
>        // No UserContext:
>        case 0: return "";

This should be case "0".
But, just because userContextId is always a number, what about if we do:

switch(parseInt(usercontextId)) {
...

Comment 4

[Tanvi Vyas[:tanvi]]

Bram, this might be an issue in the Windows build we are about to use for the usertesting.com study.  Do you see "Personal" in the url bar, or "Context 1"?  

The Windows build was made from this try push based on 1/21/16 moz-central.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e232642793.  The bug that regressed the issue landed on 1/15/2016.  When I run a Mac build with the repository from 1/21/16, I see "Context 1".

Steve, thanks for the help identifying the issue here and the patch!

Comment 5

[Steven Englehardt [:englehardt]]

(In reply to Andrea Marchesini (:baku) from comment #3)

> This should be case "0".

Good catch! While testing I noticed that the (userContextId = 0) case should not happen, as the browser will only have a userContextId attribute when explicitly set by the File Menu. If not set (the default), it removes the UI box (see: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4067-4070). It makes sense to keep this for completeness.

> But, just because userContextId is always a number, what about if we do:
> 
> switch(parseInt(usercontextId)) {
> ...

Agreed, updated.

Comment 6

[Steven Englehardt [:englehardt]]

Attachment: 1244887.patch

Comment 7

[Bram Pitoyo [:bram]]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Bram, this might be an issue in the Windows build we are about to use for
> the usertesting.com study.  Do you see "Personal" in the url bar, or
> "Context 1"?  

I see “Context 1” all the way through “Context 4”.

Let me know when a fixed build has been generated, and I’ll upload it to Dropbox. The good news is, the Dropbox link won’t change, so we won’t have to change our usertesting.com instruction to suit.

Comment 8

[Tanvi Vyas[:tanvi]]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c245fc12ef5

We are going to hold off on our usertesting study until this bug is fixed.  baku, if you could review as soon as possible, that would be great!

Thanks!

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 8714537 [details] [diff] [review]
1244887.patch

Approval Request Comment
[Feature/regressing bug #]: Regressed by bug Bug 1239606
[User impact if declined]: Users who enable Containers in about:config will not see the right UI in the url bar
[Describe test coverage new/current, TreeHerder]: The containers feature has mochitests.  This bug is for a visual change. https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e232642793, https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e625cb1faf3
[Risks and why]: This is a very small patch and changes UI that is turned off by default, hence this is very low risk.
[String/UUID change made/needed]: none

Comment 12

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-05-03-02-04-mozilla-central/

OS's Used:

* OSX 10.11.3 - PASSED
* Win 10 x64 Pro VM - PASSED
* Ubuntu 14.04.3 x64 VM - PASSED

* checked all four containers in e10s window and ensured the correct labels & colors are being used
* checked all four containers in a non-e10s window and ensured the correct labels & colors are being used
* checked all four containers in a private browing window and ensured that the correct labels & colors are being used

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8714537 [details] [diff] [review]
1244887.patch

Fix for regression from 46, verified fix on m-c.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/78b3e25059cd

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-10-00-40-06-mozilla-aurora/

OS's Used:

* OSX 10.11.3 - PASSED
* Win 10 x64 Pro VM - PASSED
* Ubuntu 14.04.3 x64 VM - PASSED

* checked all four containers in e10s window and ensured the correct labels & colors are being used
* checked all four containers in a non-e10s window and ensured the correct labels & colors are being used
* checked all four containers in a private browing window and ensured that the correct labels & colors are being used