Closed Bug 722460 Opened 12 years ago Closed 11 years ago

gBrowserThumbnails uninit sets a property that has only a getter


(Firefox :: General, defect)

Not set



Firefox 13


(Reporter: zpao, Assigned: zpao)



(Whiteboard: [fixed-in-fx-team])


(1 file, 1 obsolete file)

Attached patch Patch v0.1 (obsolete) — Splinter Review
This is a strict-mode warning, so not super high priority, but something that shouldn't be too hard to fix. It's also going to be filling logs for every mochitest that closes a window.

_pageThumbs is defined as a property first, then added as a lazy getter in `init`. Then in `uninit` it's nulled out. Nulling it out gives the warning because it only has a getter.

On top of it being a warning, it doesn't actually work. Looks like that's just a noop because there's no setter.

- - -

Some test case code:
> var a = {};
> Components.utils.import("resource:///modules/XPCOMUtils.jsm");
> XPCOMUtils.defineLazyGetter(a, "foo", function() [1,2]);
> = null;
> alert(; // [1,2]
> delete;
> alert(; // undefined
Attachment #592844 - Flags: review?(
Why does this reference need to be cleared at all, I wonder?
I wondered too. It probably doesn't since the window is closing anyway. Let's see if Time has a reason.
Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

Review of attachment 592844 [details] [diff] [review]:

Stealing. Thank you for fixing this!
Attachment #592844 - Flags: review?( → review+
Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

Oops, I totally missed that conversation here :/
Attachment #592844 - Flags: review+ → review?(
Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

conversation was waiting for your input :) Can we just remove this line?
Attachment #592844 - Flags: review?( → review?(ttaubert)
Yeah, this window goes away anyway, we can just remove this line. I'd give you a r+ but that's not what the patch does ;)
Attached patch Patch v0.2Splinter Review
I already put r=you but posterity's sake, here's a patch. I know the window goes away, but I left nulling the WeakMap out because I have less confidence in that, though I would imagine it's safe to skip that step too.
Assignee: nobody → paul
Attachment #592844 - Attachment is obsolete: true
Attachment #592844 - Flags: review?(ttaubert)
Attachment #595254 - Flags: review?(ttaubert)
Comment on attachment 595254 [details] [diff] [review]
Patch v0.2

Review of attachment 595254 [details] [diff] [review]:

Sorry for being so nitpicky. I think I was a little too tired and forgot all about this bug afterwards. Thank you for updating the patch! And feel free to skip nulling out the WeakMap as well - I think it's safe, too.
Attachment #595254 - Flags: review?(ttaubert) → review+
Paul, do you want me to land that patch?
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.