335028 - Firefox 1.5.0.2 Linux topcrash [@ IM_get_input_context]

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

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

One of the Linux topcrashes in Firefox 1.5.0.2 is a null pointer dereference in IM_get_input_context, which unconditionally dereferences owningWindow, which is null when we crash.

The top of the stack is pretty constant between incidents although the source of the document viewer destruction varies (direct or in fastback cache):

IM_get_input_context()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/widget/src/gtk2/nsWindow.cpp, line 4793]
nsWindow::IMELoseFocus()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/widget/src/gtk2/nsWindow.cpp, line 4387]
nsWindow::IMEDestroyContext()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/widget/src/gtk2/nsWindow.cpp, line 4358]
nsWindow::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/widget/src/gtk2/nsWindow.cpp, line 404]
nsView::~nsView()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/view/src/nsView.cpp, line 267]
nsIView::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/view/src/nsView.cpp, line 304]
nsFrame::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/layout/generic/nsFrame.cpp, line 669]
nsContainerFrame::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 169]
nsFrameManager::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/layout/base/nsFrameManager.cpp, line 298]
PresShell::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/layout/base/nsPresShell.cpp, line 1957]
DocumentViewerImpl::Destroy()  [/builds/tinderbox/Fx-Mozilla1.8.0/Linux_2.4.21-37.ELsmp_Depend/mozilla/layout/base/nsDocumentViewer.cpp, line 712]

Comment 1

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

Attachment: analysis of TB17828923

Comment 2

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

taking. but what is cause of this regression?

Comment 3

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

Attachment: Patch rv1.0

This patch is not needed on Trunk. Because trunk already has this code.

Comment 4

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

I cannot reproduce this bug, but maybe the patch fixes this bug.

Comment 5

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

Attachment: Patch rv1.1 for 1.8.x

Oops. Sorry. I'm wrong.

Comment 6

[Masaki Katakai]

Comment on attachment 219454 [details] [diff] [review]
Patch rv1.1 for 1.8.x

+katakai

I'm also not seeing this problem on my environment, but the patch seems OK.

Comment 7

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

Attachment: Patch rv1.0 for Trunk

Uh, I found similar risk. We should fix this too.

Comment 8

[Masaki Katakai]

Comment on attachment 219531 [details] [diff] [review]
Patch rv1.0 for Trunk

+katakai

Comment 9

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

Comment on attachment 219454 [details] [diff] [review]
Patch rv1.1 for 1.8.x

The code is fine, but can you explain why aArea can be null or owningWindow can be null?

Comment 10

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

Comment on attachment 219454 [details] [diff] [review]
Patch rv1.1 for 1.8.x

Actually, let's wait until we can verify that this fixes the real issue. Do we know anyone who can reproduce the crash here?

Comment 11

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

