Closed Bug 1244887 Opened 4 years ago Closed 4 years ago

incorrect usercontext label being used under the awesomebar

Categories

(Core :: Security, defect)

47 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 --- verified

People

(Reporter: kjozwiak, Assigned: englehardt)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image 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/
Attached image m-a.png
Attached patch 1244887.patch (obsolete) — Splinter Review
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
Attachment #8714515 - Flags: review?(tanvi)
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)) {
...
Attachment #8714515 - Flags: review?(tanvi) → review-
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!
Flags: needinfo?(bram)
(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.
Attached patch 1244887.patchSplinter Review
Attachment #8714515 - Attachment is obsolete: true
Attachment #8714537 - Flags: review?(amarchesini)
(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.
Flags: needinfo?(bram)
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!
Attachment #8714537 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/e8723c580c6d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee: nobody → bugzilla
Depends on: 1239606
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
Attachment #8714537 - Flags: approval-mozilla-aurora?
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 on attachment 8714537 [details] [diff] [review]
1244887.patch

Fix for regression from 46, verified fix on m-c.
Attachment #8714537 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking since this is a regression.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.