"ASSERTION: Event listener manager hash not empty at shutdown!" with Flash and Greasemonkey

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
Plug-ins
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: Jesse Ruderman)

Tracking

(Blocks: 1 bug, {assertion, memory-leak, testcase})

Trunk
mozilla1.9beta2
x86
Mac OS X
assertion, memory-leak, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

(Assignee)

Description

10 years ago
Loading the testcase (in Mac trunk debug Firefox) triggers:

###!!! ASSERTION: Event listener manager hash not empty at shutdown!: 'sEventListenerManagersHash.entryCount == 0', file /Users/jruderman/trunk/mozilla/content/base/src/nsContentUtils.cpp, line 708

It also makes Firefox leak 1 nsEventListenerManager, 1 nsGenericElement, 1 nsHTMLSharedObjectElement, and a bunch of other objects.
(Assignee)

Comment 1

10 years ago
Created attachment 286425 [details]
testcase
(Assignee)

Comment 2

10 years ago
Created attachment 286426 [details]
debug output: warnings, leak log
You'll get that assert any time you leak an element, basically.

Can you get a balance tree for that nsHTMLSharedObjectElement leak?  And possibly for the nsPluginInstanceOwner and ns4xPluginInstance?

I would assume the  nsPluginInstanceOwner leaks the element, but not sure how the plugin stuff ends up leaking itself here.

Comment 4

10 years ago
This doesn't leak for me.
(Assignee)

Comment 5

10 years ago
Created attachment 286462 [details]
balance tree for the leaked nsHTMLSharedObjectElement

This is my first time trying to make a balance tree, so I don't know whether I did it correctly.
Hmm.  What command line did you use?  That tree has some really odd things going on...
(Assignee)

Comment 7

10 years ago
The command line is included in the attachment.
Ah, ok. Try something more like:

  export XPCOM_MEM_REFCNT_LOG=log-file.dat
  export XPCOM_MEM_COMPTR_LOG=log-comptr.dat
  export XPCOM_MEM_LOG_CLASSES=nsHTMLSharedObjectElement

Run firefox.  Run find-leakers.pl on log-file.dat to find the address.

  make-tree.pl --object ADDRESS_HERE --comptr log-comptr.dat --ignore-balanced < ~/log-file.dat | fix-macosx-stack.pl

The difference with what you did is logging all the nsCOMPtr activity and getting rid of the matching addrefs/releases from that even if they're in different parts of the balance tree.  Makes the tree smaller and simpler to read.
(Assignee)

Comment 9

10 years ago
Created attachment 286590 [details]
ignoring nsCOMPtr activity

Ignoring nsCOMPtr activity made the whole tree go away?
(Assignee)

Comment 10

10 years ago
Created attachment 286592 [details]
balance tree for the leaked nsPluginInstanceOwner

This log makes more sense.  There's even some stuff at the bottom involving "scroll position listeners".
(In reply to comment #9)
> Ignoring nsCOMPtr activity made the whole tree go away?

When that's the case, generally find-comptr-leakers will show some leaks.
(Assignee)

Comment 12

10 years ago
find-comptr-leakers.pl tells me that it's the nsPluginInstanceOwner's mContent pointer that continues to hold onto the nsHTMLSharedObjectElement.
(Assignee)

Comment 13

10 years ago
nsPluginInstanceOwner::ScrollPositionDidChange's use of timers is Mac-specific.
Note that the leak log (XPCOM_MEM_LEAK_LOG) will say for the leaked things how many refs were from COMPtrs, so you can tell whether to use find-comptr-leakers or find-leakers.
Hmm.  So there are all sorts of CancelTimer() calls in nsPluginInstanceOwner.  So I'd expect this cycle to be broken...  Why aren't we hitting any of them?  Should the frame Destroy() method make sure the timer gets killed?
Flags: blocking1.9?
And for what it's worth, I can't reproduce on Mac either.
(Assignee)

Comment 17

10 years ago
Hmm, so it's up to me to debug it, then?
If it's consistently reproducible, yes....

I'd start by adding printfs to the places where the timer is set up and to CancelTimer and seeing whether the latter is being hit at all, I guess.

Does this perhaps depend on Flash version?
Jesse, here's your first blocker :)

Let me know if you need help and I'll do my best.
Assignee: nobody → jruderman
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 20

10 years ago
I can't reproduce in a completely clean profile, but once I install Greasemonkey (even without installing any user scripts), I can reproduce.

It seems like two nsPluginInstanceOwner objects are being created for the same frame, and one of them isn't getting torn down properly.  More soon.
Summary: "ASSERTION: Event listener manager hash not empty at shutdown!" with Flash → "ASSERTION: Event listener manager hash not empty at shutdown!" with Flash and Greasemonkey
(Assignee)

Comment 21

10 years ago
I might have figured out what's going wrong.  nsPluginInstanceOwner::PrepareToStop is called on the first nsPluginInstanceOwner object, but mWidget is null, so PrepareToStop bails out early and doesn't unregister its scroll listener.  Later, the scroll listener is notified, setting up a cycle between the timer and the nsPluginInstanceOwner.  nsPluginInstanceOwner::Destroy doesn't get to break this cycle because it is called before the cycle is set up.

If I change the mWidget null check in nsPluginInstanceOwner::PrepareToStop to only wrap the code added in bug 347743 (which uses mWidget) instead of bailing early (and skipping the "Unregister scroll position listener" code), the leak goes away.  Eli and jst, do you remember why you made the null check do that in bug 347743?

I don't know why Greasemonkey causes two nsPluginInstanceOwners to be created, though.  I wonder if that's related to bug 390183.
Blocks: 347743
(Assignee)

Comment 22

10 years ago
Created attachment 287472 [details] [diff] [review]
patch to add printfs (not for review)

This is what I used to debug the leak so far.
(Assignee)

Comment 23

10 years ago
Created attachment 287473 [details]
output with that patch
(Assignee)

Comment 24

10 years ago
Created attachment 287474 [details] [diff] [review]
patch that fixes the leak
Attachment #287474 - Flags: superreview?(jst)
Attachment #287474 - Flags: review?(sharparrow1)

Updated

10 years ago
Attachment #287474 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M10
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
> It seems like two nsPluginInstanceOwner objects are being created 

What are the stacks to those creations?
(Assignee)

Comment 26

10 years ago
Created attachment 287509 [details]
stacks to nsPluginInstanceOwner creations
Uh.  So we reenter nsObjectFrame::Instantiate, looks like.  I think we need to flag ourselves as being inside Instantiate and bail out if it's reentered.  Probably a separate (blocking1.9) bug on that?

It looks like greasemonkey installs a content policy implementation?  I wonder what they do in it...
Huh.  OK, that's kinda cute.
(Assignee)

Comment 30

10 years ago
Are Greasemonkey's nsIContentPolicy callbacks doing anything that's disallowed?   Do we have assertions to catch that kind of thing?

Boris, can you file the bug you mentioned in comment 27?  I think you can describe it better than I can.
> Are Greasemonkey's nsIContentPolicy callbacks doing anything that's disallowed?

Not offhand, no.

> Do we have assertions to catch that kind of thing?

For the most part, yeah.

I filed bug 402937.

Attachment #287474 - Flags: review?(sharparrow1) → review+
(Assignee)

Comment 32

10 years ago
Patch checked in.

For automated testing: once bug 397725 is fixed, I can check the testcase in as a crashtest, but the tests would have to be run with Greasemonkey installed to trigger the assertion and leak.  Hmm.
Group: security
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.