82697 - crash when typing at end of location bar string

Status: RESOLVED FIXED

(SeaMonkey :: Autocomplete, defect)

Description

[Colin Blake]

I get a crash when I enter a character at the end of current string
in the location bar. If I position the cursor somewhere in the string
and type, all is well. I suspect its crashing trying to do autocompletion
or bring up the new(?) search tip or whatever its called.

This problem started with the M0.9.1 code and still exists in code I
pulled yesterday 5/24). I have not seen the problem with M0.9 or earlier
versions. I've only seen the problem on OpenVMS, so its likely to be
my bug, but I'm looking for clues about that it could be.

Here's the stack trace. The line numbers unfortunately are trash so don't
try to understand them. I've just started a debug build but that won't be
done until tomorrow.

%SYSTEM-F-ACCVIO, access violation, reason mask=00,
virtual address=000000000000001C, PC=000000000053F0E0, PS=0000001B
%TRACE-F-TRACEBACK, symbolic stack dump follows
  image    module    routine             line      rel PC           abs PC
 LIBGDK  GDKWINDOW  gdk_window_ref      31869 0000000000001440 000000000053F0E0
 LIBGDK  GDKEVENTS  gdk_event_copy      20001 0000000000000AB4 00000000005280A4
 LIBGDK  GDKEVENTS  gdk_event_put       19949 0000000000000984 0000000000527F74
 LIBWIDGET_GTK  NSGTKEVENTHANDLER  handle_key_release_event
                                        75747 0000000000001404 0000000000E5CE44
 LIBWIDGET_GTK  NSGTKEVENTHANDLER  handle_gdk_event
                                        75890 000000000000180C 0000000000E5D24C
 LIBGDK  GDKEVENTS  gdk_event_dispatch  31216 0000000000002690 0000000000529C80
 LIBGLIB  GMAIN  g_main_dispatch        19203 0000000000000AD0 000000000047FE20
 LIBGLIB  GMAIN  g_main_iterate         19424 000000000000128C 00000000004805DC
 LIBGLIB  GMAIN  g_main_run             19482 0000000000001470 00000000004807C0
 LIBGTK  GTKMAIN  gtk_main              20665 0000000000000A58 00000000006D0508
 LIBWIDGET_GTK  NSAPPSHELL  Run         75011 0000000000001544 0000000000E4D5E4
 MOZILLA-BIN  NSAPPRUNNER  main1        91917 00000000000085A8 00000000000385A8
 MOZILLA-BIN  NSAPPRUNNER  main         92215 0000000000009238 0000000000039238
 MOZILLA-BIN  NSAPPRUNNER  __MAIN           0 0000000000000070 0000000000030070
 PTHREAD$RTL                                0 000000000004E3DC 00000000001323DC
 PTHREAD$RTL                                0 0000000000032E78 0000000000116E78

Anyone have any ideas where I should start looking?

Colin.

Comment 1

[Colin Blake]

If I turn on file tracing, I can see Mozilla access files such as 
netscapesearch.gif and netscapesearch.src (basically everything in 
searchplugins) before it crashes.

Comment 2

[Joe Hewitt (gone)]

When you type at the end of the url, the global history database is queried for
matching urls.  You should try debugging in nsGlobalHistory.cpp 

Comment 3

[Colin Blake]

In the version I built with debug, the problem happens a lot less frequently.
So maybe this is an uninitialized piece of data, a buffer overflow, or timing
related?

Unfortunately when it does happen in the debugger it completely locks up my
debug session. I can't type a damn thing. I sometimes see this on the console:

X Toolkit Warning:
  Name: src_menu_bar
  Class: XmRowColumn
  XtGrabKeyboard failed

Comment 4

[Colin Blake]

Not sure how relevant it is, but after typing the first character at the end
of the URL string, I get this:

nsWidget::~nsWidget() of toplevel: 13 widgets still exist.

Is this important? A clue?

Comment 5

