22360 - Customize, Cancel, Click-bookmarks, crash.

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Morgan Schweers]

I'm using a copy of the binary that showed up in the releases/m12 directory on
the FTP site earlier today.  This happens with the fullcircle-enabled build as
well, and in fact a similar bug-report to this one was just fired off using
that.  The time for the one showing up on the ftp site is December 21,
1999, 16:27:00.  Fullcircle shows the build identifier as 1999122023.  (I'm
guessing that means the build was started at 11pm yesterday.)

Instructions to replicate crash:

Start mozilla.
Click on 'customize...' on the sidebar.
Click cancel.
Now click on the 'Bookmarks' button entry in the sidebar.
For me, at that point, it crashes very consistently.

Notes:
I have a reasonably large bookmarks file (~360K), and the toplevel tree has
enough entries to causes the sidebar to have a scrollbar for them.  Bookmarks
are also 'open' (that is the top level of the tree is visible) when I start.

The crash is:
Mozilla: mozilla.exe - Application Error
The instruction at "0x00eccec8" referenced memory at "0x00000000".  The memory
could not be "read".

Click OK...etc.

When I click Cancel for the debugging, after the debugger comes up, it says:
Unhandled exceptionin mozilla.exe (GKHTML.DLL): 0xC0000005: Access Violation.

The call stack is:
GKHTML! 00eccec8()
GKHTML! 00ecccb5()
RDF! 606487eb()
GKHTML! 00ecc3cb()
GKHTML! 00ecb719()
GKHTML! 00e49054()
GKVIEW! 602f1df9()
GKVIEW! 602f70bd()
GKVIEW! 602f2537()
GKWIDGET! 6096528b()
GKWIDGET! 60967c51()
GKWIDGET! 60967f90()
GKWIDGET! 6096558b()
USER32! 77e71820()

I can reproduce this with 100% consistency, if you need any other details.

Comment 1

[Morgan Schweers]

Greetings,
I grabbed the source and rebuilt it with debug information, and this is the
results...

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().:
'mRawPtr != 0', file ..\..\dist\include\nsCOMPtr.h, line 569

I debugged a bit, (the code in question is in
mozilla\layout\events\src\nsEventStateManager.cpp, line 1928), and it goes a bit
like this:
         nsCOMPtr<nsIDocument> doc;
         gLastFocusedContent->GetDocument(*getter_AddRefs(doc));
*        nsCOMPtr<nsIPresShell> shell = getter_AddRefs(doc->GetShellAt(0));

* = line that crashes, of course.

Now in GetDocument() (nsXULElement::GetDocument(...)) it turns out that
mDocument is NULL also, suggesting that the document has been deleted, but
gLastFocusedContent wasn't told about it.

Now here's the fun part...  I had previously also noticed a crash with opening
the find dialog, and cancelling.  I was all set to file a bug report, but there
is one already.  It's bug 22234.  Once I had a debug build, I replicated that,
getting curious about it's cause, the crash location is identical.  I also
noticed that you can reproduce this by clicking on virtually anything that would
take focus, after clicking the cancel in either of them.

I *don't* know the Mozilla source well enough at all yet, but I'm going to jump
to a bit of a conclusion...  Each of the dialogs being drawn are common
documents.  When they are cancelled, the documents are (reasonably) destroyed,
but the 'gLastFocusedContent' (global, presumably, by the preface) is not zeroed
or reset to a previous value?  The code I'm looking at
(nsEventStateManager:1928) is surrounded by an 'if (gLastFocusedContent)'
clause, which gets triggered in other cases, so presumably it expects that to be
zeroed if the last focused content doesnt exist anymore?  (A focus stack would
make more sense, but I don't know if that's been implemented or not, probably
not as this is the place that it would likely go...)

Potentially important is that I've noticed that closing the window using the
standard windows 'close' decoration at the top of the window does NOT cause
either of these bugs.  This suggests to me that the close button is doing the
Right Thing(tm) at some point that Cancel isn't.

I apologize in advance if this is useless rambling, but since both bugs seem to
have a common set of circumstances, and a common failure point, I imagine they
may be related, and intuitively what I've described is my best guess.

Hope it helps.

--  Morgan Schweers

Comment 2

[Morgan Schweers]

Greetings,
    I guess I should correct that, before anybody takes it seriously.  Of course
all the cancel functions are being handled by javascript, and just call
window.close().  The problem here is that window.close() directly calls the
(nsWebShellWindow *)->Close(); function.  Since it doesn't call through
HandleEvent, a SetPresContext never gets called.  Currently, SetPresContext
is one of the only places that does an NS_IF_RELEASE(gLastFocusedContent);  When
you close the window using the window control, it's sent as an event, and
handled properly, of course.

My gut reaction would be that window.close() should send a NS_XUL_CLOSE message
to the window, but I'm losing track now, and I looked at the source for the
first time at about 1am, about 5 hours ago, so I'm probably missing miles of
stuff.

    For what it's worth, if you uncomment lines 278/279 in
nsEventStateManager.cpp, the crash goes away.  However, that code is commented
out to fix bug 20524.  Truth be told, uncommenting it (while it works for me) is
the wrong answer, and I would argue that the right answer is that Close();
shouldn't be called directly, it should only go through the event handler.
Otherwise, you run the risk of similar problems in the future.  Better to have
only one place that calls Close (the event handler) than many.  Is this a sane
refactoring?  Unlikely.  If there's no resolution in a few days, I'll see if I
can suggest a better workaround that doesn't break a bugfix.

Oh, and sorry about all this spam.  I probably should have just worked on it
first, and spouted off later.  It is a bug though.

--  Morgan Schweers

Comment 3

[don]

Dan, this sounds like an XPToolkit issue ...

Comment 4

[don]

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

Comment 5

[Paul MacQuiddy]

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

Comment 6

[Paul MacQuiddy]

this one slipped thru the cracks for m12, but is fixed in the daily builds (if
you dare).

Comment 7

[Paul MacQuiddy]

verified with a 12/22 M13 tip build

Comment 8

[Joseph Elwell]

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

Comment 9

[Matthew T (active 1999-2002)]

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

Comment 10

[Paul MacQuiddy]

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