539897 - OOPP: gdk_window_get_user_data: assertion `GDK_IS_WINDOW (window)' failed and/or crash resizing Flash plugin [@ XChangeProperty]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #538914 +++

STR:
1) Set dom.ipc.plugins.enabled (and restart).
2) Load data:text/html,<embed width="98%" height="98%" type="application/x-shockwave-flash" src="http://www.communitymx.com/content/source/E5141/wmode.swf">
3) resize the window.

Results:

Usually first an assertion

Gdk-CRITICAL **: gdk_window_get_user_data: assertion
`GDK_IS_WINDOW (window)' failed

#0  0x00007fedf6f5d1d6 in IA__gdk_window_get_user_data (window=0x1a34290, 
    data=0x7fedf38b7368) at gdkwindow.c:2116
#1  0x00007fedf77f9f68 in IA__gtk_plug_construct_for_display (plug=0x1a89300, 
    display=0x19c4010, socket_id=44040484) at gtkplug.c:472
#2  0x00007fedf77fa09b in IA__gtk_plug_new_for_display (display=0x19c4010, 
    socket_id=44040484) at gtkplug.c:529
#3  0x00007fede6025ad8 in ?? ()
   from /home/karl/.mozilla/plugins/libflashplayer.so
#4  0x00007fede601c448 in ?? ()
   from /home/karl/.mozilla/plugins/libflashplayer.so
#5  0x00007fede60205b9 in ?? ()
   from /home/karl/.mozilla/plugins/libflashplayer.so
#6  0x00007fedfde2f955 in mozilla::plugins::PluginInstanceChild::AnswerNPP_SetWindow (this=0x1a43000, aWindow=@0x7fedf38b7570, rv=0x7fedf38b763c)
    at /home/karl/moz/dev/dom/plugins/PluginInstanceChild.cpp:474

but sometimes a crash here.

If not crashing here, then there are usually a number of other assertions
before a crash in

#4  XChangeProperty (dpy=0x0, w=0, property=0, type=0, format=32, mode=0, 
    data=0x7f6fdd8649c0 "\001", nelements=2) at ChProp.c:48
#5  0x00007f6fe18691b6 in xembed_set_info (window=<value optimized out>, 
    flags=0) at gtkplug-x11.c:126
#6  0x00007f6fe1734194 in gtk_plug_realize (widget=0x18682e0) at gtkplug.c:637

Comment 1

[Karl Tomlinson (:karlt)]

I filed https://bugzilla.gnome.org/show_bug.cgi?id=607061
which is the main cause of this.
Though it seems there is more to it than that because the foreign window should not still be in the xid table after it is finalized.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Add missing socket_window reference due to GtkPlug bug

We'll have to add a version check when the bug is fixed in GTK.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> Though it seems there is more to it than that because the foreign window should
> not still be in the xid table after it is finalized.

That's probably not an issue.  The xid table holds a ref to the window, so the window shouldn't be finalized while still in the table.  (It only happens because of the bug linked in comment 1.)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 422018 [details] [diff] [review]
Add missing socket_window reference due to GtkPlug bug

Looks OK to me, as far as I understand it.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/d0918e6f611f

Comment 6

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/58145ca17256

Comment 7

[Karl Tomlinson (:karlt)]

Fixed in GTK+-2.19.5: http://git.gnome.org/browse/gtk+/commit/?id=beddf67e562e1670d692cb9ba0a2546713cc80fc

Filed bug 545297 on removing the extra reference with fixed versions of GTK.

Comment 8

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=11593 has been added to Litmus BFT in the Plugins area.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: crashtest

The test also passes on WINNT tinderbox.  Jim, can you have a quick look please and tell me if running this test on WINNT is a crazy idea?
There's not much point running this test on mac, as there are no plugins with widgets there.

BTW, we still need to keep the litmus test for bug 538914 (et al.).

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 434822 [details] [diff] [review]
crashtest

Looks like it, we expect re-inits if the underlying win32 widget gets destroyed. In this test case we'd unsubclass the old widget, destroy a scrollbar child we've created for some other test, then re-subclass the same widget and create a new child.

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/testplugin/nptest.cpp#833

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/testplugin/nptest_windows.cpp#91

Can we add a comment at the top of pluginWidgetInit letting people know there's a test that does this, just to be safe?

Comment 11

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/902d859ce09e
http://hg.mozilla.org/mozilla-central/rev/f0a50c3f10d3

Comment 12

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/c942ce0beb31
http://hg.mozilla.org/projects/firefox-lorentz/rev/3ab8c594880f

This merged into 1.9.2