Closed Bug 829817 Opened 7 years ago Closed 6 years ago

Leak when CSSOM modifies an @page rule


(Core :: CSS Parsing and Computation, defect)

Not set



Tracking Status
firefox23 --- affected
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: jruderman, Assigned: mccr8)



(Keywords: memory-leak, testcase, Whiteboard: [MemShrink])


(2 files, 1 obsolete file)

Attached file testcase
Split from bug 827591.  The leak can be seen using XPCOM_MEM_LEAK_LOG=2.
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.)
Group: core-security
Whiteboard: [sg:nse] unhide when bug 827591 is revealed → [sg:nse]
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.
Assignee: nobody → continuation
Whiteboard: [sg:nse] → [sg:nse][MemShrink]
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.
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]]-->

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.
It does indeed look like a regression from the initial landing of this PageRule stuff in bug 115199, which was Fx19.
Attached patch cycle collect nsCSSPageRule (obsolete) — Splinter Review
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.
Whiteboard: [sg:nse][MemShrink] → [MemShrink]
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.
Attachment #761812 - Attachment is obsolete: true
Comment on attachment 761820 [details] [diff] [review]
cycle collect nsCSSPageRule

Linux64 debug run looks good.
Attachment #761820 - Flags: review?(dbaron)
Comment on attachment 761820 [details] [diff] [review]
cycle collect nsCSSPageRule

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

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

Attachment #761820 - Flags: review?(dbaron) → review+
(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.
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.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.