Closed Bug 829817 Opened 7 years ago Closed 6 years ago

Leak when CSSOM modifies an @page rule

Categories

(Core :: CSS Parsing and Computation, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox23 --- affected
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: mccr8)

References

Details

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

Attachments

(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]]-->
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.
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.

https://tbpl.mozilla.org/?tree=Try&rev=97a1061a7467
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

>+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
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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a41df37be7e
https://hg.mozilla.org/mozilla-central/rev/7a41df37be7e
Status: NEW → RESOLVED
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.