Closed Bug 289379 Opened 19 years ago Closed 19 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: ajschult784, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

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)?
Attached file stacktrace
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... ;)
Flags: blocking1.8b2?
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta2
Blocks: 286000
Assignee: events → bzbarsky
Status: NEW → ASSIGNED
Attached patch Same as diff -wSplinter Review
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.
Attachment #179927 - Flags: superreview?(jst)
Attachment #179927 - Flags: review?(brade)
Summary: chatzilla crashes after /quit → [FIX]chatzilla crashes after /quit
*** Bug 289343 has been marked as a duplicate of this bug. ***
Blocks: 289336
Blocks: 289430
Flags: blocking1.8b2? → blocking1.8b2+
Blocks: 289613
Blocks: 289507
FYI: patch from attachment 179927 [details] [diff] [review] doesn't fix a crash in bug 289507 (which is
marked as depending on this bug).
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).
Blocks: 289573
Blocks: 289701
Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

sr=jst
Attachment #179927 - Flags: superreview?(jst) → superreview+
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?
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 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.
Blocks: 289704
> 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.
*** Bug 289812 has been marked as a duplicate of this bug. ***
(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.
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).
Summary: [FIX]chatzilla crashes after /quit → [FIX]chatzilla crashes after /quit [@ nsTextEditorFocusListener::Blur ]
Attachment #179927 - Flags: review?(brade) → review+
Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

Requesting approval for 1.8b for this crash fix...
Attachment #179927 - Flags: approval1.8b2?
Comment on attachment 179927 [details] [diff] [review]
Same as diff -w

a=asa
Attachment #179927 - Flags: approval1.8b2? → approval1.8b2+
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
*** Bug 289430 has been marked as a duplicate of this bug. ***
*** Bug 289573 has been marked as a duplicate of this bug. ***
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
*** Bug 289993 has been marked as a duplicate of this bug. ***
*** Bug 289507 has been marked as a duplicate of this bug. ***
No longer blocks: 289613
Crash Signature: [@ nsTextEditorFocusListener::Blur ]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: