635434 - Crash after hitting Ctrl+T while status message textbox is open in Gmail Chat [@ nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&) ]

Status: VERIFIED FIXED

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

Description

[Mauricio Collares [:collares]]

On the last few nightlies, quickly typing something in the Gmail Chat status message textbox and dismissing it crashes Firefox after a few tries. I have no consistent way of reproducing this, but trying this a few times generates a SIGSEGV consistently. I've been seeing this for at least a few days.

I seem to be getting a poisoned memory for a frame (the full stacktrace for this is below):

(gdb) f 6
#6  0x00cfdd40 in nsPresShellEventCB::HandleEvent (this=0xbf94d7dc, aVisitor=...) at /home/mauricioc/tracemonkey/layout/base/nsPresShell.cpp:1475
1475	                           &aVisitor.mEventStatus);
(gdb) p frame
$1 = (nsIFrame *) 0xa5f8bc48
(gdb) p *frame
$2 = {<nsQueryFrame> = {_vptr.nsQueryFrame = 0xf0dea7ff}, static kFrameIID = nsQueryFrame::nsIFrame_id, mRect = {x = -253843457, y = -253843457, width = -253843457, height = -253843457}, mContent = 
    0xf0dea7ff, mStyleContext = 0xf0dea7ff, mParent = 0xf0dea7ff, mNextSibling = 0xf0dea7ff, mPrevSibling = 0xf0dea7ff, mState = 17356494731632093183, mOverflow = {mType = 4041123839, mVisualDeltas = {
      mLeft = 255 '\377', mTop = 167 '\247', mRight = 222 '\336', mBottom = 240 '\360'}}}

The full stacktrace for the crash is:

(gdb) bt
#0  0x001d9416 in __kernel_vsyscall ()
#1  0x030591a6 in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#2  0x03058fa0 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:138
#3  0x009c072d in ah_crap_handler (signum=11) at /home/mauricioc/tracemonkey/toolkit/xre/nsSigHandlers.cpp:132
#4  0x009c51b7 in nsProfileLock::FatalSignalHandler (signo=11, info=0xbf94d2ac, context=0xbf94d32c) at nsProfileLock.cpp:226
#5  <signal handler called>
#6  0x00cfdd40 in nsPresShellEventCB::HandleEvent (this=0xbf94d7dc, aVisitor=...) at /home/mauricioc/tracemonkey/layout/base/nsPresShell.cpp:1475
#7  0x0111d1b0 in nsEventTargetChainItem::HandleEventTargetChain (this=0xa2d7a9d0, aVisitor=..., aFlags=6, aCallback=0xbf94d7dc, aMayHaveNewListenerManagers=0, aPusher=0xbf94d700)
    at /home/mauricioc/tracemonkey/content/events/src/nsEventDispatcher.cpp:386
#8  0x0111dc8c in nsEventDispatcher::Dispatch (aTarget=0xa40a8780, aPresContext=0xb1e40800, aEvent=0xbf94db68, aDOMEvent=0x0, aEventStatus=0xbf94d958, aCallback=0xbf94d7dc, aTargets=0x0)
    at /home/mauricioc/tracemonkey/content/events/src/nsEventDispatcher.cpp:628
