Last Comment Bug 722479 - browser/components/thumbnails/test/ tests leak chrome://global/content/mozilla.xhtml
: browser/components/thumbnails/test/ tests leak chrome://global/content/mozill...
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks: bc-leaks 721422
  Show dependency treegraph
 
Reported: 2012-01-30 13:59 PST by Dão Gottwald [:dao]
Modified: 2012-03-29 15:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1 (1.19 KB, patch)
2012-02-02 02:03 PST, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Review
patch v2 (1.45 KB, patch)
2012-02-03 04:28 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v3 (930 bytes, patch)
2012-02-03 14:29 PST, Tim Taubert [:ttaubert]
dao+bmo: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Dão Gottwald [:dao] 2012-01-30 13:59:20 PST

    
Comment 1 Tim Taubert [:ttaubert] 2012-02-02 02:03:27 PST
Created attachment 593766 [details] [diff] [review]
patch v1

Trivial patch.
Comment 2 Tim Taubert [:ttaubert] 2012-02-02 07:24:04 PST
https://hg.mozilla.org/integration/fx-team/rev/2b7cea9aa6a3
Comment 3 Tim Taubert [:ttaubert] 2012-02-02 14:36:05 PST
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/403652510ddd

This fix only worked on OS X, not on any other OS (oranges instead).
Comment 4 Tim Taubert [:ttaubert] 2012-02-03 04:28:37 PST
Created attachment 594128 [details] [diff] [review]
patch v2

Sent this patch to try and it works now on all platforms.
Comment 5 Dão Gottwald [:dao] 2012-02-03 04:39:49 PST
Comment on attachment 594128 [details] [diff] [review]
patch v2

>+  cachedXULDocument = cachedXULFrame = null;

This shouldn't be needed.

What guarantees that getXULDocument won't be called multiple times, creating multiple iframes and removing only the last one?
Comment 6 Tim Taubert [:ttaubert] 2012-02-03 14:29:06 PST
Created attachment 594302 [details] [diff] [review]
patch v3

(In reply to Dão Gottwald [:dao] from comment #5)
> What guarantees that getXULDocument won't be called multiple times, creating
> multiple iframes and removing only the last one?

Something much easier that works for multiple iframes.
Comment 7 Dão Gottwald [:dao] 2012-02-04 01:56:57 PST
Comment on attachment 594302 [details] [diff] [review]
patch v3

registerCleanupFunction(function () { doc.body.removeChild(iframe); });

registerCleanupFunction(function () doc.body.removeChild(iframe)); translates to:
registerCleanupFunction(function () { return doc.body.removeChild(iframe); });
Comment 8 Tim Taubert [:ttaubert] 2012-02-04 02:55:40 PST
(In reply to Dão Gottwald [:dao] from comment #7)
> registerCleanupFunction(function () doc.body.removeChild(iframe));
> translates to:
> registerCleanupFunction(function () { return doc.body.removeChild(iframe);
> });

I know, didn't think it's a big deal but actually you're right. We better be explicit about that.

https://hg.mozilla.org/integration/fx-team/rev/a7ea6d49bc69
Comment 9 Tim Taubert [:ttaubert] 2012-02-05 04:39:18 PST
https://hg.mozilla.org/mozilla-central/rev/a7ea6d49bc69
Comment 10 Tim Taubert [:ttaubert] 2012-02-09 13:34:16 PST
Comment on attachment 594302 [details] [diff] [review]
patch v3

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky): Very low risk.
String changes made by this patch: None.

Prevents the thumbnail tests from leaking.
Comment 11 Alex Keybl [:akeybl] 2012-02-09 13:53:08 PST
Comment on attachment 594302 [details] [diff] [review]
patch v3

[Triage Comment]
Test fix for Aurora 12 - approved.
Comment 12 Tim Taubert [:ttaubert] 2012-02-09 16:22:30 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/c99d404f4267

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