Setting getter-only property "styleSheet" in mail-compacttheme.js

RESOLVED FIXED in Thunderbird 64.0

Status

defect
--
trivial
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 64.0
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

When closing a compose window, I get this error in the Error console:

TypeError: setting getter-only property "styleSheet" mail-compacttheme.js:66:5

The code in question is:

uninit() {
  Services.obs.removeObserver(this, "lightweight-theme-styling-update");
  this.styleSheet = null;
}

The styleSheet object member is defined as:

var CompactTheme = {
  get styleSheet() {
    delete this.styleSheet;
    for (let styleSheet of document.styleSheets) {
      if (styleSheet.href == "chrome://messenger/skin/compacttheme.css") {
        this.styleSheet = styleSheet;
        break;
      }
    }
    return this.styleSheet;
  },

So it seems at the beginning it is a getter only, but then may be deleted and  created as a property on 'this' which can be assigned further.

It happens because uninit() is called sooner than the getter is ever called.
Any way to check if the 'styleSheet' is still a getter and then do not touch it?
The same code exists at https://dxr.mozilla.org/comm-central/source/browser/base/content/browser-compacttheme.js#66, but maybe nobody noticed, or in Firefox the getter actually gets run first, but in c-c it is not called (in some cases).
Posted patch 1499489.patchSplinter Review
Arai, thanks for your hints on how to check the getter. Does this look good?
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #9018056 - Flags: review?(jorgk)
Attachment #9018056 - Flags: feedback?(arai.unmht)
Comment on attachment 9018056 [details] [diff] [review]
1499489.patch

Let's not pretend the reviewer know anything about this ;-)
Attachment #9018056 - Flags: review?(jorgk)
Attachment #9018056 - Flags: review?(arai.unmht)
Attachment #9018056 - Flags: feedback?(arai.unmht)
> Let's not pretend the reviewer know anything about this ;-)
Better: Let's not pretend the original reviewer (myself) knows anything about this ;-)
Comment on attachment 9018056 [details] [diff] [review]
1499489.patch

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

what's the purpose of `this.styleSheet = null;` ?
just to remove the reference, and the existence of the property doesn't matter?
if so, you don't have to check the type of the property, but just delete it, regardless whether it's getter or data property.

  uninit() {
    delete this.styleSheet;
  }

if CompactTheme.styleSheet can be read after uninit() call and it should be null instead of undefined then, this looks fine.
Attachment #9018056 - Flags: review?(arai.unmht) → review+
(In reply to Tooru Fujisawa [:arai] from comment #5)
> what's the purpose of `this.styleSheet = null;` ?
> just to remove the reference, and the existence of the property doesn't
> matter?

I don't know so I do not change that behaviour. Whether a getter or a property, it will return something and not be undefined or throw errors in console. The original author wanted to set it to null so I just want to allow him to do that cleanly without error :)

Thanks.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6482582d97bf
handle CompactTheme.styleSheet getter when trying to set its value. r=arai
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.