722460 - gBrowserThumbnails uninit sets a property that has only a getter

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: 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

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Why does this reference need to be cleared at all, I wonder?

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I wondered too. It probably doesn't since the window is closing anyway. Let's see if Time has a reason.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

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

Stealing. Thank you for fixing this!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

Oops, I totally missed that conversation here :/

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

conversation was waiting for your input :) Can we just remove this line?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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 ;)

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: 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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Paul, do you want me to land that patch?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/94ab58a742a9

Comment 11

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/94ab58a742a9