757749 - Leak with canvas, addEventListener, matchMedia

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase

According to trace-refcnt, this testcase makes Firefox leak nsGlobalWindow objects.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I just tried this with a slightly old build (from rev fb6109c9dae5) and that doesn't leak...

I'll try updating.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I can't reproduce this on linux.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, rev a4240610972e does show the leak.

Comment 4

[Boris Zbarsky [:bzbarsky]]

And now I see it with rev fb6109c9dae5 too.

My first guess would be that we end up with a cycle through the prescontext somehow, but I dunno exactly how...

Comment 5

[Andrew McCreight [:mccr8]]

A canvas element is being kept alive here somehow.  My money is on bungled refcounting in nsCanvasRenderingContext2DAzure. ;)

0x12626ded0 [nsGenericElement (xhtml) canvas https://bug757749.bugzilla.mozilla.org/attachment.cgi?id=626343]
    --[[via hash] mListenerManager]-> 0x12674f310 [nsEventListenerManager]
    --[mListeners[i] mListener]-> 0x12549b410 [nsXPCWrappedJS (nsIDOMEventListener)]
    --[mJSObj]-> 0x1184e58c0 [JS Object (Function)]
    --[script]-> 0x1184740e8 [JS Script]
    --[object]-> 0x118470060 [JS Object (Window)]
    --[xpc_GetJSPrivate(obj)]-> 0x126f42ba0 [XPCWrappedNative (Window)]
    --[mIdentity]-> 0x1264d5080 [nsGlobalWindow]

    Root 0x12626ded0 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x126697bc0 [nsGenericElement (xhtml) body https://bug757749.bugzilla.mozilla.org/attachment.cgi?id=626343] --[mAttrsAndChildren[i]]-> 0x12626ded0
       0x1264d3000 [nsCanvasRenderingContext2DAzure] --[mCanvasElement]-> 0x12626ded0

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: ref count tree, with balanced subtrees ignored

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: refcount tree

Comment 8

[Boris Zbarsky [:bzbarsky]]

The first refcount log shows outstanding refs from the canvas context (expected: they form a cycle), the parent element in the DOM, and what looks like GetCanvasLayer (via CanvasRenderingContext2DUserData)

Is it possible that the canvas layer is leaking for some reason?

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Is it possible that the canvas layer is leaking for some reason?
It could be, but it must not be in the CC graph.  That means either that it isn't a CCed class, or it was erroneously dropped.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I really hope layers aren't CCd objects!

Comment 11

[Boris Zbarsky [:bzbarsky]]

The layers aren't CCed.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

So isn't this then a dup of ... the one bug where prescontext ends up keeping layers alive
too long.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(or was that fixed?)

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #12)
> So isn't this then a dup of ... the one bug where prescontext ends up
> keeping layers alive too long.
Do you have a bug number for that?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I was thinking Bug 731868

Comment 16

[Andrew McCreight [:mccr8]]

That does sound very similar.

Comment 17

[Andrew McCreight [:mccr8]]

There are (In reply to Boris Zbarsky (:bz) from comment #8)
> Is it possible that the canvas layer is leaking for some reason?
There are 8 Layers leaking.  I'm not sure which is which, though.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can't reproduce this. Anyone got exact steps to reproduce?

Comment 19

[Andrew McCreight [:mccr8]]

I'm able to reproduce it by just opening the browser with the test case loading as the initial tab, then exiting the browser.  Maybe it is OSX only or something?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Windows and GTK2 nsWindow::Destroy are doing stuff like this:
  if (mLayerManager) {
    mLayerManager->Destroy();
  }
  mLayerManager = nsnull;

I'm guessing nsChildView::Destroy needs that somewhere too. I don't think that's happening right now.

Comment 21

[Andrew McCreight [:mccr8]]

I tried blindly putting that chunk of code into nsChildView::Destroy in a few places but it didn't fix the leak.  Somebody who actually knows some of this code should probably take over from here.

Comment 22

[Andrew McCreight [:mccr8]]

Ignore the first sentence of the previous comment.  I was managing to edit a different directory from the one I was running.  When I tried it for real, I ended up crashing in the cycle collector on shutdown while tearing down this view stuff:

mozilla::gl::TextureImageCGL::~TextureImageCGL() + 71 (GLContext.h:583)
nsChildView::~nsChildView() + 357 (nsChildView.mm:246)
nsBaseWidget::Release() + 282 (nsBaseWidget.cpp:56)
nsChildView::Release() + 24 (nsChildView.mm:251)
nsPresContext::cycleCollection::Unlink(void*) + 1102 (mozalloc.h:224)

~nsChildView() has some comment about doing some cleanup in case Destroy() hasn't been called, so presumably my random nulling out has upset something there.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed in bug 731868. CanvasRenderingContext2DUserData(Azure) no longer has an owning reference to anything.