Bug 20779

Summary: segmentation fault in swfdec mozilla plugin
Product: WebKit Reporter: Riccardo Magliocchetti <riccardo.magliocchetti>
Component: Plug-insAssignee: Marco Barisione <marco.barisione>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, christian, compnerd, hausmann, marco.barisione, marc.ordinasillopis, otte, stempubuntu
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
A backtrace from a segfault caused by swfdec 0.8.0 and webkit.
none
Stop segfault on leaving page with plugin load
none
Stop segfault on leaving page with plugin load - Qt
none
Stop segfault on leaving page with plugin load -Gtk w/ changelog
none
Patch for GTK with better changelog. none

Description Riccardo Magliocchetti 2008-09-11 05:20:05 PDT
Since swfdec-mozilla 0.8.0 we have a segmentation fault with webkit based browsers because we dereference a null pointer. Please note that swfdec is linux only for now.

I'm seeing this with midori 0.19 / 0.21-git and webkit 1.0.1 / r36309 so it's not a regression.

You can reproduce this by installing swfdec and swfdec-mozilla packages (see http://swfdec.freedesktop.org for instructions) and pointing your browser to any site that have a flash object.

This is the swfdec bug containing the patch that introduced the segmentation fault:
https://bugs.freedesktop.org/show_bug.cgi?id=16717

The bug tracking this issue in swfdec is here:
https://bugs.freedesktop.org/show_bug.cgi?id=17521

This is the stack trace:

[Switching to Thread 0xb57cd720 (LWP 7846)]
0xb4bdf742 in plugin_set_window (instance=0xb4c06820, window=0xb4c06828)
    at plugin.c:398
398         plugin_x11_setup_windowed (instance->pdata, (Window)
window->window, 
(gdb) bt 1 full
#0  0xb4bdf742 in plugin_set_window (instance=0xb4c06820, window=0xb4c06828)
    at plugin.c:398
No locals.
(More stack frames follow...)
(gdb) print window
$1 = (NPWindow *) 0xb4c06828
(gdb) print instance->pdata
$2 = (void *) 0x9d86000
(gdb) print window->ws_info
$3 = (void *) 0x0
(gdb) print window->ws_info->visual
Attempt to dereference a generic pointer.
Comment 1 Benjamin Otte 2008-09-11 08:33:04 PDT
Yeah, it seems Webkit doesn't provide a ws_info field as it should according to http://developer.mozilla.org/en/NPWindow
The code to use it was added because using the wrong Visual might cause an X error, see https://bugs.freedesktop.org/show_bug.cgi?id=16717 and https://bugzilla.mozilla.org/show_bug.cgi?id=445250
Comment 2 Jeff Cook 2008-09-20 09:46:36 PDT
I may be experiencing this bug as well -- not entirely sure if it's the same or different, but it looks the same to me. This occurs whenever I leave a page with a Flash element and swfdec, using epiphany-webkit 2.23.91 and webkit r36705. Again, the first time a Flash element is loaded, thing is fine, but when one tries to leave this page, thing become sad and crashes. :(

I've attached the full backtrace I encountered. Hope it helps.

Arch Linux w/ kernel 2.6.26 (x86_64)
Webkit r36705
Gnome 2.22.3 / epiphany-webkit 2.23.91
swfdec 0.8.0

Thanks in advance.
Comment 3 Jeff Cook 2008-09-20 09:49:12 PDT
Created attachment 23602 [details]
A backtrace from a segfault caused by swfdec 0.8.0 and webkit.

A backtrace from a segfault caused by swfdec 0.8.0 and webkit.
Comment 4 marcoil 2008-09-29 00:45:59 PDT
According to http://developer.mozilla.org/en/Gecko_Plugin_API_Reference/Drawing_and_Event_Handling and http://developer.mozilla.org/en/NPP_SetWindow , passing a null window indicates that the window is being destroyed. I seem to have misread that as window *and* ws_info, sorry for that.

It should be easy to fix, all's needed is to change the order of deleting ws_info and calling the plugin in PluginView::stop in both WebCore/plugins/gtk/PluginViewGtk.cpp:276 and WebCore/plugins/qt/PluginViewQt.cpp:234.

I can't patch myself right away, any takers?
Comment 5 Jeff Cook 2008-09-30 03:23:31 PDT
I've included a patch that seems to resolve the issue. Thanks for your help and attention. : )
Comment 6 Jeff Cook 2008-09-30 03:24:26 PDT
Created attachment 23935 [details]
Stop segfault on leaving page with plugin load
Comment 7 Riccardo Magliocchetti 2008-09-30 03:33:04 PDT
I was doing the same thing(In reply to comment #5)
> I've included a patch that seems to resolve the issue. Thanks for your help and
> attention. : )

i was doing the exact same thing :) it still crash when you close the page with the flash file (webkit r37056 and midori 0.2.1). Should file a new bug?
Comment 8 Jeff Cook 2008-09-30 03:33:47 PDT
Created attachment 23936 [details]
Stop segfault on leaving page with plugin load - Qt
Comment 9 Jeff Cook 2008-09-30 03:43:50 PDT
I don't get that crash with epiphany-webkit. Perhaps you're experiencing a Midori bug?
Comment 10 Marco Barisione 2008-09-30 03:49:46 PDT
I was doing the same, and this is why I assigned the bug to me :)

