884120 - themeData is null @ chrome://browser/content/browser.js:618

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

When entering customization mode without a lightweight theme applied, LightweightThemeListener.observe dereferences themeData even if "null" was passed as aData.

We should only disable lightweight themes if one is applied when entering customization mode, since this looks like it may break consumers unexpectedly.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #0)
> We should only disable lightweight themes if one is applied when entering
> customization mode, since this looks like it may break consumers
> unexpectedly.

No, consumers have to expect null being passed in. This is a bug in 
LightweightThemeListener.

Comment 2

[Matthew N. [:MattN]]

My fault so I can take this eventually.

Comment 4

[Justin Dolske [:Dolske]]

Unclear if this actually breaks something.

Comment 5

[:Gijs (he/him)]

Attachment: nullcheck themeData before using it in LightweightThemeListener

This was annoying me, so I looked... I think this is all we need here?

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 777009 [details] [diff] [review]
nullcheck themeData before using it in LightweightThemeListener

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

::: browser/base/content/browser-addons.js
@@ +419,5 @@
>  
>    // nsIObserver
>    observe: function (aSubject, aTopic, aData) {
>      if (aTopic != "lightweight-theme-styling-update" || !this.styleSheet)
>        return;

Are you sure you can't just return if !aData?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 777009 [details] [diff] [review]
nullcheck themeData before using it in LightweightThemeListener

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

::: browser/base/content/browser-addons.js
@@ +419,5 @@
>  
>    // nsIObserver
>    observe: function (aSubject, aTopic, aData) {
>      if (aTopic != "lightweight-theme-styling-update" || !this.styleSheet)
>        return;

As discussed on IRC, aData is 'null' in this case because of JSON.stringify being called on null so this is fine.

@@ +422,5 @@
>      if (aTopic != "lightweight-theme-styling-update" || !this.styleSheet)
>        return;
>  
>      let themeData = JSON.parse(aData);
> +    if (themeData)

Nit: we generally prefer early returns:
  if (!themeData)
    return;

Comment 8

[:Gijs (he/him)]

Pushed with nits fixed:

https://hg.mozilla.org/projects/ux/rev/2597a5d813c6

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/2597a5d813c6