Last Comment Bug 837033 - Leak with expando on ValidityState
: Leak with expando on ValidityState
Status: RESOLVED FIXED
[MemShrink]
: mlk, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla21
Assigned To: Andrew McCreight [:mccr8]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 326633 827158
  Show dependency treegraph
 
Reported: 2013-01-31 20:25 PST by Jesse Ruderman
Modified: 2013-02-06 03:41 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
testcase (91 bytes, text/html)
2013-01-31 20:25 PST, Jesse Ruderman
no flags Details
QI to participant (2.04 KB, patch)
2013-02-01 10:31 PST, Andrew McCreight [:mccr8]
bzbarsky: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-01-31 20:25:43 PST
Created attachment 708931 [details]
testcase

trace-refcnt reports a large leak, including nsGlobalWindow and nsDocument.
Comment 1 Olli Pettay [:smaug] 2013-02-01 00:00:23 PST
Is this a regression? And if so, from Bug 827158?
Comment 2 Andrew McCreight [:mccr8] 2013-02-01 05:22:47 PST
I don't know how this could be related, but HTMLObjectElement inherits from nsIConstraintValidation without traversing/unlinking mValidity.  Button element itself looks okay.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2013-02-01 06:05:44 PST
> Is this a regression? And if so, from Bug 827158?

Quite likely, since we didn't use to do wrapper preservation for ValidityState before that.

So the likely cycle here is preserved wrapper for validity state to JS object's parent (which is the JS object for the mConstraintValidation) to C++ HTMLButtonElement object to mValidity.

But it looks to me like we should traverse that cycle, and unlink it both by unpreserving the wrapper and by dropping mValidity.

So what gives?

This is eerily similar to the issue Ms2ger was running into with StyleSheetList...
Comment 4 :Ehsan Akhgari 2013-02-01 06:08:04 PST
(In reply to Andrew McCreight [:mccr8] from comment #2)
> I don't know how this could be related, but HTMLObjectElement inherits from
> nsIConstraintValidation without traversing/unlinking mValidity.  Button
> element itself looks okay.

You're right, I actually handled that in my patches there, but then left those hunks in the unlanded WIP patch over in that bug...  But that is not related to this bug.
Comment 5 Andrew McCreight [:mccr8] 2013-02-01 06:10:18 PST
> So what gives?
I can look at the CC graph to see what is missing later today.
Comment 6 Andrew McCreight [:mccr8] 2013-02-01 07:23:24 PST
> This is eerily similar to the issue Ms2ger was running into with StyleSheetList
What's the bug number for that?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2013-02-01 07:27:30 PST
It was just an IRC discussion.
Comment 8 Andrew McCreight [:mccr8] 2013-02-01 09:19:08 PST
As expected, the leak is rooted on a ValidityState object with two references, one unknown.  The known reference is from the Validity state's reflector.

0x123b064c0 [ValidityState]
    --[Preserved wrapper]-> 0x1199cc1c0 [JS Object (ValidityState)]
    --[parent]-> 0x1199c83d0 [JS Object (HTMLButtonElement)]
    --[parent]-> 0x1199c81f0 [JS Object (HTMLDocument)]
    ...

    Root 0x123b064c0 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x1199cc1c0 [JS Object (ValidityState)] --[UnwrapDOMObject(obj)]-> 0x123b064c0
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2013-02-01 09:31:45 PST
Per irc debugging just now, definitely fallout from the ValidityState change in bug 827158.  We're not actually traversing/unlinking anything in button because its QI impl doesn't mention CC stuff.
Comment 10 Andrew McCreight [:mccr8] 2013-02-01 10:31:41 PST
Created attachment 709126 [details] [diff] [review]
QI to participant

https://tbpl.mozilla.org/?tree=Try&rev=9f20b5e6446b
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2013-02-01 10:39:38 PST
Comment on attachment 709126 [details] [diff] [review]
QI to participant

r=me
Comment 12 Andrew McCreight [:mccr8] 2013-02-01 10:42:20 PST
I looked at all of the other classes modified in that patch, and they all look like they QI to ValidityState (they were CCed beforehand).

It would be nice if we could dynamically check that classes with participants QI to their participant, but I'm not sure how to do that.  Filed bug 837182 for that.
Comment 13 Andrew McCreight [:mccr8] 2013-02-01 10:42:42 PST
Rather, "they all look like they QI to their participant".
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-02-01 15:03:08 PST
Too bad jst never reviewed bug 614238!
Comment 15 Andrew McCreight [:mccr8] 2013-02-01 15:08:49 PST
Ah right, that.  Want me to review that patch or unbitrot it, Kyle?  Just assign/flag me as appropriate and I'll deal with it eventually.
Comment 16 Andrew McCreight [:mccr8] 2013-02-01 15:11:35 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b52eded5b89
Comment 17 Phil Ringnalda (:philor) 2013-02-03 12:44:54 PST
https://hg.mozilla.org/mozilla-central/rev/3b52eded5b89

Note You need to log in before you can comment on or make changes to this bug.