722479 - browser/components/thumbnails/test/ tests leak chrome://global/content/mozilla.xhtml

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Trivial patch.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2b7cea9aa6a3

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

Attachment: patch v2

Sent this patch to try and it works now on all platforms.

Comment 5

[Dão Gottwald [:dao]]

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] (inactive)]

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

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] (inactive)]

(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] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a7ea6d49bc69

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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]]

Comment on attachment 594302 [details] [diff] [review]
patch v3

[Triage Comment]
Test fix for Aurora 12 - approved.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c99d404f4267