337036 - Crashes in IM_get_owning_window

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current Firefox trunk GTK2 debug build

STEPS TO REPRODUCE:  Close a dialog (the security dialog that comes up when going from an insecure page to a secure one, an alert, etc).

ACTUAL RESULTS:  About half the time, Mozilla crashes.

EXPECTED RESULTS:  No crashes.

DETAILS:

The stack is (for an alert):

#6  0x00d14d4d in g_type_check_instance_is_a () from /usr/lib/libgobject-2.0.so.0
#7  0x00cfae0e in g_object_get_data () from /usr/lib/libgobject-2.0.so.0
#8  0xb6736149 in get_window_for_gtk_widget (widget=0x8b68c78)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:3833
#9  0xb67393c1 in IM_get_owning_window (aArea=0x8bdc638)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:5402
#10 0xb67386e5 in nsWindow::IMEGetOwningWindow (this=0x8e79c60)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:4975
#11 0xb6738686 in nsWindow::IMEComposingWindow (this=0x8e79c60)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:4966
#12 0xb6737f62 in nsWindow::IMEDestroyContext (this=0x8e79c60)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:4782
#13 0xb672e1d2 in nsWindow::Destroy (this=0x8e79c60)
    at ../../../../mozilla/widget/src/gtk2/nsWindow.cpp:429
#14 0xb56fcb81 in ~nsView (this=0x8e79bf0) at ../../../mozilla/view/src/nsView.cpp:264
#15 0xb56fcd76 in nsIView::Destroy (this=0x8e79bf0)
    at ../../../mozilla/view/src/nsView.cpp:302
#16 0xb5702140 in ~nsViewManager (this=0x8e79b70)
    at ../../../mozilla/view/src/nsViewManager.cpp:228
#17 0xb570267f in nsViewManager::Release (this=0x8e79b70)
    at ../../../mozilla/view/src/nsViewManager.cpp:293
#18 0xb5256e1c in nsCOMPtr<nsIViewManager>::~nsCOMPtr ()
    at ../../../../dist/include/dom/nsIDOMDocument.h:27
#19 0xb5248427 in ~DocumentViewerImpl (this=0x8e84fb0)
    at ../../../mozilla/layout/base/nsDocumentViewer.cpp:555
#20 0xb52479cf in DocumentViewerImpl::Release (this=0x8e84fb0)
    at ../../../mozilla/layout/base/nsDocumentViewer.cpp:526
#21 0xb663d198 in ~nsCOMPtr (this=0xbfff8774)
    at ../../../../dist/include/xpcom/nsCOMPtr.h:583
#22 0xb6636add in nsWindowWatcher::OpenWindowJSInternal (this=0x81897f0, 
    aParent=0x88cc1d0, aUrl=0xb66851e0 "chrome://global/content/commonDialog.xul", 
    aName=0xb6685003 "_blank", 
    aFeatures=0xb6685080 "centerscreen,chrome,modal,titlebar", aDialog=1, argc=1, 
    argv=0x9163850, aCalledFromJS=0, _retval=0xbfff8b24)
    at ../../../../../mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:932
#23 0xb6634798 in nsWindowWatcher::OpenWindow (this=0x81897f0, aParent=0x88cc1d0, 
    aUrl=0xb66851e0 "chrome://global/content/commonDialog.xul", 
    aName=0xb6685003 "_blank", 
    aFeatures=0xb6685080 "centerscreen,chrome,modal,titlebar", aArguments=0x8ba9648, 
    _retval=0xbfff8b24)
    at ../../../../../mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:480
#24 0xb664961d in nsPromptService::DoDialog (this=0x8a65d78, aParent=0x88cc1d0, 
    aParamBlock=0x8ba9648, 
    aChromeURL=0xb66851e0 "chrome://global/content/commonDialog.xul")
    at ../../../../../mozilla/embedding/components/windowwatcher/src/nsPromptService.cpp:657
#25 0xb6646e14 in nsPromptService::Alert (this=0x8a65d78, parent=0x88cc1d0, 
    dialogTitle=0xbfff8d6c, text=0x87b63e8)
    at ../../../../../mozilla/embedding/components/windowwatcher/src/nsPromptService.cpp:132
#26 0xb6630079 in nsPrompt::Alert (this=0x87b6400, dialogTitle=0xbfff8d6c, 
    text=0x87b63e8)
    at ../../../../../mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp:217

I have no idea what we're passing to GTK here, but it's clearly not what it expects...

Comment 1

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

I think that this bug is same as bug 335028.
But I and Katakai-san cannot reproduce this crash, why?

Comment 2

[Boris Zbarsky [:bzbarsky]]