#9  0x00cf40df in PresShell::HandleEventInternal (this=0xb767d100, aEvent=0xbf94db68, aView=0xb1e3b1c0, aStatus=0xbf94d958) at /home/mauricioc/tracemonkey/layout/base/nsPresShell.cpp:7034
#10 0x00cf37a9 in PresShell::HandleEvent (this=0xb767d100, aView=0xb1e3b1c0, aEvent=0xbf94db68, aDontRetargetEvents=0, aEventStatus=0xbf94d958) at /home/mauricioc/tracemonkey/layout/base/nsPresShell.cpp:6781
#11 0x012f6782 in nsViewManager::HandleEvent (this=0xb1e9ab00, aView=0xb1e3b1c0, aEvent=0xbf94db68) at /home/mauricioc/tracemonkey/view/src/nsViewManager.cpp:1105
#12 0x012f66d4 in nsViewManager::DispatchEvent (this=0xb1e9ab00, aEvent=0xbf94db68, aView=0xb1e3b1c0, aStatus=0xbf94da9c) at /home/mauricioc/tracemonkey/view/src/nsViewManager.cpp:1083
#13 0x012efa44 in HandleEvent (aEvent=0xbf94db68) at /home/mauricioc/tracemonkey/view/src/nsView.cpp:161
#14 0x01d26e2d in nsWindow::DispatchEvent (this=0xb2c715b0, aEvent=0xbf94db68, aStatus=@0xbf94dbbc) at /home/mauricioc/tracemonkey/widget/src/gtk2/nsWindow.cpp:563
#15 0x01d2d696 in nsWindow::OnKeyReleaseEvent (this=0xb2c715b0, aWidget=0xb213bb50, aEvent=0xa2b58b00) at /home/mauricioc/tracemonkey/widget/src/gtk2/nsWindow.cpp:3245
#16 0x01d34403 in key_release_event_cb (widget=0xb213bb50, event=0xa2b58b00) at /home/mauricioc/tracemonkey/widget/src/gtk2/nsWindow.cpp:5906
#17 0x049e7284 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#18 0x0059e412 in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#19 0x005b4595 in ?? () from /usr/lib/libgobject-2.0.so.0
#20 0x005b583b in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#21 0x005b5e62 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#22 0x04b15b96 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#23 0x04b286df in gtk_window_propagate_key_event () from /usr/lib/libgtk-x11-2.0.so.0
#24 0x04b2873c in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#25 0x049e7284 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#26 0x0059ca87 in ?? () from /usr/lib/libgobject-2.0.so.0
#27 0x0059e412 in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0
#28 0x005b41e6 in ?? () from /usr/lib/libgobject-2.0.so.0
#29 0x005b583b in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#30 0x005b5e62 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
#31 0x04b15b96 in ?? () from /usr/lib/libgtk-x11-2.0.so.0
#32 0x049df933 in gtk_propagate_event () from /usr/lib/libgtk-x11-2.0.so.0
#33 0x049e0c17 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#34 0x003fe36a in ?? () from /usr/lib/libgdk-x11-2.0.so.0
#35 0x054a4855 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#36 0x054a8668 in ?? () from /lib/libglib-2.0.so.0
#37 0x054a8848 in g_main_context_iteration () from /lib/libglib-2.0.so.0
#38 0x01d38519 in nsAppShell::ProcessNextNativeEvent (this=0xb4e94240, mayWait=0) at /home/mauricioc/tracemonkey/widget/src/gtk2/nsAppShell.cpp:144
#39 0x01d603f1 in nsBaseAppShell::DoProcessNextNativeEvent (this=0xb4e94240, mayWait=0) at /home/mauricioc/tracemonkey/widget/src/xpwidgets/nsBaseAppShell.cpp:173
#40 0x01d6079b in nsBaseAppShell::OnProcessNextEvent (this=0xb4e94240, thr=0xb76d4880, mayWait=0, recursionDepth=0) at /home/mauricioc/tracemonkey/widget/src/xpwidgets/nsBaseAppShell.cpp:315
#41 0x02112bd3 in nsThread::ProcessNextEvent (this=0xb76d4880, mayWait=0, result=0xbf94e69c) at /home/mauricioc/tracemonkey/xpcom/threads/nsThread.cpp:597
#42 0x0209bcc5 in NS_ProcessNextEvent_P (thread=0xb76d4880, mayWait=0) at nsThreadUtils.cpp:250
#43 0x01ed91b0 in mozilla::ipc::MessagePump::Run (this=0xb76a32b0, aDelegate=0xb76207c0) at /home/mauricioc/tracemonkey/ipc/glue/MessagePump.cpp:110
#44 0x0217b497 in MessageLoop::RunInternal (this=0xb76207c0) at /home/mauricioc/tracemonkey/ipc/chromium/src/base/message_loop.cc:219
#45 0x0217b417 in MessageLoop::RunHandler (this=0xb76207c0) at /home/mauricioc/tracemonkey/ipc/chromium/src/base/message_loop.cc:202
#46 0x0217b3bb in MessageLoop::Run (this=0xb76207c0) at /home/mauricioc/tracemonkey/ipc/chromium/src/base/message_loop.cc:176
#47 0x01d6048a in nsBaseAppShell::Run (this=0xb4e94240) at /home/mauricioc/tracemonkey/widget/src/xpwidgets/nsBaseAppShell.cpp:192
#48 0x01a8fb57 in nsAppStartup::Run (this=0xb4ef4670) at /home/mauricioc/tracemonkey/toolkit/components/startup/src/nsAppStartup.cpp:220
#49 0x009b2a38 in XRE_main (argc=1, argv=0xbf94ee44, aAppData=0xb760e380) at /home/mauricioc/tracemonkey/toolkit/xre/nsAppRunner.cpp:3766
#50 0x080496d7 in main (argc=1, argv=0xbf94ee44) at /home/mauricioc/tracemonkey/browser/app/nsBrowserApp.cpp:158

