Closed Bug 350109 Opened 18 years ago Closed 16 years ago

Need to check Info.plist for plugin metadata

Categories

(Core Graveyard :: Plug-ins, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: sayrer, Assigned: jaas)

References

Details

Attachments

(2 files, 3 obsolete files)

Right now, we only look in the resource fork.
Attached patch cleanup v1.0 (obsolete) — Splinter Review
having this in first will make the actual patch's diff much easier to read
Attachment #237226 - Flags: superreview?
Attachment #237226 - Attachment is obsolete: true
Attachment #237227 - Flags: superreview?
Attachment #237226 - Flags: superreview?
Comment on attachment 237227 [details] [diff] [review]
cleanup v1.0 diff -w

r+sr=jst
Attachment #237227 - Flags: superreview?
Attachment #237227 - Flags: superreview+
Attachment #237227 - Flags: review+
Blocks: 435041
Attached patch fix v1.0 (obsolete) — Splinter Review
Attachment #333427 - Flags: review?(smichaud)
At some point we should turn nsPluginsDirDarwin.cpp into a .mm file, then we can use Cocoa and do exception handling. We can do that after this though.
Comment on attachment 333427 [details] [diff] [review]
fix v1.0

I've looked through the code, and found no coding problems.

I do have a couple of questions about the design -- but they don't
effect the loading/operation of currently available plugins, so they
don't need to be addressed right now.  I'll put	them in later
comments.

I also ran some basic tests.  I confirmed that the test plugin Josh
posted at bug 435041 (attachment 333450 [details], originally from the WebKit)
loads (is visible in about:plugins) with this patch, but doesn't load
on the trunk or 1.9.0 branch.  I also confirmed that several commonly
used plugins (which are loaded and used by current trunk and branch
code) continue to appear in about:plugins -- the Java Embedding
Plugin, QuickTime, Silverlight, RealPlayer, Flip4Mac, and Flash.

Oddly, even with this patch, not all the plugins appear in
about:plugins that appear in Safari's list of plugins -- the "Web Kit
plugins" are all absent (Quartz Composer, and the .webplugin versions
of the Flip4Mac and QuickTime plugins).  I assume these are excluded
elsewhere in the browser from the list of loadable plugins.  And in
any case none of them are used or needed.
Attachment #333427 - Flags: review?(smichaud) → review+
Forgot to mention that I deleted pluginreg.dat and restarted FF before
every test of about:plugins.
Josh's v1.0 patch (attachment 333427 [details] [diff] [review]) looks in Info.plist only for
fields specific to the (currently undocumented) "WebKit plugin API" --
"WebPluginName", "WebPluginDescription" and "WebPluginMIMETypes".

But two of these ("WebPluginName" and "WebPluginDescription") mirror
fields found in all "bundle" Info.plist files ("CFBundleName" and
"CFBundleGetInfoString"), and aren't used by even all of the
"standard" plugins (for example the Flash plugin).

Because not all vendors support the "WebKit plugin API", and because
it's currently undocumented (see
http://webkit.org/projects/plugins/index.html), I think the code
should fall back to using the "CFBundleName" if the "WebPluginName" is
absent, and to using the "CFBundleGetInfoString" if the
"WebPluginDescription" is absent.

This isn't a problem with any plugins that currently work with
Mozilla.org browsers -- Mozilla.org browsers currently look for a
plugin's "name" and "description" only in a plugin's resource fork,
which plugin vendors who wish to support FF know they need to provide.
But it may be a problem going forward.  It all depends on how soon
Apple provides decent documentation for its "WebKit plugin API", and
how universal support for it becomes.

