812422 - Valgrind on tbpl detects leak - 96 bytes are definitely lost (direct) with NS_NewComputedDOMStyle and nsGlobalWindow::GetComputedStyleHelper on the stack, due to leaking entire layout module

Status: RESOLVED WORKSFORME

(Core :: General, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: m-c changeset a761bfc192b5 Valgrind stack with line numbers

Valgrind detects a leak of 96 bytes (direct) with NS_NewComputedDOMStyle and nsGlobalWindow::GetComputedStyleHelper on the stack, see attached snippet which comes from:

https://tbpl.mozilla.org/php/getParsedLog.php?id=17064538&tree=Firefox&full=1

m-c changeset rev is a761bfc192b5.

It didn't happen in the previous Valgrind run here: https://tbpl.mozilla.org/?noignore=1&jobname=valgrind&rev=dd68409d7810

So I'm guessing the regression range is:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dd68409d7810&tochange=a761bfc192b5

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Guessing Core: DOM, please feel free to change components if necessary.

Comment 2

[Andrew McCreight [:mccr8]]

Bug 115199 is in the range, and has "style" in the name, but it doesn't seem to directly touch a computed style file.

I looked at the stack and there's some element stuff near the call site:
8256   nsCOMPtr<dom::Element> element = do_QueryInterface(aElt);
8257   NS_ENSURE_TRUE(element, NS_ERROR_FAILURE);
8258   nsRefPtr<nsComputedDOMStyle> compStyle =
8259     NS_NewComputedDOMStyle(element, aPseudoElt, presShell,
8260                            aDefaultStylesOnly ? nsComputedDOMStyle::eDefaultOnly :
8261                                                 nsComputedDOMStyle::eAll);
8262 
8263   *aReturn = compStyle.forget().get();
8264 

So maybe bug 811449 or bug 807075 are more likely.

Comment 3

[Peter Van der Beken [:peterv]]

Isn't that just because we're leaking nsLayoutStatics and don't deallocate the sCachedComputedDOMStyle as a result? I doubt the nsComputedDOMStyle is the real issue.

Comment 4

[Peter Van der Beken [:peterv]]

The leaking argument objects and XPCWrappedNativeScopes seem more serious to me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, Peter is right.  We leaked the layout module as a whole here....

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nominating for tracking since leaking the whole layout module sounds bad.

Comment 7

[Andrew McCreight [:mccr8]]

I'm not sure it is that bad. We always hold this alive until shutdown anyways.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> I'm not sure it is that bad. We always hold this alive until shutdown
> anyways.

Given this, not tracking for FF19. Please re-nominate if the landscape changes.

Comment 9

[Nicholas Nethercote [inactive]]

This is no longer occurring in Valgrind-on-TBPL runs.