Bug 50657 - Attribute lost with Flash plugin without mime type set
Summary: Attribute lost with Flash plugin without mime type set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 52288
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-07 16:57 PST by Nicolas Dufresne
Modified: 2011-01-12 03:26 PST (History)
12 users (show)

See Also:


Attachments
Patch (4.51 KB, patch)
2010-12-09 17:13 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff
Patch (5.28 KB, patch)
2010-12-17 10:23 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff
patch (6.23 KB, patch)
2011-01-11 07:56 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Dufresne 2010-12-07 16:57:16 PST
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
Comment 1 Alexey Proskuryakov 2010-12-08 14:10:41 PST
So it sounds like this should affect Safari on Windows.
Comment 2 Nicolas Dufresne 2010-12-08 14:23:37 PST
(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.
Comment 3 Nicolas Dufresne 2010-12-09 17:13:29 PST
Created attachment 76140 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-12-10 03:04:25 PST
Seems sane to me.
Comment 5 Adam Roben (:aroben) 2010-12-10 07:37:54 PST
Comment on attachment 76140 [details]
Patch

Presumably we'll need to do something similar for WebKit2?
Comment 6 Nicolas Dufresne 2010-12-14 08:06:37 PST
(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).
Comment 7 Nicolas Dufresne 2010-12-17 10:21:03 PST
I just realized the patch is missing the expected results, will update soon ...
Comment 8 Nicolas Dufresne 2010-12-17 10:23:48 PST
Created attachment 76888 [details]
Patch
Comment 9 Eric Seidel (no email) 2010-12-24 09:15:48 PST
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. :)
Comment 10 Nicolas Dufresne 2011-01-11 07:56:22 PST
Created attachment 78531 [details]
patch

- Rebase against current tree reorganisation
- Inline code in test
- Fixed Uppon -> Upon
Comment 11 Adam Barth 2011-01-11 14:55:49 PST
Comment on attachment 78531 [details]
patch

Forwarding Eric's R+.
Comment 12 Andy Estes 2011-01-11 15:07:22 PST
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.
Comment 13 Nicolas Dufresne 2011-01-11 15:17:32 PST
(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.
Comment 14 Andy Estes 2011-01-11 15:47:59 PST
(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.
Comment 15 WebKit Commit Bot 2011-01-11 16:33:38 PST
Comment on attachment 78531 [details]
patch

Clearing flags on attachment: 78531

Committed r75567: <http://trac.webkit.org/changeset/75567>
Comment 16 WebKit Commit Bot 2011-01-11 16:33:44 PST
All reviewed patches have been landed.  Closing bug.