Closed Bug 337036 Opened 18 years ago Closed 18 years ago

Crashes in IM_get_owning_window

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: masayuki)

References

Details

(4 keywords)

Attachments

(2 files, 3 obsolete files)

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...
Flags: blocking1.9a1?
I think that this bug is same as bug 335028.
But I and Katakai-san cannot reproduce this crash, why?
No idea; I don't really know this code.  I also can't reproduce it in Seamonkey (also GTK2).  Just in Firefox.
Depends on: 335028
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.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Keywords: crash, topcrash
Attached patch Patch rv1.0 (obsolete) — Splinter Review
Boris:

Can you test this patch?
Attached patch Patch rv1.1 (obsolete) — Splinter Review
This is better.
Attachment #221279 - Attachment is obsolete: true
No crash with that patch applied.  :)
thank you, Boris.
Comment on attachment 221298 [details] [diff] [review]
Patch rv1.1

Roc:

Would you fix up the my poor comments in nsWindow.h?
Attachment #221298 - Flags: superreview?(roc)
Attachment #221298 - Flags: review?(masaki.katakai)
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?
(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 :-)
Attached patch Patch rv1.2 (obsolete) — Splinter Review
adding the comment on IMEDestroyContext().
Attachment #221298 - Attachment is obsolete: true
Attachment #221493 - Flags: superreview?(roc)
Attachment #221493 - Flags: review?(masaki.katakai)
Attachment #221298 - Flags: superreview?(roc)
Attachment #221298 - Flags: review?(masaki.katakai)
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."
Attachment #221493 - Flags: superreview?(roc) → superreview+
Attached patch Patch rv1.3Splinter Review
Attachment #221493 - Attachment is obsolete: true
Attachment #221583 - Flags: superreview+
Attachment #221583 - Flags: review?(masaki.katakai)
Attachment #221493 - Flags: review?(masaki.katakai)
This patch removes bug 335028 from trunk.
Blocks: 335028
No longer depends on: 335028
Comment on attachment 221583 [details] [diff] [review]
Patch rv1.3

Sorry for late. It looks OK for me.
Attachment #221583 - Flags: review?(masaki.katakai) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: blocking1.9a1?
Resolution: --- → FIXED
> we need to use nsCOMPtr for mIMEData.

Or, mIMEData should have simple ref counter.
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
(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?
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?
(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.
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?
(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.
You need to log in before you can comment on or make changes to this bug.