Bug 50657 – Attribute lost with Flash plugin without mime type set

• Component: Plug-ins
• Status: RESOLVED FIXED

[Nicolas Dufresne]

When a Flash animation is embedded into a page without the mime-type the <embed> tag attributes or <object> parameters are lost. I've track down this issue as only affecting ports that uses the PluginDatabase object, which means all of them EXCEPT Mac, Chromium and EFL.

The problem I see is that MIME-Type are not resolved using the PluginDatabase which result into some fallback that recreates the element when the type is finally found. I currently have a fix that make sure extensions are resolved with the plugin database before going into the fallback. I still need to write a test before sending it.

This issue breaks transparency of Flash animation on certain pages like this one:
http://www.flashdesignerzone.com/help/transparent.htm

[Alexey Proskuryakov]

So it sounds like this should affect Safari on Windows.

[Nicolas Dufresne]

(In reply to comment #1)
> So it sounds like this should affect Safari on Windows.

Right, it should if you don't already have the extension/mime information in your win registy. IIRC the windows Flash plugin installer adds an entry to it, so probably that no windows users has been affected so far.

[Nicolas Dufresne]

Created attachment 76140 [details]
Patch

[Eric Seidel (no email)]

Seems sane to me.

[Adam Roben (:aroben)]

Comment on attachment 76140 [details]
Patch

Presumably we'll need to do something similar for WebKit2?

[Nicolas Dufresne]

(In reply to comment #5)
> (From update of attachment 76140 [details])
> Presumably we'll need to do something similar for WebKit2?

Best way to know is to test it. The patch contains a unit test so you can check what happens on platforms running WebKit2 (I'm not doing WebKit2 dev atm).

[Nicolas Dufresne]

I just realized the patch is missing the expected results, will update soon ...

[Nicolas Dufresne]

Created attachment 76888 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 76888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76888&action=review

> LayoutTests/plugins/no-mime-with-valid-extension-expected.txt:2
> +This test checks that bug 50568 is fixed. It runs logSrc test as found in the test plugin which prints the src attribute to stdout. If the bug is present, the logSrc attrbitute that triggers the test will be lost and nothing will be displayed on screen. Uppon success, this test should display the src attribute.

I believe it's "upon" :)

> LayoutTests/plugins/no-mime-with-valid-extension.html:16
> +and nothing will be displayed on screen. Uppon success, this test should display the src

Here is the original Upon

> LayoutTests/plugins/no-mime-with-valid-extension.html:22
> +    finishTest();

You could just inline the code instead of making it a function call. :)

[Nicolas Dufresne]

Created attachment 78531 [details]
patch

- Rebase against current tree reorganisation
- Inline code in test
- Fixed Uppon -> Upon

[Adam Barth]

Comment on attachment 78531 [details]
patch

Forwarding Eric's R+.

[Andy Estes]

Comment on attachment 78531 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78531&action=review

> Source/WebCore/loader/FrameLoader.cpp:972
> +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM) && !PLATFORM(EFL) // Mac has no PluginDatabase, nor does Chromium or EFL
> +    if (mimeType.isEmpty())
> +        mimeType = PluginDatabase::installedPlugins()->MIMETypeForExtension(extension);
> +#endif

Shouldn't this code be in the relevant platforms' implementations of MIMETypeRegistry so as to not litter cross-platform code with ifdefs? I know you're probably just following a convention already established in this method, but it still seems ugly.

[Nicolas Dufresne]

(In reply to comment #12)
> (From update of attachment 78531 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78531&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:972
> > +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM) && !PLATFORM(EFL) // Mac has no PluginDatabase, nor does Chromium or EFL
> > +    if (mimeType.isEmpty())
> > +        mimeType = PluginDatabase::installedPlugins()->MIMETypeForExtension(extension);
> > +#endif
> 
> Shouldn't this code be in the relevant platforms' implementations of MIMETypeRegistry so as to not litter cross-platform code with ifdefs? I know you're probably just following a convention already established in this method, but it still seems ugly.

The ifdef are only there because MAC, Chromium and ELF port have decided not to use the plugin database (we can only blame those port). Also, doing it in the paltform specific MIME database implementation is complex and forces plug-ins to be loaded at start instead of on-demand. This was my conclusion after trying it in GTK port. It fully convinced me that this approach is the right one, and it fixes bug for many port at once.

[Andy Estes]

(In reply to comment #13)
> The ifdef are only there because MAC, Chromium and ELF port have decided not to use the plugin database (we can only blame those port). Also, doing it in the paltform specific MIME database implementation is complex and forces plug-ins to be loaded at start instead of on-demand. This was my conclusion after trying it in GTK port. It fully convinced me that this approach is the right one, and it fixes bug for many port at once.

I wasn't suggesting making a change that affects when plugins load. But it does seem that it would be more elegant to have only one call to make when mapping extensions to MIME types. There would be a way to provide a cross-platform implementation (e.g. MIMETypeRegistry.cpp) with exceptions for the ports that need it (e.g. MIMETypeRegistryWin.cpp), so it wouldn't be that much more complex.

Also, there are other places that call getMIMETypeForExtension() (createBlobDataForFile() for instance). Is it correct that this function doesn't also consult the plugin database? Could that lead to bugs if a blob is created with an extension for which a MIME type is registered in the plugin database but not the MIMETypeRegistry?

I'm not suggesting this patch isn't correct as-is in that it fixes a bug; I'm just thinking of ways it could perhaps be improved as a follow-on.

[WebKit Commit Bot]

Comment on attachment 78531 [details]
patch

Clearing flags on attachment: 78531

Committed r75567: <http://trac.webkit.org/changeset/75567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.