Once Josh's patch has landed (and plugin vendors have something to
play with), I'll bring this up again on the plugin-futures mailing
list (https://mail.mozilla.org/listinfo/plugin-futures).
Attachment #333427 - Flags: superreview?(roc)
+
+  return NPERR_GENERIC_ERROR;

Seems like the only reason we need this is that NPNVpluginDrawingModel fails to return in one case. Why don't you just fix that with a final 'return' to look like the other cases.

ParsePlistPluginInfo and OpenPluginResourceFork should be static.

+    CFTypeRef* keys = (CFTypeRef*)PR_Malloc(mimeDictKeyCount * sizeof(CFTypeRef));
+    CFTypeRef* values = (CFTypeRef*)PR_Malloc(mimeDictKeyCount * sizeof(CFTypeRef));

Use nsAutoArrayPtr. And is it true that CFDictionaryGetKeysAndValues doesn't addref anything so we don't need to worry about releasing?

+        info.fVariantCount -= 1;
+        continue;

This leaves garbage in fMimeTypeArray[i], fExtensionArray[i], fMimeDescriptionArray[i] so we must either leak or crash because there is no way to know which entries have valid pointers so we can clean up. Probably best just to zero out the array buffers after allocation.

A helper function that converts a CFStringRef to a PR_malloc'ed UTF8 char* buffer would simplify things a lot.
Josh's v1.0 patch (attachment 333427 [details] [diff] [review]) returns (from
nsPluginFile::GetPluginInfo()) if it finds (static) mime type information in
Info.plist (via a call to ParsePlistPluginInfo()).  This happens before
nsPluginFile::GetPluginInfo() attempts to find and call an
NP_GetMIMEDescription() or BP_GetSupportedMIMETypes() entry point in the
plugin, which (if present) allows the plugin to specify its mime types
dynamically.

Whatever is returned by NP_GetMIMEDescription() or BP_GetSupportedMIMETypes()
still (as previously) takes precedence over any (static) mime type information
found in a plugin's resource fork.

NP_GetMIMEDescription() seems to be part of the NPAPI "spec" for "UNIX"
systems (e.g. http://gplflash.sourceforge.net/gplflash2_blog/npapi.html), and
BP_GetSupportedMIMETypes() appears to be specific to QuickTime (though it's
difficult to be sure).  But BP_GetSupportedMIMETypes() isn't supported by the
WebKit.  And NP_GetMIMEDescription() is only supported on GTK systems (for
which no WebKit implementation currently exists) -- in other words the WebKit
doesn't classify OS X as a "UNIX" system (in the terms of the NPAPI "spec"),
though Mozilla.org traditionally has done so.

So neither NP_GetMIMEDescription() nor BP_GetSupportedMIMETypes() is likely to
find its way into Apple's "WebKit plugin API" (when they finally get around to
documenting it) as applied to OS X.  But I still think we should give
precedence to dynamic information returned by these calls over static
mime-type information found in a plugin's Info.plist.

We're already doing this for static mime-type information found in a plugin's
resource fork.  And there are times when it's important for a plugin to be
able to specify its mime types dynamically.  For example, I could have used
this in the Java Embedding Plugin, had I known about it in time -- on OS X the
user can change the Java version dynamically (using the Java Preferences
panel), which means that its list of supported mime types should also change
dynamically.

It's true that Josh's patch allows you to work around this by not specifying
any (static) mime-type information in the Info.plist file.  But I think it'd
be preferable to do what we're already doing for static mime-type information
found in a plugin's resource fork -- allow it to be overridden (dynamically)
even if it's been specified.

Once Josh's patch has landed, we need to see what plugin vendors think.  One
more question for the plugin-futures mailing list.
"And is it true that CFDictionaryGetKeysAndValues doesn't addref anything so we don't need to worry about releasing?"

Docs: "If the keys are Core Foundation objects, ownership follows the Get Rule."

Same goes for the values. We do not need to worry about releasing.
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #333427 - Attachment is obsolete: true
Attachment #333657 - Flags: superreview?(roc)
Attachment #333427 - Flags: superreview?(roc)
+  int bufferLength = (::CFStringGetLength(cfString) * sizeof(char)) + 1;

sizeof(char) is 1 by definition

+  char* newBuffer = (char*)NS_Alloc(bufferLength);

These and your other casts should be static_cast

+    nsAutoArrayPtr<CFTypeRef> keys(new CFTypeRef[mimeDictKeyCount]);
+    nsAutoArrayPtr<CFTypeRef> values(new CFTypeRef[mimeDictKeyCount]);

Check for OOM here and above

+        if (description && ::CFGetTypeID(description) == ::CFStringGetTypeID())
+          info.fMimeDescriptionArray[i] = CFStringRefToUTF8Buffer((CFStringRef)description);

Missing {}

There are 3 more places you can use CFStringRefToUTF8Buffer
Attached patch fix v1.2Splinter Review
Attachment #333657 - Attachment is obsolete: true
Attachment #333666 - Flags: superreview?(roc)
Attachment #333657 - Flags: superreview?(roc)
Attachment #333666 - Flags: superreview?(roc) → superreview+
landed on trunk
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.