83157 - embedding widget doesn't handle reparenting well

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Christopher Blizzard (:blizzard)]

The embedding widget doesn't handle reparenting well.  If you try to reparent it
from one notebook to another it will create a new toplevel window.  We need the
hidden window again so we can unrealize and re-realize safely.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: patch updated with tip ( no code changes )

Comment 3

[Christopher Blizzard (:blizzard)]

Can you guys review/super-review this?

Comment 4

[Daniel Erat]

Great, the patch is a huge improvement!  There are still a few things I'm
noticing:

1. I'm frequently, but not always, seeing segfaults when destroying an embed
that's been reparented.

2. When I remove an embed from one notebook and add it to another, it seems to
be placed a bit farther down on the notebook page than it should be.  See
http://prtr-13.ucsc.edu/~erat/temp/mozembed.png for a screenshot of this.  This
happened in 0.8 as well, I believe.

Comment 5

[Maher Awamy]

Chris,

	This works much better with SkipStone now, when reparenting an embed 
out of hte notebook into a toplevel no more top level window for itself.
However, the crash that used to happen when reparenting the only embed in the
notebook is now happening when you reparent an embed that was reparented once
before into the notebook I think sorta like Daniel said.. Here is the backtrace,
its gtk+ related so it must be coming from gtksuperwin internals is my guess.

Steps to reproduce in SkipStone:

1) open a couple of notebook tabs
2) detab one (alt-d)
3) retab the same one (ctrl-r)



Program received signal SIGSEGV, Segmentation fault.
0x4017f466 in gdk_window_get_size (window=0x39, width=0xbffff57c, 
    height=0xbffff580) at gdkwindow.c:1395
1395    gdkwindow.c: No such file or directory.
(gdb) bt
#0  0x4017f466 in gdk_window_get_size (window=0x39, width=0xbffff57c, 
    height=0xbffff580) at gdkwindow.c:1395
#1  0x401227f8 in gtk_widget_clip_rect (widget=0x8164028, window=0x8518a80, 
    rect=0x82ed780, x_offset=0x0, y_offset=0x0) at gtkwidget.c:2019
#2  0x40122b02 in gtk_widget_idle_draw (cb_data=0x0) at gtkwidget.c:2112
#3  0x401a0803 in g_idle_dispatch (source_data=0x40122988, 
    dispatch_time=0xbffff6e4, user_data=0x0) at gmain.c:1367
#4  0x4019f796 in g_main_dispatch (dispatch_time=0xbffff6e4) at gmain.c:656
#5  0x4019fda3 in g_main_iterate (block=1, dispatch=1) at gmain.c:877
#6  0x4019ff4c in g_main_run (loop=0x8303338) at gmain.c:935
#7  0x400bcb59 in gtk_main () at gtkmain.c:524
#8  0x8051c4f in main (argc=1, argv=0xbffff7e4) at main.c:102
(gdb) 

Comment 6

[Christopher Blizzard (:blizzard)]

Attachment: new patch

Comment 7

[Christopher Blizzard (:blizzard)]

OK, this patch fixes the problems reported with retabbing.  There's still a
problem with skipstone if you de-tab the only window that you have open but I
suspect that's a problem with the somewhat frighting way that skipstone
reparents windows or a problem with notebooks.  Please prove me wrong and tell
me it's my code.

Anyway, I was able to drag tabs like nuts in galeon and detab to my heart's
content in skipstone as long as it wasn't the last one. :)

Comment 8

[Christopher Blizzard (:blizzard)]

Attachment: patch that changes the ordering of items removed in destroy

Comment 9

[Christopher Blizzard (:blizzard)]

The latest patch changes the order that we remove items in the Destroy() method.
 The old way would trigger state changes and location changes and you would end
up emitting signals on the already destroyed widget.

Comment 10

[Yanko Kaneti]

EmbedPrivate.cpp:220: non-lvalue in unary `&'
egcs 1.1.2
(yes,  some people still use that, namely all on rh 6.2 :)

Comment 11

[Maher Awamy]

This is much better Chris, the stuff I had to do for detabbing that horrified
you is becuae of moz 0.8 wierdness with detabbing, however even when I do remove
that uneeded stuff anymore, I do get this crash but it sure looks like its Gtk+
related at the core, I can make a workaround I suppose by not allowing detabbing
until you have 2 or more tabs, here is  a backtrace:

1065    gtkcontainer.c: No such file or directory.
(gdb) bt
#0  gtk_container_resize_children (container=0x813b0c0) at gtkcontainer.c:1065
#1  0x4012d839 in gtk_window_move_resize (window=0x813b0c0) at gtkwindow.c:1803
#2  0x4012d191 in gtk_window_check_resize (container=0x813b0c0)
    at gtkwindow.c:1524
#3  0x400be63f in gtk_marshal_NONE__NONE (object=0x813b0c0, 
    func=0x4012d0d4 <gtk_window_check_resize>, func_data=0x0, args=0xbffff33c)
    at gtkmarshal.c:312
#4  0x400ee87d in gtk_signal_real_emit (object=0x813b0c0, signal_id=57, 
    params=0xbffff33c) at gtksignal.c:1492
#5  0x400ec837 in gtk_signal_emit (object=0x813b0c0, signal_id=57)
    at gtksignal.c:552
#6  0x40079e38 in gtk_container_check_resize (container=0x813b0c0)
    at gtkcontainer.c:928
#7  0x40079ad2 in gtk_container_idle_sizer (data=0x0) at gtkcontainer.c:847
#8  0x401a0803 in g_idle_dispatch (source_data=0x40079a6c, 
    dispatch_time=0xbffff704, user_data=0x0) at gmain.c:1367
#9  0x4019f796 in g_main_dispatch (dispatch_time=0xbffff704) at gmain.c:656
#10 0x4019fda3 in g_main_iterate (block=1, dispatch=1) at gmain.c:877
#11 0x4019ff4c in g_main_run (loop=0x824ddc0) at gmain.c:935
#12 0x400bcb59 in gtk_main () at gtkmain.c:524
#13 0x8051bff in main (argc=1, argv=0xbffff804) at main.c:108
(gdb) 

Comment 12

[Christopher Blizzard (:blizzard)]

Yeah, that's the crash I was seeing.  It's a crash on the new window that pops
up which I thought was odd.  It doesn't seem to be related to the widget at all.
 I would suggest trying reparenting with another non-mozilla kind of widget and
see if it's still there.

Comment 13

[Christopher Blizzard (:blizzard)]

Attachment: patch that fixes compliation problem on 6.2

Comment 14

[Christopher Blizzard (:blizzard)]

I need more testing on this if possible.  tor/bryner/pavlov can you guys review
this patch for me?  Thanks!

Comment 15

[Daniel Erat]

The latest patch appears to work perfectly here under Galeon.  Thanks again!

What are the chances of seeing it in 0.9.1? :)

Comment 16

[selmer (gone)]

Isn't danm the guy to look at this?  

Comment 17

[Christopher Blizzard (:blizzard)]

danm can come up to bat if he feels like it, sure.  Come on down, Dan!

Comment 18

[Christopher Blizzard (:blizzard)]

Out to 0.9.2 since I can't get a review to save my life.

Comment 19

[Brian Ryner (not reading)]

r=bryner

Comment 20

[tor]

sr=tor

Comment 21

[Asa Dotzler [:asa]]

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

Comment 22

[Christopher Blizzard (:blizzard)]

Checked in.