Can you please provide a ChangeLog entry with the patch? (maybe a patch for both gtk and qt is enough)
Did you test the qt patch?
When the patch is ready for review you should set the review field to "?", so a reviewer can review it.

Even fixing this bug I can still see problems (using GtkLauncher) with both swfdec and the adobe flash plugin, if you want you can open a bug report and try to fix that too.
Comment 11 Riccardo Magliocchetti 2008-10-01 01:02:39 PDT
(In reply to comment #9)
> I don't get that crash with epiphany-webkit. Perhaps you're experiencing a
> Midori bug?

Nope, it happens with GtkLauncher too, filed bug 21240.

Comment 12 Jeff Cook 2008-10-02 05:09:16 PDT
Thank you for the help Marco. I've uploaded a new patch with ChangeLog and marked it ready for review.

Thus far, I've been unable to get QtLauncher to function with swfdec, so I haven't been able to test the patch. I hope to figure this out shortly, and will mark that as needing review once I get that funny business sorted.
Comment 13 Jeff Cook 2008-10-02 05:11:13 PDT
Created attachment 24017 [details]
Stop segfault on leaving page with plugin load -Gtk w/ changelog
Comment 14 Marco Barisione 2008-10-02 06:05:05 PDT
Comment on attachment 24017 [details]
Stop segfault on leaving page with plugin load -Gtk w/ changelog

Patches that need a review should be marked as "?", not "+".

It's nice to have a link to the bug report in the changelog entry but I don't know if it's a rule or what.

The changelog entry should also say how the bug was fixed, not only describe the bug.
Comment 15 Jeff Cook 2008-10-03 21:55:11 PDT
(In reply to comment #14)
> (From update of attachment 24017 [details] [edit])
> Patches that need a review should be marked as "?", not "+".
> 
> It's nice to have a link to the bug report in the changelog entry but I don't
> know if it's a rule or what.
> 
> The changelog entry should also say how the bug was fixed, not only describe
> the bug.
> 

Thanks Marco. I misclicked on the +, meant ?. Submitting updated patch.
Comment 16 Jeff Cook 2008-10-03 22:01:58 PDT
Created attachment 24080 [details]
Patch for GTK with better changelog.
Comment 17 Alp Toker 2008-10-13 15:48:57 PDT
(In reply to comment #16)
> Created an attachment (id=24080) [edit]
> Patch for GTK with better changelog.
> 

Hi,

in r37535 a fix for a plugin crasher was landed. It adds a quirk. I wonder if your patch achieves the same thing without the need for a quirk. If so, we can back out r37535 and land your patch instead.

Can you check this out? Thanks
Comment 18 Riccardo Magliocchetti 2008-10-14 05:16:02 PDT
(In reply to comment #17)
> Hi,
> 
> in r37535 a fix for a plugin crasher was landed. It adds a quirk. I wonder if
> your patch achieves the same thing without the need for a quirk. If so, we can
> back out r37535 and land your patch instead.
> 
> Can you check this out? Thanks

Hi Alp,

Webkit r37567 + this patch + swfdec -> ok
Webkit r37567 + this patch + flash 10 rc -> ok
Webkit r37567 + this patch + reverted r37535 with a patch generated by trac  + flash 10 rc -> segmentation fault in adobe plugin

Please land this patch :)
Comment 19 Holger Freyther 2008-10-15 15:04:16 PDT
Comment on attachment 24080 [details]
Patch for GTK with better changelog.

Looks sane. I did land this patch.
Comment 20 Holger Freyther 2008-10-15 15:23:30 PDT
Comment on attachment 24080 [details]
Patch for GTK with better changelog.

Clearing review flag as this has been applied.
Comment 21 Simon Hausmann 2008-11-05 07:34:59 PST
Landed Jeff's patch for the Qt port in 38125
Comment 22 Gustavo Noronha (kov) 2009-01-12 15:34:50 PST
Closing, since it is landed; please reopen if needed.