(In reply to comment #10)
> (From update of attachment 219454 [details] [diff] [review] [edit])
> Actually, let's wait until we can verify that this fixes the real issue. Do we
> know anyone who can reproduce the crash here?
> 

No, I don't know. I don't see this report on bugzilla-jp, Japanese forums and Japanese blogs.

Comment 12

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

Oshima-san:

Can you reproduce this bug?

Comment 13

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

Could it be a teardown ordering issue?  Are the stacks something that commonly happens during window teardown?

Comment 14

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

dbaron: looks like it to me.

I'm a bit afraid that checking in this patch will just make us crash somewhere else, or worse, corrupt memory instead of crashing.

Comment 15

[Daniel Veditz [:dveditz]]

This sounds like the sort of fix that truly needs the trunk-baking time to make sure there are't any bad regressions or side-effects. Too late for 1.8.0.4

Comment 16

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

(In reply to comment #15)
> This sounds like the sort of fix that truly needs the trunk-baking time to make
> sure there are't any bad regressions or side-effects. Too late for 1.8.0.4

We cannot test on Trunk, because the trunk doesn't have this code. We can test only on MOZILLA_1_8_BRANCH.

Comment 17

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

Boris has reported same report in bug 337036.

Boris:
Can you help us? See comment 10, comment 13 and comment 14.

Comment 18

[Boris Zbarsky [:bzbarsky]]

So I see bug 337036 almost half the time when closing windows.  Simple testcase:

  javascript:alert('aaa')

Then click "ok".

Half the time I crash (inside GTK); that's bug 337036 (which has a slightly different stack from this one).  The other half, I get:

(Gecko:12931): GLib-GObject-WARNING **: invalid cast from `(null)' to `GtkWidget'
(Gecko:12931): GLib-GObject-WARNING **: invalid cast from `(null)' to `GObject'
(Gecko:12931): GLib-GObject-CRITICAL **: file gobject.c: line 1642 (g_object_get_data): assertion `G_IS_OBJECT (object)' failed

When I crash, mIMEData is false.  So I wouldn't even hit the codepath the patch in this bug is changing.  Furthermore, in IM_get_owning_window I have a non-null aArea and a non-null aArea->inner_window.  But the call to get_gtk_widget_for_gdk_window crashes.

So as far as I can tell, either bug 337036 is a different bug or this patch is wallpaper that doesn't fix the underlying issue.

Comment 19

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

(In reply to comment #18)
> Half the time I crash (inside GTK); that's bug 337036 (which has a slightly
> different stack from this one).

Ah, right, sorry.

Comment 20

[Daniel Veditz [:dveditz]]

Can we get this bug landed on the 1.8 branch ("fixed1.8.1" state) and get some verification there that this doesn't cause the worse problems roc was worried about?

Comment 21

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

Has anyone yet confirmed that this patch actually fixes anything? Comment #18 suggests it does not. I don't think we should check in a patch that we're not sure actually fixes anything.

Comment 22

[Daniel Veditz [:dveditz]]

Too late to get an unbaked patch into 1.8.0.5 for a non-security issue.

Comment 23

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

Minusing for blocking1.8.1 because it doesn't look like we're going to be able to get a fix for this one, although it would be great if we could still get a patch.

Comment 24

[Adam Guthrie]

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

Comment 25

[Mats Palmgren (inactive)]

Just a long shot: does the patch in bug 351225 fix it?
(speculating that we destroy a view/window that has already been destroyed)

Comment 26

[Alexander Sack]

(In reply to comment #25)
> Just a long shot: does the patch in bug 351225 fix it?
> (speculating that we destroy a view/window that has already been destroyed)
> 

I don't think so; we have received a report whose stacktrace looks quite similar from a user of 2.0.0.1. bug 351225 is verified1.8.1.1 so this is unlikely to be the same problem.

You can take a look here:
https://launchpad.net/ubuntu/+source/firefox/+bug/85627

Comment 27

[Adam Guthrie]

For all intents and purposes, this crash is the #1 Linux topcrash in 2.0.0.1 (only crashes in libc.so.6 and Flash outnumber it). Renominating in hopes that someone has time to take a second look.

Comment 28

[Mike Connor [:mconnor]]

Not going to block 1.8.1.3 for this, this is a very short cycle with no time for baking.  Moving nom over to 1.8.1.4, we probably want to take this early.

Comment 29

[Daniel Veditz [:dveditz]]

We should get this fixed -- the small percentage of our users who use an IM (5%? 10% at most?) have managed to generate the #2 linux top crash.

roc: is there a way to track down your concerns in comment 14?

Comment 30

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

roc:

see comment 29.

Comment 31

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

Comment on attachment 219454 [details] [diff] [review]
Patch rv1.1 for 1.8.x

OK. I'm unconvinced this will fix anything but it obviously won't make things worse, so let's land this on the 1.8.1 branch and see if that affects anything.

What we really need here, though, is someone who can reproduce this crash reliably. If it's such a huge crasher we should be able to find such a person...

Comment 32

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

Comment on attachment 219454 [details] [diff] [review]
Patch rv1.1 for 1.8.x

Thank you roc. Let's land it.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 219454 [details] [diff] [review]
Patch rv1.1 for 1.8.x

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 34

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

checked-in to 1.8 branch.

Comment 35

[Tracy Walker [:tracy]]

Verified with Simple testcase:

  javascript:alert('aaa')

Then click "ok"

on Linux FC5 with 2.0.0.4 rc2

Comment 36

[Alexander Sack]

i never saw this crash with the simple testcase mentioned. Hope this fix does not just split this issue in crashes at multiple places.