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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: ttaubert)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 593766 [details] [diff] [review]
patch v1

Trivial patch.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #593766 - Flags: review?(dao)
(Reporter)

Updated

5 years ago
Attachment #593766 - Flags: review?(dao) → review+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/2b7cea9aa6a3
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
(Assignee)

Comment 3

5 years ago
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).
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 4

5 years ago
Created attachment 594128 [details] [diff] [review]
patch v2

Sent this patch to try and it works now on all platforms.
Attachment #593766 - Attachment is obsolete: true
Attachment #594128 - Flags: review?(dao)
(Reporter)

Comment 5

5 years ago
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?
(Assignee)

Comment 6

5 years ago
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.
Attachment #594128 - Attachment is obsolete: true
Attachment #594128 - Flags: review?(dao)
Attachment #594302 - Flags: review?(dao)
(Reporter)

Comment 7

5 years ago
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); });
Attachment #594302 - Flags: review?(dao) → review+
(Assignee)

Comment 8

5 years ago
(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
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a7ea6d49bc69
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 10

5 years ago
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.
Attachment #594302 - Flags: approval-mozilla-aurora?
Comment on attachment 594302 [details] [diff] [review]
patch v3

[Triage Comment]
Test fix for Aurora 12 - approved.
Attachment #594302 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

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

Updated

5 years ago
status-firefox12: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.