Closed Bug 884120 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M8][Australis:P5])

Attachments

(1 file)

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.
Summary: [17:03:31.214] themeData is null @ chrome://browser/content/browser.js:618 → themeData is null @ chrome://browser/content/browser.js:618
(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.
Component: Toolbars and Customization → Theme
My fault so I can take this eventually.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
No longer blocks: 870602
Duplicate of this bug: 885746
Unclear if this actually breaks something.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
This was annoying me, so I looked... I think this is all we need here?
Attachment #777009 - Flags: review?(mnoorenberghe+bmo)
Assignee: mnoorenberghe+bmo → gijskruitbosch+bugs
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 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;
Attachment #777009 - Flags: review?(mnoorenberghe+bmo) → review+
Pushed with nits fixed:

https://hg.mozilla.org/projects/ux/rev/2597a5d813c6
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M8][Australis:P5][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/2597a5d813c6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P5][fixed-in-ux] → [Australis:M8][Australis:P5]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.