289379 - [FIX]chatzilla crashes after /quit [@ nsTextEditorFocusListener::Blur ]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Andrew Schultz]

After loading chatzilla and typing "/quit" to quit, chatzilla crashes
The crash occurs with linux seamonkey trunk build 2005040605, build 2005040505
is ok.

could this be bug 286000 (nsGenericElement is the highest frame in the stack
that got touched in that window)?

Comment 1

[Andrew Schultz]

Attachment: stacktrace

Comment 2

[Boris Zbarsky [:bzbarsky]]

So here's the problem.  When we're closing the window, we end up in
nsDocShell::Destroy, which calls Close() and Destroy() on the document viewer,
in that order.  The former call sets the script global on the document to null,
which incidentally makes the document remove all its kids.  The latter call
tears down the presshell.

So by the time we go to tear down the frame tree, all the document's content has
already been unbound.  Now unbinding the document content messes with the anon
content list for it in the presshell.  In particular, it used to clear the
document pointer in the anon content list, but I changed it to also clear the
parent pointer (otherwise that can dangle).  In this case, what that means is
that by the time we're tearing down the presshell the anonymous div inside the
text input has no pointer to its parent.

Now when we tear down the text control frame we also tear down the editor on it.
 This attempts to remove the event listeners it's registered, but it can't get
to the node it registered them on (that would be the ex-parent of that anonymous
div).  So it ends up thinking the document is its event receiver, which doesn't
get it very far as far as removing listeners goes.

I'm still not sure how we manage to actually crash, since the event listener
manager should be holding refs to its listeners, etc, but we're ending up
calling a method on a deleted event listener _somehow_, according to valgrind...

The question is, how to fix this.  I suppose we could undo the "clear parent
pointer" change for now, and leave it dangling as it was before...  I'd also
have to either remove the "clear out anon content" line I added or hack that
code to also leave the parent pointer.  Alternately, the editor could store a
pointer to its event receiver when it attaches the listeners (but could that
cause leaks?  We hold a strong ref to mRoot, but a weak one to mDocument; I
suppose I could just cache the event receiver if it's not the document).  The
third option is to sort out shutdown ordering here so we kill off frames before
unbinding the content, which is the order we do it in elsewhere.

jst, sicking, thoughts on what we want to do for the short term?  Sorting out
shutdown ordering isn't what I'd prefer to do for 1.8b... ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch to editor to fix this crash

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Some of this is just shuffling members around to use less memory.  The real
patch is adding a cached DOMEventReceiver pointer and a boolean that says
whether to use it.

Comment 5

[Adam Hauner]

*** Bug 289343 has been marked as a duplicate of this bug. ***

Comment 6

[kmike]

FYI: patch from attachment 179927 [details] [diff] [review] doesn't fix a crash in bug 289507 (which is
marked as depending on this bug).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, that bug has little to do with this one.  This one (and most of its deps)
is a regression from bug  286000.  That bug has been around for a while now. 
It's just marked dependent because it can't be tested without this patch,
probably (because you'd just see the crash from this bug).

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

sr=jst

Comment 9

[Stefan Persson]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050408

^L, type in URL, RET = crash.  Maybe a result of the same bug?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Possible, but unlikely (since I can't reproduce that, and I _can_ reproduce this
bug).  File a new bug, cc me, post the talkback incident id or stack trace from
a build with symbols?

Comment 11

[[e-mail]]

Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

> already_AddRefed<nsIDOMEventReceiver>
> nsEditor::GetDOMEventReceiver()
I don't see why you need mGotDOMEventReceiver.
Something along these lines would seem to suffice:
if (!mDOMEventReceiver)
{
  nsCOMPtr<nsIContent> content(do_QueryInterface(GetRoot()));
  if (content->IsNativeAnonymous())
  {
    mDOMEventReceiver = do_QueryInterface(content->GetParent());
  }
}
nsIDOMEventReceiver *erp = nsnull;
if (mDOMEventReceiver)
{
  NS_ADDREF(erp = mDOMEventReceiver);
}
else
{
  CallQueryReferent(mDocWeak, &erp);
}
return erp;

Nit: you didn't use editor bracing style.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I don't see why you need mGotDOMEventReceiver.

Because I don't want to bet that we'll never end up with a new parent on our
root after we'd gotten the event receiver off our document once.  That would
land us back where we came from, with not removing our event listeners.

> Nit: you didn't use editor bracing style.

In the first block?  Will fix.

Comment 13

[Jo Hermans]

*** Bug 289812 has been marked as a duplicate of this bug. ***

Comment 14

[[e-mail]]

(In reply to comment #12)
>>I don't see why you need mGotDOMEventReceiver.
>
>Because I don't want to bet that we'll never end up with a new parent on
>our root after we'd gotten the event receiver off our document once.
You could always fix the code to use IsNativeAnonymous to figure out whether to
get the event receiver from the document or the parent? Seems to work for me.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah, if I weren't trying to change the logic as little as possible I might do
that.  But given that I don't fully understand the use cases this code is trying
to address, I'm not sure that behavior change is ok...  Certainly not for 1.8b2
(unless someone who understands all this a lot better than I does it).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

Requesting approval for 1.8b for this crash fix...

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

a=asa

Comment 18

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050411
Firefox/1.0+ 11:38PDT (patched included)

Fixed for me

Comment 19

[Boris Zbarsky [:bzbarsky]]

*** Bug 289430 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

*** Bug 289573 has been marked as a duplicate of this bug. ***

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fix checked in.

Comment 22

[Jo Hermans]

*** Bug 289993 has been marked as a duplicate of this bug. ***

Comment 23

[kmike]

*** Bug 289507 has been marked as a duplicate of this bug. ***