social sidebar tests are marked as "leaking" the sidebar document

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: markh)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

See bug 775380. This applies to browser/base/content/test/browser_social_sidebar.js (and browser/base/content/test/browser_social_mozSocial_API.js which I'll be adding in bug 773351).

This seems similar to bug 759711 and/or bug 728426. The tests trigger the unhiding of the sidebar, which loads a URL in its browser. When the sidebar is hidden again, the sidebar browser is hidden and set to load about:blank.

For the moment I've just disabled the tests in debug builds. Hopefully bug 728294 will help with this? I'm not sure.
Now that bug 728426 has been fixed I think this leak could be gone. You still might be blamed for leaking the sidebar's outer window as long as bug 728294 hasn't landed but that should still be within the leak threshold.
(In reply to Tim Taubert [:ttaubert] from comment #1)
> Now that bug 728426 has been fixed I think this leak could be gone.

Nope. For some to-me-unknown reason, the social UI doesn't use the standard sidebar infrastructure but has its own sidebar on the right side of the browser.
Yes, we'll need to port bug 728426's fix to the social sidebar code.
(Assignee)

Comment 4

5 years ago
Created attachment 645144 [details] [diff] [review]
prevent "leaking" of the sidebar document.

This patch is on top of the patch in bug 773351 - it creates a new AboutBlankContentViewer before resetting to about:blank and re-enables the test on debug builds.  I'll request review once 773351 lands.
Blocks: 776554
(Assignee)

Comment 5

5 years ago
Created attachment 647076 [details] [diff] [review]
updated against trunk

This version also removes the "skip if debug" check from browser_social_sidebar.js
Attachment #645144 - Attachment is obsolete: true
Attachment #647076 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

5 years ago
Created attachment 647138 [details] [diff] [review]
Don't enable test in mozSocial_api due to bug 778639
Attachment #647076 - Attachment is obsolete: true
Attachment #647076 - Flags: review?(gavin.sharp)
Attachment #647138 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

5 years ago
Comment on attachment 647138 [details] [diff] [review]
Don't enable test in mozSocial_api due to bug 778639

*sigh* - it's not clear that this patch has any effect on the reported leaks in current builds.  However, IMO this should be a priority so we can get all our tests running in debug builds to prevent things like the "compartment mismatch" issue screwing us.
Attachment #647138 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 649989 [details] [diff] [review]
Avoid skipping social tests in debug builds.

It appears bug 728294 prevented these "leaks" from being reported.  I pushed the attachment to try at https://tbpl.mozilla.org/?tree=Try&rev=fb7acc56fb24 and the builds that have already finished look green (except for one orange that looks unrelated).
Attachment #647138 - Attachment is obsolete: true
Attachment #649989 - Flags: review?(gavin.sharp)
Comment on attachment 649989 [details] [diff] [review]
Avoid skipping social tests in debug builds.

I noticed this recently too! (https://tbpl.mozilla.org/?tree=Try&rev=d222940df46c)
Attachment #649989 - Flags: review?(gavin.sharp) → review+
No longer blocks: 776554
Blocks: 781386
Assignee: nobody → mhammond
No longer blocks: 781386
Blocks: 781386
https://hg.mozilla.org/integration/mozilla-inbound/rev/a461b5f53b20
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/a461b5f53b20
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.