Last Comment Bug 722460 - gBrowserThumbnails uninit sets a property that has only a getter
: gBrowserThumbnails uninit sets a property that has only a getter
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
Mentors:
Depends on:
Blocks: 497543
  Show dependency treegraph
 
Reported: 2012-01-30 13:22 PST by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-02-22 09:35 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (879 bytes, patch)
2012-01-30 13:22 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review
Patch v0.2 (941 bytes, patch)
2012-02-07 17:04 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
ttaubert: review+
Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-30 13:22:36 PST
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
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-30 13:56:57 PST
Why does this reference need to be cleared at all, I wonder?
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-30 14:00:12 PST
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] 2012-02-01 22:53:25 PST
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] 2012-02-01 23:01:23 PST
Comment on attachment 592844 [details] [diff] [review]
Patch v0.1

Oops, I totally missed that conversation here :/
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-02 10:23:16 PST
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] 2012-02-02 14:55:29 PST
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 reading much bugmail, email directly) 2012-02-07 17:04:40 PST
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.
Comment 8 Tim Taubert [:ttaubert] 2012-02-07 17:12:06 PST
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] 2012-02-20 08:16:05 PST
Paul, do you want me to land that patch?
Comment 10 Tim Taubert [:ttaubert] 2012-02-20 15:38:16 PST
https://hg.mozilla.org/integration/fx-team/rev/94ab58a742a9
Comment 11 Rob Campbell [:rc] (:robcee) 2012-02-22 09:35:44 PST
https://hg.mozilla.org/mozilla-central/rev/94ab58a742a9

Note You need to log in before you can comment on or make changes to this bug.