564485 - Support WebPluginMIMETypesFilename in plugin loading

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Stuart Morgan]

It looks like Gecko is still relying on BP_GetSupportedMIMETypesProcPtr to get QuickTime MIME types; the new plist-y way of doing this is using the WebPluginMIMETypesFilename key.

Supporting that would presumably mean that BP_GetSupportedMIMETypes could be completely dropped (Chromium doesn't support it, and it doesn't appear that WebKit does either).

Comment 1

[Josh Aas]

We need to support this in Firefox 4 in order to load Quicktime in a 64-bit browser. Otherwise we can't find MIME types and we skip the plugin altogether.

Comment 2

[Stuart Morgan]

Chromium's implementation and a bit of explanation is here:
http://src.chromium.org/svn/trunk/src/webkit/glue/plugins/plugin_lib_mac.mm
which may be handy since AFAIK there's no real documentation of this anywhere.

Comment 3

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch

Comment 4

[Josh Aas]

Comment on attachment 481501 [details] [diff] [review]
WebPluginMIMETypesFilename patch

I don't think we need to bring obj-c/cocoa into this. Let's use "CFPropertyList".

Comment 5

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.1)

Uses CF instead of Obj-C/Cocoa

Comment 6

[Josh Aas]

Comment on attachment 481865 [details] [diff] [review]
WebPluginMIMETypesFilename patch (v1.1)

>+    CFURLRef mimeTypeFileURL = ::CFURLCreateWithString(kCFAllocatorDefault, mimeTypeFilePath, NULL);

This needs to be null checked in case it wasn't created properly.

>+    if (::CFURLCreateDataAndPropertiesFromResource(kCFAllocatorDefault, mimeTypeFileURL, &resourceData, NULL, NULL, &errorCode) && !errorCode) {

I dislike boolean checks on numerical returns ("!errorCode") except for pointers, it incorrectly implies that the return value is boolean. Please check for 0.

>+                mimeDict = ::CFDictionaryCreateCopy(kCFAllocatorDefault, static_cast<CFDictionaryRef>(mimeTypes));

Why create a copy of this dictionary? Can you just retain it?

>+  if (mimeDict == NULL) {

Please make this "!mimeDict".

Comment 7

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.2)

Comment 8

[Josh Aas]

Scott - I think you mentioned before that you were seeing a crash in tbox opt builds with a previous patch. Did you figure that out?

Comment 9

[Scott Greenlay [:sgreenlay]]

It was a problem in the tree at the time of the run. I did a clean run today (30e51c7019d5).

Comment 10

[Josh Aas]

Comment on attachment 481969 [details] [diff] [review]
WebPluginMIMETypesFilename patch (v1.2)

> static void ParsePlistPluginInfo(nsPluginInfo& info, CFBundleRef bundle)
> {
>-  CFTypeRef mimeDict = ::CFBundleGetValueForInfoDictionaryKey(bundle, CFSTR("WebPluginMIMETypes"));
>+  CFTypeRef mimeDict = NULL;
>+  CFTypeRef mimeFileName = ::CFBundleGetValueForInfoDictionaryKey(bundle, CFSTR("WebPluginMIMETypesFilename"));
>+  

Put the blank line before 'mimeFileName' with no blank line after it. That variable is tied to the following "if" block only.

>+      CFStringRef errorString = NULL;
>+      CFPropertyListRef propertyList = ::CFPropertyListCreateFromXMLData(kCFAllocatorDefault, mimeFileData, kCFPropertyListImmutable, &errorString);

Pass 'NULL' for the error string. You're not using it so don't get it.

>+    } while (0);

Why is this a do-while loop? If you're doing this simply to provide a more limited scope you can just use '{}'.

>+  if (!mimeDict) {
>+    CFTypeRef mimeTypes = ::CFBundleGetValueForInfoDictionaryKey(bundle, CFSTR("WebPluginMIMETypes"));
>+    if (mimeTypes) {
>+      mimeDict = ::CFRetain(mimeTypes);
>+    }
>+  }
>+  
>   if (mimeDict && ::CFGetTypeID(mimeDict) == ::CFDictionaryGetTypeID() && ::CFDictionaryGetCount(static_cast<CFDictionaryRef>(mimeDict)) > 0) {
>     int mimeDictKeyCount = ::CFDictionaryGetCount(static_cast<CFDictionaryRef>(mimeDict));
> 
>     // Allocate memory for mime data
>     int mimeDataArraySize = mimeDictKeyCount * sizeof(char*);
>     info.fMimeTypeArray = static_cast<char**>(NS_Alloc(mimeDataArraySize));
>     if (!info.fMimeTypeArray)
>       return;

Now that you're retaining 'mimeDict' you're leaking it every time you do an early return here. I suggest bringing back the stack-based CFType release object. You can also use it for other things you have to release more than once, like 'currentLocale'.

Comment 11

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.3)

Comment 12

[Josh Aas]

Comment on attachment 482705 [details] [diff] [review]
WebPluginMIMETypesFilename patch (v1.3)

>+  AutoCFTypeObject mimeDictAutorelease(mimeDict);

'mimeDict' can be 'NULL' here and in that case '::CFRelease' will be called on 'NULL'. The docs for '::CFRelease' specifically say "This value must not be NULL".

Also, after further thought I'm not comfortable with the "do-while-false" scheme. Please write that code in a more recognizable way.

Comment 13

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.4)

Comment 14

[Stuart Morgan]

(In reply to comment #12)
> Also, after further thought I'm not comfortable with the "do-while-false"
> scheme. Please write that code in a more recognizable way.

Can I suggest splitting that part out into a helper function that takes a path and returns either a dict or NULL? That way you can use early returns (instead of ever-increasing nesting you were trying to avoid).

Comment 15

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.5)

Comment 16

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.51)

Small syntax change

Comment 17

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.6)

Added home directory evaluation (since CFURL doesn't evaluate '~')

Comment 18

[Josh Aas]

Comment on attachment 485211 [details] [diff] [review]
WebPluginMIMETypesFilename patch (v1.6)

>+static CFTypeRef ParsePlistForMIMETypesFilename(CFBundleRef bundle)

Make this return a "CFDictionaryRef" since you're checking the type before returning.

>+  if (FSFindFolder(kUserDomain, kCurrentUserFolderType, kDontCreateFolder, &homeDir) != noErr) {

"FSFindFolder" -> "::FSFindFolder"

>+  CFURLRef userDirURL = CFURLCreateFromFSRef(kCFAllocatorDefault, &homeDir);

"CFURLCreateFromFSRef" -> "::CFURLCreateFromFSRef"

>+  CFTypeRef mimeDict = ParsePlistForMIMETypesFilename(bundle);

Make "mimeDict" type "CFDictionaryRef".

Comment 19

[Scott Greenlay [:sgreenlay]]

Attachment: WebPluginMIMETypesFilename patch (v1.61)

Comment 20

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/2bb53d0e9a64