Bug 44405 – [Qt] Proper workaround for missing Gtk initialization in Adobe's flash plugins...

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dawit A.]

Created attachment 65076 [details]
Initialize Gtk if the loaded plugin is Adobe's flash player...

This bug report is intended to supercede bug# 44324 and bug# 40567 as the proper fix for the Gtk initialization problems that is prevelant in some releases of Adobe's flash player plugins. Specifically, the patch attached ensures that the workaround in applied ONLY to Adobe's plugin and nothing else.

I tested this change with both Adobe's proprietary plugin as well as Gnu's gnash and everything seems to work fine. For those using the nspluginwrapper, the Adobe workaround needs to be implemented inside of the wrapper itself since it is responsible for loading the flash player plugin.

[WebKit Review Bot]

Attachment 65076 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/plugins/qt/PluginPackageQt.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 65076 [details]
Initialize Gtk if the loaded plugin is Adobe's flash player...

>WebCore/ChangeLog:5
> +          Need a short description and bug URL (OOPS!)
ChangeLog needs to be filled out.

>WebCore/plugins/qt/PluginPackageQt.cpp:154
> +      qDebug() << "**** PATH:" << m_path;
Please remove this, or if you really think it should stay in, clean up the message and put it within a preprocessor conditional.

r- for those two things.

You say the wrapper needs to be fixed, how do we make sure that happens?
Is there no other workaround possible for that case?

[Dawit A.]

Created attachment 65077 [details]
Initialize Gtk if the loaded plugin is Adobe's flash player [Update I]...

Removed debuging statement and corrected changelog entry...

[Dawit A.]

(In reply to comment #2)

> You say the wrapper needs to be fixed, how do we make sure that happens?

Not entirely sure the plugin needs to be fixed. For all I know it might already contain this workaround since this is not the first time this particular bug is seen in the history of adobe's flash player plugin releases. Unfortunately, someone else would need to test it because it is not supported by my distro (no ready made packages available). And looking at its website, I can see why. For all intense and purposes it seems to be unmaintained No new releases since Jan 02, 2009. 

I personally tell people to uninstall it when they send crash reports to kwebkitpart's database and sof far almost all of them have reported that doing so have fixed their problems.

> Is there no other workaround possible for that case?

There is... Always initialize gtk (not only gdk) like you guys did before when the plugin being loaded is the nspluginwrapper. It should however be noted that there is no way  to know which flash player is actually going to be used ; so the workaround would be applied regardless.

Personally I think this is not the place to solve the problem for a library the nspluginwrapper loads itself. If the workaround was for the wrapper itself, it would be completely understandable.

[Kenneth Rohde Christiansen]

Comment on attachment 65077 [details]
Initialize Gtk if the loaded plugin is Adobe's flash player [Update I]...

Can you please verify if this is an issue on maemo5? Also shouldn't we use a quirk for flash instead?

[Dawit A.]

Created attachment 65085 [details]
Alternate patch that also attempts to provide the workaround for the nspluginwrapper...

This is an alternate patch to 65077 that provides a blind workaround for nspluginwrapper as well. The nspluginwrapper portion is untested by me and since there is no way to know which flash player the wrapper will load and use, the initialization workaround for the wrapper will load and initialize the gtk toolkit regardless of the need!

NOTE: I am only providing this patch as an alternate based on per prior discussions. I personally believe the proper approach for the wrapper is to implement the workaround there, specially since the wrapper itself is open source...

[Dawit A.]

(In reply to comment #5)
> (From update of attachment 65077 [details])
> Can you please verify if this is an issue on maemo5? 

Unfortunately, I personally cannot. Very little time to do that, but unless Andreas updated qtwebkit 2.0 with his recent fix, the original version of this patch is what made it into qtwebkit 2.0 and hence Qt 4.7 ; so perhaps someone with maemo5 can test that and report ?

> Also shouldn't we use a quirk for flash instead?

Don't follow... What do you mean ?

[Girish Ramakrishnan]

The suggested patches shouldn't break Maemo5.

Some comments:
1. Can't we use gtk_init_check for both the cases and thus combine the code for both cases?
2. We already have a "if (m_path.contains("npwrapper.")) {", can we combine all the hacks in one place?
3. Adam, I think Kenneth is suggesting adding a Quirk enumeration - PluginQuirkInitializeGtk. Maybe a new quirk is not necessary and we can just combine with the existing PluginQuirkRequiresGtkToolKit (I prefer reusing this enum but for some reason it is special cased for Flash 10).

To summarize,
1. in determineQuriks, we add a new quirk or reuse PluginQuirkRequiresGtkToolKit. If we reuse, we move the quirk for all versions of Flash (for Qt only).
2. in load(), we check the above quirk and call gtk_init_check. in addition, if module name contains npwrapper, we override getvalue to staticPluginQuirkRequiresGtkToolKit_NPN_GetValue.

[Dawit A.]

(In reply to comment #8)
> The suggested patches shouldn't break Maemo5.
> 
> Some comments:
> 1. Can't we use gtk_init_check for both the cases and thus combine the code for both cases?

Thought Gtk is already loaded and will be reused at this point, why go through all the stuff QLibrary does to load a library again when the symbol we need is already available ? It is there so why not use it ? However, you are right the code for the workaround could definitely be improved.

The main question I have is whether or not the non-optimal Gtk workaround for nspluginwrapper is acceptable to do here in QtWebKit instead of getting the plugin wrapper itself fixed ?

[Dawit A.]

Created attachment 65160 [details]
Initialize Gtk plugin to prevent some versions of Adobe's flash player plugin from causing crash...

Combines the two workaround patches into one. Still not sure whether or not the workaround for nspluginwrapper belongs here. However, with this version of the patch it is simply a matter or removing a single line to get rid of the nspluginwrapper workaround if it is not needed...

[Andreas Kling]

Comment on attachment 65160 [details]
Initialize Gtk plugin to prevent some versions of Adobe's flash player plugin from causing crash...

This looks quite reasonable to me.

The nspluginwrapper issue will certainly be revisited but we should do our best to avoid crashes in the meantime.

[Ariya Hidayat]

Comment on attachment 65160 [details]
Initialize Gtk plugin to prevent some versions of Adobe's flash player plugin from causing crash...

I think we can have this fix, it's a good compromise. Nice job :)

[Girish Ramakrishnan]

+1 thanks Adam. I will land it with staticPluginRequiresGtkInit renamed to initializeGtk (as agreed with kling)

[Girish Ramakrishnan]

Landed in 66017

[Dawit A.]

This needs to be cherry picked into qtwebkit 2.1 as replacement for the patch that was cherry picked from bug# 44324:

http://gitorious.org/webkit/qtwebkit/commit/a74784a. 

That patch has two issues:

#1.) It does not provide proper support for flash plugin wrappers such as the nspluginwrapper. 

#2.) Its use of a static flag to avoid multiple initializations breaks sites that embed multiple flash content in a single page. If you get improperly rendered flash content in qtwebkit-2.1 this is most likely the culprit.

[Ademar Reis]

Revision r66017 cherry-picked into qtwebkit-2.1 with commit 64a2028 <http://gitorious.org/webkit/qtwebkit/commit/64a2028>