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)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: sayrer, Assigned: jaas)
References
Details
Attachments
(2 files, 3 obsolete files)
3.72 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
15.91 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
Right now, we only look in the resource fork.
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 3•18 years ago
|
||
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+
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 6•16 years ago
|
||
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+
Comment 7•16 years ago
|
||
Forgot to mention that I deleted pluginreg.dat and restarted FF before every test of about:plugins.
Comment 8•16 years ago
|
||
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.
Comment 10•16 years ago
|
||
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.
Assignee | ||
Comment 11•16 years ago
|
||
"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.
Assignee | ||
Comment 12•16 years ago
|
||
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
Assignee | ||
Comment 14•16 years ago
|
||
Attachment #333657 -
Attachment is obsolete: true
Attachment #333666 -
Flags: superreview?(roc)
Attachment #333657 -
Flags: superreview?(roc)
Attachment #333666 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 15•16 years ago
|
||
landed on trunk
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•