Bug 48046 – WebKit shouldn't load a plug-in based on file extension if a MIME type is specified.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

WebKit shouldn't load a plug-in when a page author specifies a MIME type that no plug-in supports.

[Andy Estes]

In the following example:

<embed src="movie.m4v" type="application/x-shockwave-flash">

when Flash isn't installed but QuickTime is, WebKit will unexpectedly load the QuickTime plug-in. This is because WebKit recognizes that QuickTime supports files ending in '.m4v' and loads the file in QuickTime despite the markup specifying a MIME type QuickTime doesn't support.

In this case, WebKit should display the missing plug-in indicator if an explicit MIME type is specified that no installed plug-in claims to support.

[Andy Estes]

<rdar://problem/8573762>

[Andy Estes]

Created attachment 71401 [details]
Patch

[Andy Estes]

Giving myself an r-. I need to see what other platforms need this fix in their FrameLoaderClient implementation before posting this for review.

[Andy Estes]

(In reply to comment #4)
> Giving myself an r-. I need to see what other platforms need this fix in their FrameLoaderClient implementation before posting this for review.

Looks okay.

[Brian Weinstein]

Comment on attachment 71401 [details]
Patch

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

> WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1632
> +    if (!pluginPackage && [extension length] != 0 && [MIMEType length] == 0) {

Should we be using [extension length] && ![MIMEType length] and get rid of the comparisons with 0?

[Eric Carlson]

Comment on attachment 71401 [details]
Patch

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

r=me with the suggested changes

>> WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1632
>> +    if (!pluginPackage && [extension length] != 0 && [MIMEType length] == 0) {
> 
> Should we be using [extension length] && ![MIMEType length] and get rid of the comparisons with 0?

Indeed, please follow current WebKit style even though the old code is incorrect.

> LayoutTests/plugins/invalid-mime-with-valid-extension-shows-missing-plugin.html:19
>  

I would guess that the data: url will be delivered to the plug-in much faster than the test file, so I think you have a potential race condition here even with the setTimeout(0). I think you may want to use the test file for both plug-in instances and use a non-zero timeout instead.

[Andy Estes]

Created attachment 71514 [details]
Patch

[Andy Estes]

(In reply to comment #8)
> Created an attachment (id=71514) [details]
> Patch

I rewrote the test. It changed enough that it probably warrants a new review.

[Eric Carlson]

Comment on attachment 71514 [details]
Patch

Much simpler test, nice! 

One very minor nit that you might want to change: you don't need to call finishTest from logFailure, just get rid of the "else" after calling testCallback.

r=me

[Andy Estes]

Thanks Eric. Landed in http://trac.webkit.org/changeset/70332.

[Andy Estes]

Oops, forgot to svn add the tests. Layout tests landed in http://trac.webkit.org/changeset/70334.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70332 might have broken GTK Linux 32-bit Release

[WebKit Review Bot]

http://trac.webkit.org/changeset/70334 might have broken GTK Linux 32-bit Release

[Andy Estes]

(In reply to comment #14)
> http://trac.webkit.org/changeset/70334 might have broken GTK Linux 32-bit Release

Fixing.

[Andy Estes]

I didn't handle this for WebKit2 or Win/Gtk/Qt (oops!). Fixed in http://trac.webkit.org/changeset/70379.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70379 might have broken GTK Linux 32-bit Debug