1279297 - need to restart fx for the container icon to appear under the hamburger menu

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

When enabling containers, users will have to restart the browser for the container icon to appear under the hamburger customization window. Is there anything we can do here or is this a limitation of fx?

STR:

* launch a brand new installation of m-c
* enable containers in about:config via the "privacy.userContext.enabled;true" pref
* click on the hamburger menu -> customize and you'll notice that the container icon isn't being displayed
* restart fx
* click on the hamburger menu -> customize and you'll notice that the container icon is being displayed correctly

Platforms Used:

* Ubuntu 14.04.4 LTS x64 - Reproduced the issue
** fx50.0a1 buildId: 20160609064045, changeset: f8e3b81a79f4

* OSX 10.11.5 x64 - Reproduced the issue
** fx50.0a1 buildId: 20160609064045, changeset: f8e3b81a79f4

* Win 10 x64 - Reproduced the issue
** fx50.0a1 buildId: 20160609064045, changeset: f8e3b81a79f4

Comment 1

[Andrea Marchesini [:baku]]

Attachment: no_restart1.patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8771270 [details] [diff] [review]
no_restart1.patch

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1133,5 @@
> +        aNode.hidden = !Services.prefs.getBoolPref("privacy.userContext.enabled");
> +      }
> +
> +      Services.prefs.addObserver("privacy.userContext.enabled", updateVisibility, false);
> +      updateVisibility();

You never remove this observer, so if you open a window with the button in it and then close it this code will leak the window (because the pref service has a strong ref to the observer which has a ref to the node and thereby to the window and everything in it).

Can you refactor this so there's only 1 observer for the pref instead of 1 per window, and it uses a weak reference? You can store the widget spec in an object, make it implement nsIObserver and nsISupportsWeakReference, and in its observe() method you can loop over CUI.getWidget(this.id).instances and access their 'node' property to make modifications to each of the nodes, inasmuch as they exist.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: no_restart1.patch

Comment 4

[Andrea Marchesini [:baku]]

Attachment: no_restart1.patch

Comment 5

[:Gijs (he/him)]

Comment on attachment 8772415 [details] [diff] [review]
no_restart1.patch

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

w/ the below, r=me

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1128,5 @@
>        if (PrivateBrowsingUtils.isWindowPrivate(win)) {
>          aNode.setAttribute("disabled", "true");
>        }
> +
> +      Services.prefs.addObserver("privacy.userContext.enabled", this, true);

Almost... now just need to make sure we only do this once. Simplest solution is probably to make the spec here a variable ("containerWidget") and push it onto the widget array afterwards, and also run this one line.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ee6a3e31d6
No restarts needed for Containers when its pref is changed, r=gijs

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82ee6a3e31d6