Comment 1

[Mauricio Collares [:collares]]

A few more things. This showed up right before the crash both times I tried to repro this in a debug build:

WARNING: NS_ENSURE_TRUE(mBoundFrame) failed: file /home/mauricioc/tracemonkey/content/html/content/src/nsTextEditorState.cpp, line 1539

Also, I saved a core dump for the crash if you want me to get some extra crash info on GDB.

Comment 2

[Mats Palmgren (inactive)]

Is this mozilla-central nightlies, or something else?
Can find the first build when the problem started?

Comment 3

[Mauricio Collares [:collares]]

mozilla-central nightlies, yes. I can try to find the first build when it started, sure, but the problem is that it's kind of a pain to reproduce the bug on the spot; I have to try doing random focus changes with the status message textbox before it crashes. I'll try to find consistent repro steps before trying to bisect it.

Comment 4

[Mats Palmgren (inactive)]

Long shot: bug 635295 crash was just fixed, it's JS but it's also GMail...
Try updating and see if it fixes it?

Comment 5

[Mauricio Collares [:collares]]

It's unlikely that it does, because this is new in today's nightly while I've been seeing this bug for at least a few days; it also has a different stack trace. I'll try anyway, though.

Comment 6

[Mauricio Collares [:collares]]

I've managed to crash a build that has the fix for bug 635295. Here's a relatively consistent way to do it:

1) Ensure you have a status message in Gmail Chat.
2) Click on your status message to change it.
3) Select the whole status message (I tried with the mouse, but maybe Ctrl+A works as well).
4) Hit Ctrl+T.

Comment 7

[Mauricio Collares [:collares]]

Here's a sample (and probably useless) crash report for this: https://crash-stats.mozilla.com/report/index/bp-8260fd27-730d-43f0-b723-aae8b2110218 

I'm bisecting it now.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

I got a better stack trace for this on Windows. Easy to reproduce with STR in comment 6.

https://crash-stats.mozilla.com/report/index/bp-4c8137c3-c4b7-4946-9b8e-0f9152110218

Comment 9

[Mauricio Collares [:collares]]

hg bisect points to http://hg.mozilla.org/mozilla-central/rev/059044e44314:

The first bad revision is:
changeset:   61533:059044e44314
user:        Neil Deakin <[e-mail]>
date:        Tue Feb 01 09:46:48 2011 -0500
summary:     Bug 619089, retarget key events if the focus changes during a keydown event from content to chrome, r=olli,a=blocking

Comment 10

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

I doubt http://hg.mozilla.org/mozilla-central/rev/059044e44314 has
caused this, but perhaps made it happen more often or so.

Comment 11

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

Ah, ok, I think I know where the problem is.

Comment 12

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

Er, no.

Comment 13

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

Ok, I know the problem. This is certainly a hardblocker.
Patch coming soon, tomorrow at latest.

Comment 14

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

Attachment: patch

Something like this. posted to tryserver, but if you Neil or Masayuki
have time to look at this, comments welcome.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 513742 [details] [diff] [review]
patch

I don't understand well about this bug. Therefore, following my worry may be impossible thing.

1. If aDontRetargetEvents is TRUE, shouldn't we recall nsIViewObserver::HandleEvent()?  It seems that we can create an infinite loop.

2. If targetDoc and mDocument are not same origin, doesn't your patch create a new security issue?

3. If targetDoc and mDocument are in different top level window, the event will cross the window boundary. It might be too bad for some cases.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Olli: does this need beta coverage? Speculatively marking as a betaN+ hardblocker as per your comment 13, but happy to have this wait until final.

Comment 17

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

