569775 - Out of process plugins can accidentally release XColormaps due to gdk bug.

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Prior to Gtk+ 2.12.10, releasing a GdkColormap will also release the underlying XColormap. 

Fixed in http://git.gnome.org/browse/gtk+/log/gdk/x11/gdkcolor-x11.c?id=GTK_2_12_10

Plugins running in a separate process use separate refcounts and can accidently release Colormap objects required by the main thread.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Suggested fix

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Fixed erroneous unref

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 449172 [details] [diff] [review]
Fixed erroneous unref


>+    if (gtk_check_version(2, 12, 10)) {
>+        /* Current gtk version is older than the fix */

Doesn't that "if" mean that the current gtk version is >= 2.12.10?

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
No, gtk_check_version returns pointer to a string if the library is not compatible with code compiled against 2.12.10.

A "!= NULL" might make things a little clearer.

Comment 5

[Karl Tomlinson (:karlt)]

Tryserver crashtests have:

REFTEST INFO | Loading file:///builds/slave/tryserver-linux-opt-unittest-crashtest/build/reftest/tests/modules/plugin/test/crashtests/540114-1.html
NPP_Destroy
WARNING: gdk_x11_colormap_foreign_new: assertion `GDK_IS_VISUAL (visual)' failed: 'glib warning', file /builds/slave/tryserver-linux/build/toolkit/xre/nsSigHandlers.cpp, line 193

(<unknown>:3026): Gdk-CRITICAL **: gdk_x11_colormap_foreign_new: assertion `GDK_IS_VISUAL (visual)' failed

###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/tryserver-linux-opt-unittest-crashtest/build/reftest/tests/modules/plugin/test/crashtests/540114-1.html | plugin should not crash item 1

Thread 0 (crashed)
 0  libxul.so!mozilla::plugins::PluginInstanceChild::AnswerNPP_SetWindow [PluginInstanceChild.cpp:10e17bfa2d0a : 818 + 0x0]
    eip = 0x016e51ed   esp = 0xbfaf1e20   ebp = 0xbfaf2028   ebx = 0x01b73450
    esi = 0xbfaf2160   edi = 0x0000000d   eax = 0x00000000   ecx = 0x00000000
    edx = 0x00000000   efl = 0x00010282
    Found by: given as instruction pointer in context

I'm guessing that probably means we have another bug that we are passing an invalid visual to the plugin.

Comment 6

[Karl Tomlinson (:karlt)]

There is a check for VisualID == None here:
http://hg.mozilla.org/mozilla-central/annotate/cac6db5caec8/dom/plugins/PluginInstanceChild.cpp#l756
Another check for None would avoid this crash.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: leak all colormaps from the browser

Using g_object_set_data avoids having to access the private ref_count.

Comment 8

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/3c108dceb5bd