No idea; I don't really know this code.  I also can't reproduce it in Seamonkey (also GTK2).  Just in Firefox.

Comment 3

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

O.K. I don't know the reason of this crash.
But maybe, the crash is occured by accessing to native API of GTK while destroying. We may not use native API in this time.

I have an idea for better IM code, I'll create the patch tomorrow or next day.

Comment 4

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

Attachment: Patch rv1.0

Boris:

Can you test this patch?

Comment 5

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

Attachment: Patch rv1.1

This is better.

Comment 6

[Boris Zbarsky [:bzbarsky]]

No crash with that patch applied.  :)

Comment 7

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

thank you, Boris.

Comment 8

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

Comment on attachment 221298 [details] [diff] [review]
Patch rv1.1

Roc:

Would you fix up the my poor comments in nsWindow.h?

Comment 9

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

In IMEDestroyContext you need to comment that deleting mIMEData will not leave any dangling pointers because all the child widgets of this widget must have been destroyed. In fact you should probably asssert that this widget has no children.

In what situations does IMEGetOwningWindow() return something other than "this"?

Why is this approach better than just finding mIMEData every time you need it?

Comment 10

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

(In reply to comment #9)
> In what situations does IMEGetOwningWindow() return something other than
> "this"?

I don't know about this code. But in almost cases, 'this' and the result of IMEGetOwningWindow() is different. I think that IMEGetOwningWindow() returns the top level nsWindow of the 'this'.

> Why is this approach better than just finding mIMEData every time you need it?

The Boris's stack trace shows that it's crashed in |g_object_get_data| that is called on |get_window_for_gtk_widget|. Probably, on some environments, we should not call |get_window_for_gtk_widget| on the widget destroying.

This is only assumption, but I cannot think other causes.
# And this code is readable :-)

Comment 11

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

Attachment: Patch rv1.2

adding the comment on IMEDestroyContext().

Comment 12

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

Comment on attachment 221493 [details] [diff] [review]
Patch rv1.2

This seems OK.

+     *  So, this is created and destroyed only on owner widget.

You should also say here what an "owner widget" is. I think

"Only stand-alone windows and child windows embedded in non-Mozilla GTK containers own IME contexts."

Comment 13

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

Attachment: Patch rv1.3

Comment 14

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

This patch removes bug 335028 from trunk.

Comment 15

[Masaki Katakai]

Comment on attachment 221583 [details] [diff] [review]
Patch rv1.3

Sorry for late. It looks OK for me.

Comment 16

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

checked-in.

But I have a worry. The stack trace of bug 335028 may mean the child widget being destroyed after owner widget destroyed. If so, this patch may make new crash bug on the environments they can reproduce bug 335028. If there is the new crash bug, we need to use nsCOMPtr for mIMEData.

thanks.

Comment 17

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

> we need to use nsCOMPtr for mIMEData.

Or, mIMEData should have simple ref counter.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is this something we could land on the 1.8.0 branch? If so, please nominate the patch for approval-branch-1.8.0.5 and the bug for blocking1.8.0.5

Comment 19

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

(In reply to comment #18)
> Is this something we could land on the 1.8.0 branch? If so, please nominate the
> patch for approval-branch-1.8.0.5 and the bug for blocking1.8.0.5

This occurred only on trunk. I think that your comment is for bug 335028. Bug 335028 needs testers who can reproduce it. Can you reproduce it?

Comment 20

[Alfred Peng]

Attachment: Core stack for Firefox2.0

I met the similar core dump on my Solaris. The firefox information is: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1) Gecko/20061120 Firefox/2.0.

Is this related to this bug? Or a new one?

Comment 21

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

(In reply to comment #20)
> Created an attachment (id=247062) [edit]
> Core stack for Firefox2.0
> 
> I met the similar core dump on my Solaris. The firefox information is:
> Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1) Gecko/20061120 Firefox/2.0.
> 
> Is this related to this bug? Or a new one?

No, This bug is only for trunk. There is a similar bug that occurs on 1.8.x and before. See bug 335028.

Comment 22

[Alfred Peng]

I can reproduce the crash on Firefox2.0 on a Solaris x86 system. And the bug can't be reproduced on recently Firefox trunk build on the same system: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a1) Gecko/20061127 Minefield/3.0a1.
Is it possible that the patch for this bug also fix the problem on bug 335028?

Comment 23

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

(In reply to comment #22)
> I can reproduce the crash on Firefox2.0 on a Solaris x86 system. And the bug
> can't be reproduced on recently Firefox trunk build on the same system:
> Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9a1) Gecko/20061127
> Minefield/3.0a1.
> Is it possible that the patch for this bug also fix the problem on bug 335028?
> 

No, the code has been changed by IM code refactoring.