303725 - null pointer dereference crash [@ PresShell::RetargetEventToParent]

Status: VERIFIED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Since yesterday (I think because of the checkin for bug 301804), I've been
crashing quite frequently with the assertion followed by null pointer dereference:

###!!! ASSERTION: No docshell for container.: 'docShell', file
/builds/trunk/mozilla/layout/base/nsPresShell.cpp, line 5942
Break: at file /builds/trunk/mozilla/layout/base/nsPresShell.cpp, line 5942

nsDebugImpl::Break(char const*, int)
(/builds/trunk/mozilla/xpcom/base/nsDebugImpl.cpp:326)
nsDebugImpl::Assertion(char const*, char const*, char const*, int)
(/builds/trunk/mozilla/xpcom/base/nsDebugImpl.cpp:261)
nsDebug::Assertion(char const*, char const*, char const*, int)
(/builds/trunk/obj/firefox-debugopt/xpcom/build/nsDebug.cpp:109)
PresShell::RetargetEventToParent(nsIView*, nsGUIEvent*, nsEventStatus*, int,
int&, nsIContent*) (/builds/trunk/mozilla/layout/base/nsPresShell.cpp:5942)
PresShell::HandleEvent(nsIView*, nsGUIEvent*, nsEventStatus*, int, int&)
(/builds/trunk/mozilla/layout/base/nsPresShell.cpp:6107)
nsViewManager::HandleEvent(nsView*, nsGUIEvent*, int)
(/builds/trunk/mozilla/view/src/nsViewManager.cpp:2512)
.L2454 (/builds/trunk/mozilla/view/src/nsViewManager.cpp:2246)
HandleEvent (/builds/trunk/mozilla/view/src/nsView.cpp:171)
nsCommonWidget::DispatchEvent(nsGUIEvent*, nsEventStatus&)
(/builds/trunk/mozilla/widget/src/gtk2/nsCommonWidget.cpp:219)
nsWindow::OnKeyPressEvent(_GtkWidget*, _GdkEventKey*)
(/builds/trunk/mozilla/widget/src/gtk2/nsWindow.cpp:1694)
key_press_event_cb (/builds/trunk/mozilla/widget/src/gtk2/nsWindow.cpp:3835)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
g_closure_invoke+0x000000F6  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit_valist+0x000005B6  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit+0x00000032  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
gtk_window_propagate_key_event+0x000000C5  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgobject-2.0.so.0)
g_closure_invoke+0x000000F6  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit_valist+0x000005B6  (/usr/lib/libgobject-2.0.so.0)
g_signal_emit+0x00000032  (/usr/lib/libgobject-2.0.so.0)
UNKNOWN  (/usr/lib/libgtk-x11-2.0.so.0)
gtk_propagate_event+0x00000217  (/usr/lib/libgtk-x11-2.0.so.0)
gtk_main_do_event+0x000001B4  (/usr/lib/libgtk-x11-2.0.so.0)
UNKNOWN  (/usr/lib/libgdk-x11-2.0.so.0)
g_main_context_dispatch+0x000001CB  (/usr/lib/libglib-2.0.so.0)
UNKNOWN  (/usr/lib/libglib-2.0.so.0)
g_main_loop_run+0x000001A9  (/usr/lib/libglib-2.0.so.0)
gtk_main+0x000000B2  (/usr/lib/libgtk-x11-2.0.so.0)
nsAppShell::Run() (/builds/trunk/mozilla/widget/src/gtk2/nsAppShell.cpp:141)
nsAppStartup::Run()
(/builds/trunk/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:146)
XRE_main (/builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2324)
main (/builds/trunk/mozilla/browser/app/nsBrowserApp.cpp:62)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Brian Ryner (not reading)]

This would indicate that the event is being handled by a pres shell that's
cached in history (the only time a prescontext is unhooked from its container).
 This should never be able to happen.  But, it's possible that from the time
split-window landed up until the fix for bug 298077 landed, that we were leaving
focus in the old document, which might explain this.

I'm not seeing this in talkback crash reports either, so David, can you confirm
whether you're still seeing this after the 298077 checkin?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm pretty sure it was after I pulled yesterday's bug 298077 checkin.  It
wouldn't show up in talkback summaries until tomorrow.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is showing up now in
http://talkback-public.mozilla.org/reports/firefox/FFTrunk/FFTrunk-topcrashers.html

Comment 5

[Mark Mentovai]

topcrasher, this ought to block

Comment 6

[Chris Beard]

bryner, looks like it is bfcache related?

Comment 7

[Mark Mentovai]

Looks like bug 304003 has helped out a lot here.  This may be fixed now, watch
for talkback reports in builds > 080914.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I saw this again just now in a tree pulled at 22:30 PDT last night.

Comment 9

[Zach Lipton [:zach]]

According to talkback, this is still crashing. Clearly 304003 did not fix this. 

Comment 10

[Brian Ryner (not reading)]

Attachment: patch

This patch makes us be more aggressive about locating a container when using
bfcache.  While we keep the container disconnected for all other purposes, we
keep it around as a separate nsWeakPtr for the purpose of targetting events at
the parent PresShell.  As part of this, I made it bail if it could not find
anywhere to send the events.

Unfortunately, I've only managed to trigger the condition that causes the crash
once, and so I can't verify that this patch fixes the problem.

Comment 11

[Aaron Leventhal]

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

One question -- what can a container be other than an nsIDocShell? Why are we
always handing around nsISupports for containers? I only ever see them turned
into docshells.

Comment 12

[Brian Ryner (not reading)]

Yeah, they're only ever docshells... someone over-modularizing I guess.  
Should clean that up for Gecko 1.9.

Comment 13

[Chris Beard]

bryner, it looks like dbaron is on vacation until the 24th.  we might want to
see if we can find one or two other people to superreview in his stead.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

ugly, but sr=dbaron

Comment 15

[Brian Ryner (not reading)]

checked in on trunk and branch

Comment 16

[Brian Ryner (not reading)]

Attachment: fix assertions and other problems

Perhaps part of the reason I wasn't able to verify that the fix did anything...
you can't do_GetWeakReference on a nsWeakPtr.  I changed it to just pass in the
container as an nsWeakPtr.  Should fix the orange on balsa.

Comment 17

[Brian Ryner (not reading)]

(In reply to comment #16)
> Created an attachment (id=193470) [edit]
> fix assertions and other problems
> 
> Perhaps part of the reason I wasn't able to verify that the fix did anything...
> you can't do_GetWeakReference on a nsWeakPtr.  I changed it to just pass in the
> container as an nsWeakPtr.  Should fix the orange on balsa.

Checked in on the trunk and 1.8 branch.

Comment 18

[Tracy Walker [:tracy]]

verified, this hasn't shown up in talkback reports since 8/21