Closed Bug 569775 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: IPC, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: mattwoodrow, Assigned: karlt)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Suggested fix (obsolete) — Splinter Review
Blocks: 565833
Attached patch Fixed erroneous unref (obsolete) — Splinter Review
Attachment #448935 - Attachment is obsolete: true
Attachment #449172 - Flags: review?(karlt)
Attachment #449172 - Flags: review?(karlt) → review+
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?
(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.
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.
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.
No longer blocks: 565833
Blocks: 567065
Assignee: nobody → karlt
OS: Mac OS X → Linux
Using g_object_set_data avoids having to access the private ref_count.
Attachment #449172 - Attachment is obsolete: true
Attachment #455001 - Flags: review?(jones.chris.g)
Attachment #455001 - Flags: review?(jones.chris.g) → review+
http://hg.mozilla.org/mozilla-central/rev/3c108dceb5bd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
You need to log in before you can comment on or make changes to this bug.