Closed Bug 735574 Opened 11 years ago Closed 11 years ago

Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener coming from New Tab Page (mochitest-other)

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 14
Tracking Flags:
Tracking Status
firefox13 - affected

People

(Reporter: philor, Assigned: ttaubert)

References

Details

(Keywords: intermittent-failure, memory-leak, regression, Whiteboard: [fixed by bug 738774] [MemShrink:P1])

Attachments

(1 file)

patch v1 [Checked in: Comment 47]
5.63 KB, patch
dietrich
: review+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/?tree=Fx-Team&rev=c7e4db80d280 is when bug 729111 landed. Note both the unstarred leak (until I star it as this) in WinXP debug and the starred failure on 10.5 debug mochitest-other, which was both a test failure and this leak, but only starred as the test failure.

https://tbpl.mozilla.org/?rev=ee4e0c98cb02 was when it was merged to mozilla-central, with just one leak where I retriggered 10.5 debug while chasing this backward. From there, though, it explodes.

https://tbpl.mozilla.org/php/getParsedLog.php?id=10026260&tree=Fx-Team&full=1
Rev3 WINNT 5.1 fx-team debug test mochitest-other on 2012-03-13 00:46:30 PDT for push c7e4db80d280

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 36892 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of Mutex with size 12 bytes each (24 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 9 instances of ProxyListener with size 16 bytes each (144 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of ReentrantMonitor with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of SharedScriptableHelperForJSIID with size 12 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of XPCNativeScriptableInfo with size 8 bytes each (16 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 9 instances of XPCNativeScriptableShared with size 232 bytes each (2088 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 253 instances of XPCWrappedNative with size 56 bytes each (14168 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 24 instances of XPCWrappedNativeProto with size 32 bytes each (768 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 9 instances of imgCacheValidator with size 60 bytes each (540 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 18 instances of imgRequest with size 176 bytes each (3168 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 9 instances of imgRequestProxy with size 68 bytes each (612 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDocShell::InterfaceRequestorProxy with size 16 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 45 instances of nsJSCID with size 52 bytes each (2340 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 177 instances of nsJSIID with size 24 bytes each (4248 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 18 instances of nsPrincipal with size 56 bytes each (1008 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 9 instances of nsProgressNotificationProxy with size 28 bytes each (252 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 18 instances of nsProperties with size 8 bytes each (144 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 19 instances of nsSimpleURI with size 72 bytes each (1368 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 56 instances of nsStringBuffer with size 8 bytes each (448 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsSystemPrincipal with size 16 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 36 instances of nsTArray_base with size 4 bytes each (144 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 28 instances of nsVoidArray with size 4 bytes each (112 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsWeakReference with size 16 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsXPCComponents with size 60 bytes each (120 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsXPCComponents_Classes with size 20 bytes each (40 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsXPCComponents_Interfaces with size 28 bytes each (56 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsXPCComponents_Results with size 20 bytes each (40 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsXPCComponents_Utils with size 24 bytes each (48 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 18 instances of nsXPCWrappedJS with size 64 bytes each (1152 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsXPCWrappedJSClass with size 44 bytes each (88 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 172 instances of xptiInterfaceInfo with size 20 bytes each (3440 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of xptiInterfaceInfoManager with size 128 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of xptiWorkingSet with size 88 bytes
Since might be responsible for causing the bug, how should I tackle the cause of this issue?

(I also commented in bug 729111 https://bugzilla.mozilla.org/show_bug.cgi?id=729111#c20)
Changes for bug 729111 has been backed out of Mozilla-Inbound:
https://bugzilla.mozilla.org/show_bug.cgi?id=729111#c22
(Copied from https://bugzilla.mozilla.org/show_bug.cgi?id=729111#c24)

It looks like the build was still failing after the changes were backed out:
114c4148c6f3 Justin Lebar – Back out bug 729111 (rev c7e4db80d280) due to suspected randomorange (bug 735574).
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=114c4148c6f3

But the next build seems to pass:
d067c50e01dc Ehsan Akhgari – Backout changeset ea6be5f60c42 (bug 722946) for breaking Windows builds
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d067c50e01dc

Would that mean the problem is not caused by bug 729111?
https://tbpl.mozilla.org/php/getParsedLog.php?id=10072404&tree=Mozilla-Inbound
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 62552 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of Mutex with size 24 bytes each (48 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 5 instances of ProxyListener with size 32 bytes each (160 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of ReentrantMonitor with size 32 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of SharedScriptableHelperForJSIID with size 24 bytes
https://tbpl.mozilla.org/php/getParsedLog.php?id=10069704&tree=Firefox

Apparently I put too much faith in the number of debug mochitest-others that had run on the push before Charles' - it's already showing this same leak on more retriggers. Sorry, we'll get you relanded.
https://tbpl.mozilla.org/php/getParsedLog.php?id=10076178&tree=Firefox
Summary: Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener and more after landing of bug 729111 → Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener coming from fx-team
Thanks Phil for the clarifications. :)
So, two pushes before that was https://tbpl.mozilla.org/?tree=Fx-Team&rev=bb271ef702c6 which says "no, no, can't say as I'd ever be willing to leak that way," and one push before it was https://tbpl.mozilla.org/?tree=Fx-Team&rev=b2a4560c0af0 which grudgingly admits that it would in fact leak exactly like that. Hello, bug 729878.
Assignee: nobody → ttaubert
Blocks: 729878
No longer blocks: 729111
Component: Video/Audio Controls → General
Product: Toolkit → Firefox
QA Contact: video.audio → general
This seems to fix the leak for me. Couldn't reproduce it on try though it was really frequent on pushes for other patches.
Attachment #606263 - Flags: review?(dietrich)
Comment on attachment 606263 [details] [diff] [review]
patch v1
[Checked in: Comment 47]

Review of attachment 606263 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/NewTabUtils.jsm
@@ +549,5 @@
>    /**
>     * Resets the links cache.
>     */
>    resetCache: function Links_resetCache() {
> +    this._links = null;

hm, iirc you need to empty *and* null out the array. or that used to be the case. but since you're not seeing leaks anymore, consider this a note in case they come back ;)

@@ +583,5 @@
>   * Singleton that provides the public API of this JSM.
>   */
>  let NewTabUtils = {
>    /**
> +   * Restores all sites that haven been removed from the grid.

nit: typo

@@ +591,5 @@
>      Links.resetCache();
>      PinnedLinks.resetCache();
>      BlockedLinks.resetCache();
> +
> +    Links.populateCache(function () { AllPages.update() }, true);

hm, either break into lines or can you do Links.populateCache(AllPages.update, true) ?
Attachment #606263 - Flags: review?(dietrich) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #32)
> >    resetCache: function Links_resetCache() {
> > +    this._links = null;
> 
> hm, iirc you need to empty *and* null out the array. or that used to be the
> case. but since you're not seeing leaks anymore, consider this a note in
> case they come back ;)

Ok, I'll keep that in mind.

> > +   * Restores all sites that haven been removed from the grid.
> 
> nit: typo

Fixed.

> > +
> > +    Links.populateCache(function () { AllPages.update() }, true);
> 
> hm, either break into lines or can you do
> Links.populateCache(AllPages.update, true) ?

I'd have to .bind(this) so I just broke into lines.
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/13872068ea17
Whiteboard: [orange] → [orange][fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Doesn't seem to have gotten it, since https://tbpl.mozilla.org/php/getParsedLog.php?id=10107616&tree=Fx-Team is in the push after yours.
Whiteboard: [orange][fixed-in-fx-team] → [orange][leave open after merge]
Nominating for tracking because this is an "extremely frequent leak" in a new feature.  As far as I can tell, it hasn't been established whether this is the fault of the tests or the feature.
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
Whiteboard: [orange][leave open after merge] → [orange][leave open after merge][MemShrink]
If 13 is still affected, we should still track it for that 13.
tracking-firefox13: --- → ?
https://tbpl.mozilla.org/php/getParsedLog.php?id=10286743&tree=Firefox
Summary: Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener coming from fx-team → Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener coming from fx-team (mochitest-other)
 --> Updating summary to mention "new tab page" instead of just something vague "from fx-team", now this has been established to be coming from that particular feature. (based on Comment 24).
Summary: Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener coming from fx-team (mochitest-other) → Extremely frequent leak of 1 BackstagePass, 2 Mutex, 9 ProxyListener coming from New Tab Page (mochitest-other)
Depends on: 738774
Whiteboard: [orange][leave open after merge][MemShrink] → [orange][leave open after merge][MemShrink:P1]
Should be fixed by bug 738774.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [orange][leave open after merge][MemShrink:P1] → [orange][MemShrink:P1]
Attachment #606263 - Attachment description: patch v1 → patch v1 [Checked in: Comment 47]
Whiteboard: [orange][MemShrink:P1] → [fixed by bug 738774] [orange] [MemShrink:P1]
We went an entire day without any of these, so it looks like it is indeed fixed in 14.  Hooray.
If we're concerned about the possibility of leaks in user scenarios, please nominate for Aurora landing approval.
Nominated bug 738774 for aurora-approval. I think we're concerned about these leaks occurring in the wild but not totally sure they are.
Please nominate for Aurora uplift.
See comment 291, which says it was fixed by bug 738774, which was nominiated and approval-minused, so we're apparently just going to live with these test leaks until 13 falls off the end of the tracks.
Whiteboard: [fixed by bug 738774] [orange] [MemShrink:P1] → [fixed by bug 738774] [MemShrink:P1]
You need to log in before you can comment on or make changes to this bug.