299419 - Crash [@ nsEventStateManager::FireContextClick]

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Wevah (Nate Weaver)]

I've been getting occasional (but frequent enough to cause worry) crashes with
nsEventStateManager::FireContextClick() on the top of the stack. Digging through
LXR leads me to believe that this is fired for click-hold context menus.
However, these aren't enabled in Camino, so I don't really understand why this
code is even called.

Comment 1

[Wevah (Nate Weaver)]

Attachment: Crash log.

Comment 2

[Jon C. Thomason]

Context menus!  I needed that clue.  I may have found this recurring pest...

In a textarea, double-click on a word, then try to drag-extend the selection.

TB7199000W, TB7200472M, and TB7200493E.  And others, back at least to mid-May.

Comment 3

[Wevah (Nate Weaver)]

Wow; yeah, I can reproduce this every time by following your steps. (Double
click a word in a textarea, hold for a bit, then drag.)

Comment 4

[Jon C. Thomason]

I should clarify: the drag isn't required to reproduce the problem.
It just explains why the problem has been recurring for me so often.

Double-click-and-hold in an HTML form control -> crash

Click-and-hold after selecting separately, or on static content -> no crash

Comment 5

[Jon C. Thomason]

I believe this is a regression from the checkin in bug 255378.
Problem occurs in nightly build 2005-04-20-08, not in 2005-04-19-08.

However, I believe the proper fix is actually to change the condition
in nsEventStateManager.h to disable CLICK_HOLD_CONTEXT_MENUS for Camino
(if not maybe even for Mac OS X entirely?)

Comment 6

[Boris Zbarsky [:bzbarsky]]

This should probably block Gecko 1.8b4, but I can't set that flag while this is
in the camino product...

Comment 7

[Asaf Romano (gone)]

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

Comment 8

[Simon Fraser [no longer active]]

This is a regression from roc's checkin, so over to him.

Comment 9

[Simon Fraser [no longer active]]

The crash happens because the call to frameSel->SetMouseDownState(PR_FALSE); a
few lines above the nsMouseEvent constructor causes a selection changed event to
fire, which re-enters the ESM, clearing mCurrentTarget.

I think the appropriate fix is to move the nsMouseEvent ctor higher up, while
mCurrentTarget is still valid (as it used to be before bug 255378 was fixed).

Comment 10

[Simon Fraser [no longer active]]

Attachment: Patch to init the nsMouseEvent before setting the mouse state

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 189370 [details] [diff] [review]
Patch to init the nsMouseEvent before setting the mouse state

Looks good. 

I'm a bit concerned that mCurrentTarget->GetWindow() could be destroyed in
here. Could you add an nsCOMPtr to hold a strong reference to it until we're
done firing the DOM event? Thanks!

Comment 12

[Simon Fraser [no longer active]]

Taking for checkin.

Comment 13

[Simon Fraser [no longer active]]

Checked in, with a death grip on the nsIWidget.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 189370 [details] [diff] [review]
Patch to init the nsMouseEvent before setting the mouse state

You forgot to request approval. I'll retroactively approve it so we don't get
sent to the firing squad!

Comment 15

[Torben]

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

Comment 16

[Asaf Romano (gone)]

FWIW, i managed to crash here again: TB9047933Y.

Comment 17

[Mark Mentovai]

Also TB9320734Y and many others:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=begins&searchfor=nsEventStateManager%3A%3AFireContextClick%28%29&vendor=All&product=Firefox15&platform=MacOSX&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

And it's a Mac topcrasher:

http://talkback-public.mozilla.org/reports/firefox/

Comment 18

[Mark Mentovai]

Attachment: Check for null pres shell in FireContextClick() (branch)

I came up with a good testcase for reproducing this by looking through talkback
report comments.  Hit a new XUL error page by, for example, visiting
http://127.0.0.1:10545/ and click the "Try Again" button in rapid succession. 
Vary your rate of clicking and amount of holding on the "Try Again" button
slightly if you can't reproduce it right away.

These are crashing now because mPresContext->GetPresShell() is null.  This
patch adds a check for a null pres shell.  I'm getting rid of the check for
mGestureDownContent because the function returns early when it's false.

Comment 19

[Mark Mentovai]

Comment on attachment 195992 [details] [diff] [review]
Check for null pres shell in FireContextClick() (branch)

(This patch was prepared against the 1.8 branch.)

Comment 20

[Mark Mentovai]

Attachment: Check for null pres shell in FireContextClick() (trunk)

This is the same patch, adapted to the trunk.  A different patch is required
since bug 303779 landed on the trunk.

Comment 21

[Scott MacGregor]

this looks like a low risk, top crash bug fix that we should approve for the
branch once it has been reviewed and lands on the trunk. 

Comment 22

[Mark Mentovai]

Fixed on the trunk, need this on the branch.

Comment 23

[Mark Mentovai]

...and now it's fixed there too.

Comment 24

[Marcia Knous [:marcia]]

Looks good using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.8b5) Gecko/20050927 Firefox/1.4. I tried to repro the crash using athe
testcase in Comment #18 and did not see any issues. Adding verified keyword.