gBrowserThumbnails uninit sets a property that has only a getter

RESOLVED FIXED in Firefox 13

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: zpao, Assigned: zpao)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 592844 [details] [diff] [review]
Patch v0.1

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]);
> a.foo = null;
> alert(a.foo); // [1,2]
> delete a.foo;
> alert(a.foo); // undefined
Attachment #592844 - Flags: review?(gavin.sharp)
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?(gavin.sharp) → review+
Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

Oops, I totally missed that conversation here :/
Attachment #592844 - Flags: review+ → review?(gavin.sharp)
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?(gavin.sharp) → 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 ;)
Created attachment 595254 [details] [diff] [review]
Patch v0.2

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
Status: NEW → ASSIGNED
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?
https://hg.mozilla.org/integration/fx-team/rev/94ab58a742a9
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/94ab58a742a9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.