[Colin Blake]

The problem is in handle_key_release_event, where sometimes we call 
gdk_event_put with an event which doesn't have a window (any.window)
associated with it. This is what's causing the crash because GDK tries
to increment the ref_count on the non-existant window.

In handle_key_release_event when the event is GDK_KEY_PRESS, there is always
a window in event->any.window and all is fine. But when
handle_key_release_event pulls a GDK_PROPERTY_NOTIFY event off the queue there
event->any.window is zero and kaboom.

Comment 6

[Colin Blake]

Funny you should mention nsGlobalHistory.cpp. I just checked my build log and 
I get some compiler warnings from this module. Is anyone else getting these?


        if (mRawPtr) {
.......^
%CXX-W-UNINIT, variable "row.mRawPtr" declared in function
          "nsGlobalHistory::AddNewPageToDatabase" is fetched, not initialized

        if (mRawPtr) {
.......^
%CXX-W-UNINIT, variable "row.mRawPtr" declared in function
          "nsGlobalHistory::RemoveMatchingRows" is fetched, not initialized

        if (mRawPtr) {
.......^
%CXX-W-UNINIT, variable "cursor.mRawPtr" declared in function
          "nsGlobalHistory::CheckHostnameEntries" is fetched, not initialized

        if (mRawPtr) {
.......^
%CXX-W-UNINIT, variable "newFile.mRawPtr" declared in function
          "nsGlobalHistory::OpenNewFile" is fetched, not initialized

        if (mRawPtr) {
.......^
%CXX-W-UNINIT, variable "oldFile.mRawPtr" declared in function
          "nsGlobalHistory::OpenExistingFile" is fetched, not initialized

        if (mRawPtr) {
.......^
%CXX-W-UNINIT, variable "row.mRawPtr" declared in function
          "nsGlobalHistory::RemovePage" is fetched, not initialized

Comment 7

[selmer (gone)]

Is this *only* happening on OpenVMS?

Comment 8

[Colin Blake]

> Is this *only* happening on OpenVMS?

Believe so, yes.

Comment 9

[Colin Blake]

Anyone have any idea what's going on here? Why am I getting an event without
a window? I modified handle_key_release_event to check for a zero window
before calling gdk_event_put, and if it finds zero, then inserts the address
of a dummy window (something with a valid ref_count). Now all appears to be
working (this new autocomplete feature is neat!), but I suspect that I've only
patched over the problem rather than actually fixing it.

Comment 10

[Colin Blake]

Here's more detail about what's happening. I built GDK with
degug enabled, and these are the EVENT events. The CRB
messages are from Mozilla's handle_key_release_event.

To my untrained eye it looks like the GDK events are:
  "enter notify" - me entering the URL bar
  "button *" - me clicking to get focus
  "key *" - me typing an "h"

Then we get a "property notify2" event for "AdndAware".
This is a regular "property notify" event but I changed
the debug print statement to include the window that is
inserted into the event structure. Its the number in
parens and its zero.

Then you see my debug code in handle_key_release_event and
the window field is zero.

Gdk-Message: focus in:          window: 29360166
Gdk-Message: enter notify:              window: 29360167  detail: 1 subwin: 29360168
Gdk-Message: enter notify:              window: 29360168  detail: 1 subwin: 29360181
Gdk-Message: enter notify:              window: 29360182  detail: 0 subwin: 0
Gdk-Message: button press:              window: 29360182  x,y: 465 42  button: 1
Gdk-Message: button release:    window: 29360182  x,y: 465 42  button: 1
Gdk-Message: key press: window: 29360182  key:            h  104
Gdk-Message:            length:    1 string: "h"
Gdk-Message: key release:               window: 29360182         key:          
 h  104
Gdk-Message: destroy notify:    window: 29360698
Gdk-Message: destroy notify:    window: 29360697
Gdk-Message: destroy notify:    window: 29360696
Gdk-Message: destroy notify:    window: 29360695
Gdk-Message: destroy notify:    window: 29360694
Gdk-Message: property notify2:  window: 29360694 (0), atom(380): "XdndAware"
CRB: handle_key_release_event, before put...
CRB: handle_key_release_event, before put, nextEvent=42615568
  nextEvent->any.type=16
  nextEvent->any.window=0
  nextEvent->any.send_event=0
  nextEvent->property.time=80616618
  About to fiddle with window

Is this only happening on OpenVMS because of timing?

Is this really a GDK bug (they shouldn't play with the window's
ref_count if there's no window?

Anyone?

Comment 11

[Colin Blake]

OK, now I understand. When I built GTK+ for OpenVMS I did so with 
G_DISABLE_CHECKS defined. This means that the check for a null window
in gdk_window_ref is a no-op on OpenVMS. I rebuilt GTK+ without 
G_DISABLE_CHECKS, removed my hack from handle_key_release_event, and now I get:

>Gdk-CRITICAL **: file gdkwindow.c: line 721 (gdk_window_ref): assertion `window 
!= NULL' failed.

but it works!!!

This now looks very much like the Gdk-CRITICAL message (about which people
are saying "where is that coming from?") in bug 61439. This different line
number is because I have GTK 1.2.8 and that has 1.2.10.

Same bug?

Comment 12

[Christopher Blizzard (:blizzard)]

Ahh, I wondered where that message was coming from.  Let's get this fixed. 
Colin, if you can come up with a patch I'll be happy to marshall it in.

Comment 13

[Colin Blake]

My X knowledge is very limited. I really have no idea what is causing this Gdk 
error. What is a "XdndAware" PROPERTY_NOTIFY event, and why is there no
window associated with it? Maybe this is normal and expected, and the only
bug is in GDK (this is the only place in GDK where we call gdk_window_(un)ref 
without first checking for a NULL window).

Comment 14

[Christopher Blizzard (:blizzard)]

The property itself is set on the toplevel window to tell other X applications
that the window in question is a valid drop target for drag and drop.  The
property notify event is generated because I listen on toplevel windows for
property changes so that X-remote will work properly.

I've seen the missing window before.  It's when the client processes an event
that it posted to the queue.  I suspect that's what is happening here.

Anyway, if we do get an event without a window element we can't dispatch it to
the right handler anyway so we should just drop it on the floor.

Comment 15

[Colin Blake]

So basically in nsGtkEventHandler.cpp instead of:

    gdk_event_put(nextEvent);
    gdk_event_free(nextEvent);

we need:

    if (nextEvent->any.window == NULL)
        shouldDrop = PR_TRUE;
    else
        gdk_event_put(nextEvent);

    gdk_event_free(nextEvent);

That will cause us to ignore the event if there's no window.

Colin.

Comment 16

[Christopher Blizzard (:blizzard)]

Stealing this bug because the warnings on the console are annoying.

Comment 17

[Christopher Blizzard (:blizzard)]

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

Comment 18

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 19

[Christopher Blizzard (:blizzard)]

colin, does this work for you?

Comment 20

[Colin Blake]

That patch works for me. Thanks for taking this.

Comment 21

[Christopher Blizzard (:blizzard)]

Attachment: better patch

Comment 22

[Christopher Blizzard (:blizzard)]

This patch uses Xlib.  After looking at the gtk source code for a while I think
there's a risk that if we gdk_event_put() the event back that we can actually
put it back in the wrong order.  This simple Xlib routine will let us look in
the queue and as far as gtk is concerned "it's like we were never here."

Comment 23

[Colin Blake]

The Xlib patch works for me, too.

Comment 24

[tor]

[s]r=tor

Comment 25

[Stuart Parmenter]

r=pavlov

Comment 26

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 27

[Christopher Blizzard (:blizzard)]

Checked in.  Thanks, guys!

Comment 28

[Christopher Blizzard (:blizzard)]

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