829817 - Leak when CSSOM modifies an @page rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Split from bug 827591.  The leak can be seen using XPCOM_MEM_LEAK_LOG=2.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One thing that's interesting about this testcase:  paddingLeftLTRSource isn't an actual getter/setter.  So what the testcase is doing is setting an expando.  (Changing it to "foobar" still shows the leak.)

Comment 2

[Andrew McCreight [:mccr8]]

This feels vaguely like bug 880862, in that it is a leak involving an expando with layout-y stuff.

Here's the CC log:

0x12740cec0 [nsCSSPageStyleDeclaration]
    --[Preserved wrapper]-> 0x118bec340 [JS Object (Proxy)]
    --[type_proto]-> 0x118bd0a60 [JS Object (CSS2PropertiesPrototype)]
    --[getter]-> 0x12b7185c0 [JS Object (Function)]
    --[parent]-> 0x118b34240 [JS Object (Window)]
    --[xpc_GetJSPrivate(obj)]-> 0x12ab437b0 [XPCWrappedNative (Window)]
    --[mIdentity]-> 0x126517c00 [nsGlobalWindow #13]

    Root 0x12740cec0 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x118bec340 [JS Object (Proxy)] --[UnwrapDOMObject(obj)]-> 0x12740cec0

So we're missing a reference to the nsCSSPageStyleDeclaration.

Comment 3

[Andrew McCreight [:mccr8]]

ref count tree is straightforward:

2       XPC_WN_GetterSetter(JSContext*, unsigned int, JS::Value*) (xpcprivate.h:2542, in XUL)
  3       XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2234, in XUL)
  | 2       CallMethodHelper::Call() (XPCWrappedNative.cpp:2930, in XUL)
  | | 2       NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:162, in XUL)
  | |   1       non-virtual thunk to nsCSSPageRule::GetStyle(nsIDOMCSSStyleDeclaration**) (nsAutoPtr.h:865, in XUL)
  | |   1       non-virtual thunk to nsCSSPageRule::GetStyle(nsIDOMCSSStyleDeclaration**) (nsCSSRules.cpp:2842, in XUL)
  | 1       CallMethodHelper::GatherAndConvertResults() (XPCWrappedNative.cpp:2452, in XUL)
  |   1       XPCConvert::NativeData2JS(JS::Value*, void const*, nsXPTType const&, nsID const*, tag_nsresult*) (XPCConvert.cpp:319, in XUL)
  |     1       XPCConvert::NativeInterface2JSObject(JS::Value*, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, tag_nsresult*) (RootingAPI.h:573, in XUL)
  |       1       mozilla::dom::CSS2PropertiesBinding::Wrap(JSContext*, JS::Handle<JSObject*>, nsDOMCSSDeclaration*, nsWrapperCache*) (CSS2PropertiesBinding.cpp:17227, in XUL)
  -1      XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2285, in XUL)
    -1      CallMethodHelper::~CallMethodHelper() (XPCWrappedNative.cpp:2326, in XUL)

nsCSSPageRule lazily creates the style, sticks it in mDOMDeclaration, addrefs, and returns it.  Presumably the return is put in CallMethodHelper, then cleared up in the destructor, and the Wrap is shown in the CC log, meaning that the one remaining unexplained reference is from nsCSSPageRule::mDOMDeclaration.

Comment 4

[Andrew McCreight [:mccr8]]

With another round of refcount logging, and some printfs to confirm it, the cycle appears to be:

nsCSSStyleSheet --[mInner, then mOrderedRules]-->
nsCSSPageRule --[mDOMDeclaration]-->
nsCSSPageStyleDeclaration --[Preserved wrapper]-->
JS Object --[...various JS...]-->
nsDocument --[mStyleSheets[i]]-->
nsCSSStyleSheet

So there's the cycle.  All of this is in the CC graph, except for the nsCSSPageRule.  I think the problem is that nsCSSPageRule should be cycle collected.

Comment 5

[Andrew McCreight [:mccr8]]

It does indeed look like a regression from the initial landing of this PageRule stuff in bug 115199, which was Fx19.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: cycle collect nsCSSPageRule

This fixes the leak.  I still need to add the test case, as well as audit the other Rules, as there are quite a few of them.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: cycle collect nsCSSPageRule

Now with test.  I looked at the classes that inherit from Rule and GroupRule and they looked okay.  I'm not entirely sure about MediaRule, but it seems like nsMediaList can't contain anything that might cyclic.

https://tbpl.mozilla.org/?tree=Try&rev=97a1061a7467

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 761820 [details] [diff] [review]
cycle collect nsCSSPageRule

Linux64 debug run looks good.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 761820 [details] [diff] [review]
cycle collect nsCSSPageRule

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsCSSPageRule)
>+  if (tmp->mDOMDeclaration) {
>+    tmp->mDOMDeclaration->DropReference();
>+    ImplCycleCollectionUnlink(tmp->mDOMDeclaration);
>+  }
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END

I'm hoping ImplCycleCollectionUnlink sets tmp->mDOMDeclaration to nullptr.  If it doesn't, please do so.

r=dbaron

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to David Baron [:dbaron] (in W3C meetings through June 11) (don't cc:, use needinfo? instead) from comment #9)
> I'm hoping ImplCycleCollectionUnlink sets tmp->mDOMDeclaration to nullptr. 
> If it doesn't, please do so.
Yeah, I think that's all it does.  I'll just change that line to explicitly set the null, as using CC weirdness here doesn't help clarify anything, when we're already doing odd things.

Comment 11

[Andrew McCreight [:mccr8]]

I replaced the weird bare call to ImplCycleCollectionUnlink with an explicit nulling out, here and for the frame rule unlink that I had cribbed this from anyways.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a41df37be7e

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a41df37be7e