(In reply to comment #15)
> Comment on attachment 513742 [details] [diff] [review]
> patch
> 
> I don't understand well about this bug.
This bug is about setting mCurrentEventContent to point to an 
element which isn't in mDocument.

Apparently the patch breaks on tryserver.


> 1. If aDontRetargetEvents is TRUE, shouldn't we recall
> nsIViewObserver::HandleEvent()?  It seems that we can create an infinite loop.
How would the infinite loop happen?

> 2. If targetDoc and mDocument are not same origin, doesn't your patch create a
> new security issue?
What issue.


> 
> 3. If targetDoc and mDocument are in different top level window, the event will
> cross the window boundary. It might be too bad for some cases.
Event should be dispatched to the same element as before. We just set
mCurrentEventContent and mCurrentEventFrame in the right presshell.

And yes, I think betaN+ makes sense. Trying to fix the tryserver failures today.

Comment 18

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

Attachment: v2

This fixes the tryserver problems I could reproduce locally, and is 
anyway safer. 
Uploaded to tryserver.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Masayuki - we'd like this for the last beta, if you can prioritize the review that would be great.

Comment 20

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

Attachment: + a null check

Comment 21

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

Comment on attachment 513872 [details] [diff] [review]
+ a null check

This one seems to pass tests on tryserver.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #17)
> > 1. If aDontRetargetEvents is TRUE, shouldn't we recall
> > nsIViewObserver::HandleEvent()?  It seems that we can create an infinite loop.
> How would the infinite loop happen?

If aDontRetargetEvents is TRUE, current trunk doesn't redirect the event to another document. However, you do it on the patch. I'm *not* sure that this make new infinite loop but I can say it may cause the infinite loop in future. I thought you should have checked aDontRetargetEvents before that.

However, your latest patch doesn't have this issue because it calls HandleEventInternal() instead of HandleEvent().

> > 2. If targetDoc and mDocument are not same origin, doesn't your patch create a
> > new security issue?
> What issue.

For example, user inputs some private text on a web site's editor but another web site might receive the input event by the redirect. And if the latter one is evil site, the inputted text might be sent unexpectedly.

> > 3. If targetDoc and mDocument are in different top level window, the event will
> > cross the window boundary. It might be too bad for some cases.
> Event should be dispatched to the same element as before. We just set
> mCurrentEventContent and mCurrentEventFrame in the right presshell.

I worry about this issue, see the comment of here.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6441


I still don't understand why nsFocusManager::GetFocusedDescendant() returns different document's content.

Comment 23

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

(In reply to comment #22)
> > What issue.
> 
> For example, user inputs some private text on a web site's editor but another
> web site might receive the input event by the redirect. And if the latter one
> is evil site, the inputted text might be sent unexpectedly.

But the patch doesn't change the target of the events.
It changes the presshell in which mCurrentEventContent and mCurrentEventFrame
are set.

> I still don't understand why nsFocusManager::GetFocusedDescendant() returns
> different document's content.
In this case it is not that but the IsChromeDoc check which sets 
mCurrentEventContent = gKeyDownTarget;
Nothing guarantees that gKeyDownTarget->GetOwnerDoc() == mDocument.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to comment #23)
> (In reply to comment #22)
> > > What issue.
> > 
> > For example, user inputs some private text on a web site's editor but another
> > web site might receive the input event by the redirect. And if the latter one
> > is evil site, the inputted text might be sent unexpectedly.
> 
> But the patch doesn't change the target of the events.
> It changes the presshell in which mCurrentEventContent and mCurrentEventFrame
> are set.

Ah, right.

> > I still don't understand why nsFocusManager::GetFocusedDescendant() returns
> > different document's content.
> In this case it is not that but the IsChromeDoc check which sets 
> mCurrentEventContent = gKeyDownTarget;
> Nothing guarantees that gKeyDownTarget->GetOwnerDoc() == mDocument.

Oh, I see. Then, I think that your patch is fine. Thank you for your explanation.

But it seems that widget should know which "inputContext" should be received key events, IME related events and content command events. And it may be important for correct TSF implementation for future. I'll think about this issue when I'll restart TSF development.

Comment 25

[Neil Deakin]

Comment on attachment 513872 [details] [diff] [review]
+ a null check

Can you add a test?

Comment 26

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

Yeah, I need to figure out how to make an automated test for this...

Comment 27

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

Attachment: + test

Ah, testing this is easy after all :)

Comment 28

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

http://hg.mozilla.org/mozilla-central/rev/a5f70c575b4a

Comment 29

[Mauricio Collares [:collares]]

This works for me on the latest nightly. Thanks!