577963 - [10.6] Crash [@ nsContentEventHandler::InitCommon ]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

These crashes are Mac-only, and happen only on OS X 10.6.X.  So
they're likely to be caused by one or more OS bugs.  But I'm calling
this a Cocoa widgets bug, since we often work around OS bugs in Cocoa
widgets code.

The earliest build for which I can find a crash is 20100422030619, but
I'm not sure how meaningful that is.  The number of these crashes
picks up substantially in the last month or so, particularly with the
4.0b1 release (the 20100630131607 build), but that may just be because
4.0b1 has many more users than preceeding or succeeding nightlies.

All of these crashes result from trying to dereference a NULL
nsISelection pointer (mSelection).  The simplest fix would be to test
for a NULL mSelection after the call to
nsCopySupport::GetSelectionForCopy().  (Right now there's only an
assertion if mSelection is NULL.)

Comment 1

[Steven Michaud [:smichaud] (Retired)]

CCing some people who've worked recently on the code where the crash
happens.  What do you guys think of my suggestion (from comment #0) to
fix this bug by checking for a NULL mSelection (and returning some
kind of error) if nsCopySupport::GetSelectionForCopy() sets mSelection
to NULL?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Quite a few comments report this crash while trying to customize the toolbar.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Would you tell us some examples of this crash? I want to know what situation can have no selection.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> Would you tell us some examples of this crash?

I don't have any, because I can't (yet) reproduce this crash.

> I want to know what situation can have no selection.

I don't know, and (unfortunately) I may never know.

I will try to find a way to reproduce this crash and answer your
question.  But my question (from comment #1) is this:  If I'm not able
to answer your question (and find a more narrowly-targeted fix), would
it be reasonable to fix this bug by returning some kind of error if
nsCopySupport::GetSelectionForCopy() sets mSelection to NULL?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I meant that I want to know which crash reports are this bug. So, would you list up a couple of stack traces?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Oh, OK.  Here are two:

bp-524c86e6-7a78-498d-ba57-315672100707
bp-1d35e56c-8c33-4ebd-a922-6f6222100707

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, looks like when XUL menu is hiding, it's crashed. At this time, IMK sends an input event for a window. Then, the event handler calls |selectedRange|.

Looks like that except OOM cases, only when the shell has been already hidden, nsCopySupport::GetSelectionForCopy() returns NULL. And looks like that only when the window stored by bfcache, the shell is hidden. So, I guess that back or forward in context menu causes the crash?

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I'm going to be gone for the rest of this week and part of next -- so I won't be able to work on this bug for a while.

Would you like to take it, Masayuki? :-)

Comment 9

[Steven Michaud [:smichaud] (Retired)]

I'm baaack.

And after banging my head against this bug for several hours, I've now
found 100% reliable STR (in OS X 10.6.4, with FF 4.0b1 and the
equivalent of today's Minefield nightly):

1) Make sure the Kotoeri input method is enabled (in the Language &
   Text pref panel).

2) Choose Romaji from the input method menu (the "flags menu") in the
   right-hand part of the main menu.

3) Start Firefox 4.0 Beta 1, or a recent Minefield nightly.

4) Right-click on empty space in the navigation toolbar and choose
   "Customize" (from the resulting context menu).

5) Use the mouse scroll button to scroll the "customize" popup window
   up and down.

6) Click on the "Done" button ... and crash.

Now that I can reproduce this, I should be able to find a good fix in
the next day or two.  And hopefully I'll then understand and be able
to respond to Masayuki's comment #7 :-)

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Here are a couple of my crash stacks:

bp-96ee0b66-52e7-4709-9810-690f62100721
bp-11e1b871-0a1f-49ca-8bc0-e5b122100721

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

I've figured out why this bug happens.  It's not that mPresShell gets
hidden.  Rather, the call to mPresShell->FlushPendingNotifications() can
cause it to be destroyed.  This in turn causes
nsCopySupport::GetSelectionForCopy() to return without setting
mSelection, which leads directly to this bug's crash (caused by
dereferencing a null mSelection).

So the fix for this bug is actually quite simple.

And it turns out this is a DOM bug (or possibly a Layout bug), not a
Cocoa widgets bug (or an Apple bug).

Tryserver builds should follow in a few hours.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 459621 [details] [diff] [review]
Fix

Looks ok for me, thanks!

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Here's the Mac tryserver build (the only one with which you can test comment #9's STR):

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-bc42803079f7/tryserver-macosx/firefox-4.0b3pre.en-US.mac.dmg

Other platforms' builds are available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-bc42803079f7/

Aside from two warnings, all the tests passed.  The warnings appear to be spurious.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 459621 [details] [diff] [review]
Fix

Looks reasonable to me. The one unfortunate thing is the addition of a strong reference to the pres shell, which adds reference counting which can be slow. Not sure it matters in this case though. But smaug should know, so I'll let him review this.

Comment 15

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

Comment on attachment 459621 [details] [diff] [review]
Fix

>diff --git a/content/events/src/nsContentEventHandler.cpp b/content/events/src/nsContentEventHandler.cpp
>--- a/content/events/src/nsContentEventHandler.cpp
>+++ b/content/events/src/nsContentEventHandler.cpp
>@@ -81,20 +81,23 @@ nsContentEventHandler::InitCommon()
> {
>   if (mSelection)
>     return NS_OK;
> 
>   NS_ENSURE_TRUE(mPresShell, NS_ERROR_NOT_AVAILABLE);
> 
>   // If text frame which has overflowing selection underline is dirty,
>   // we need to flush the pending reflow here.
>   mPresShell->FlushPendingNotifications(Flush_Layout);
> 
>+  // Flushing notifications can cause mPresShell to be destroyed (bug 577963).
>+  NS_ENSURE_TRUE(!mPresShell->IsDestroying(), NS_ERROR_FAILURE);
>+
>   nsCopySupport::GetSelectionForCopy(mPresShell->GetDocument(),
>                                      getter_AddRefs(mSelection));
>   NS_ASSERTION(mSelection,
>                "GetSelectionForCopy succeeded, but the result is null");

While you're here, could you remove this useless (and actually wrong) assertion.

nsCOMPtr<nsIPresShell> is the right thing to do.
And since presshell isn't cycle collected, nor thread safe, addref/release isn't that slow.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 459621 [details] [diff] [review]
Fix

Landed on trunk with Smaug's change:
http://hg.mozilla.org/mozilla-central/rev/597d4d94e56c

Comment 18

[Steven Michaud [:smichaud] (Retired)]

The fix for this just missed beta3.  It will be in beta4.