346954 - feed MIME types trigger plugin reloads

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Robert Sayre]

application/vnd.mozilla.maybe.feed is not registered in the "gecko-content-viewers" category, so viewing a feed triggers unnecessary plugin loading, at least on Mac, where nsWebNavigation tells me:

[loaded plugin /Library/Internet Plug-Ins/iPhotoPhotocast.plugin]

every time I view a feed.

Comment 1

[Robert Sayre]

#0  nsPluginFile::LoadPlugin (this=0xbfffeacc, outLibrary=@0xbfffeac8) at /Users/sayrer/firefox/mozilla/modules/plugin/base/src/nsPluginsDirDarwin.cpp:218
#1  0x179a3713 in nsPluginHostImpl::ScanPluginsDirectory (this=0x17142340, pluginsDir=0x1713e240, compManager=0x310e50, aCreatePluginList=1, aPluginsChanged=0xbfffebf0, checkForUnwantedPlugins=0) at /Users/sayrer/firefox/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp:4946
#2  0x179a3c8f in nsPluginHostImpl::ScanPluginsDirectoryList (this=0x17142340, dirEnum=0x17140440, compManager=0x310e50, aCreatePluginList=1, aPluginsChanged=0xbfffec58, checkForUnwantedPlugins=0) at /Users/sayrer/firefox/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp:5048
#3  0x179a3f63 in nsPluginHostImpl::FindPlugins (this=0x17142340, aCreatePluginList=1, aPluginsChanged=0xbfffecd4) at /Users/sayrer/firefox/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp:5133
#4  0x179a4103 in nsPluginHostImpl::LoadPlugins (this=0x17142340) at /Users/sayrer/firefox/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp:5068
#5  0x179a49af in nsPluginHostImpl::ReloadPlugins (this=0x17142340, reloadPages=0) at /Users/sayrer/firefox/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp:2696
#6  0x0e20fd89 in nsWebNavigationInfo::IsTypeSupported (this=0x3e7560, aType=@0xbfffedfc, aWebNav=0x154e12b4, aIsTypeSupported=0xbfffedf8) at /Users/sayrer/firefox/mozilla/docshell/base/nsWebNavigationInfo.cpp:92
#7  0x0e20b031 in nsDSURIContentListener::CanHandleContent (this=0x154e0630, aContentType=0x15655768 "application/vnd.mozilla.maybe.feed", aIsContentPreferred=0, aDesiredContentType=0xbfffee8c, aCanHandleContent=0xbfffee7c) at /Users/sayrer/firefox/mozilla/docshell/base/nsDSURIContentListener.cpp:205
#8  0x0e211920 in nsDocumentOpenInfo::TryContentListener (this=0x171cf8d0, aListener=0x154e0630, aChannel=0x171cf460) at /Users/sayrer/firefox/mozilla/uriloader/base/nsURILoader.cpp:741
#9  0x0e21239e in nsDocumentOpenInfo::DispatchContent (this=0x171cf8d0, request=0x171cf460, aCtxt=0x0) at /Users/sayrer/firefox/mozilla/uriloader/base/nsURILoader.cpp:488
#10 0x0e21348e in nsDocumentOpenInfo::OnStartRequest (this=0x171cf8d0, request=0x171cf460, aCtxt=0x0) at /Users/sayrer/firefox/mozilla/uriloader/base/nsURILoader.cpp:333
#11 0x0abb1443 in nsHttpChannel::CallOnStartRequest (this=0x171cf430) at /Users/sayrer/firefox/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:715
#12 0x0abb78cc in nsHttpChannel::OnStartRequest (this=0x171cf430, request=0x171d0450, ctxt=0x0) at /Users/sayrer/firefox/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:3984
#13 0x0ab11f07 in nsInputStreamPump::OnStateStart (this=0x171d0450) at /Users/sayrer/firefox/mozilla/netwerk/base/src/nsInputStreamPump.cpp:434
#14 0x0ab12b7f in nsInputStreamPump::OnInputStreamReady (this=0x171d0450, stream=0x171d021c) at /Users/sayrer/firefox/mozilla/netwerk/base/src/nsInputStreamPump.cpp:390
#15 0x2c0c1be0 in nsInputStreamReadyEvent::Run (this=0x171d07d0) at /Users/sayrer/firefox/mozilla/xpcom/io/nsStreamUtils.cpp:111
#16 0x2c06603c in nsThread::ProcessNextEvent (this=0x30e940, mayWait=1, result=0xbffff58c) at /Users/sayrer/firefox/mozilla/xpcom/threads/nsThread.cpp:482
#17 0x2c00a956 in NS_ProcessNextEvent_P (thread=0x30e940, mayWait=1) at nsThreadUtils.cpp:225
#18 0x0c349384 in nsBaseAppShell::Run (this=0x323980) at /Users/sayrer/firefox/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:153
#19 0x0cf9a34b in nsAppStartup::Run (this=0x33a460) at /Users/sayrer/firefox/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:171
#20 0x2d009a34 in XRE_main (argc=1, argv=0xbffff9e0, aAppData=0x2040) at /Users/sayrer/firefox/mozilla/toolkit/xre/nsAppRunner.cpp:2387
#21 0x00001f36 in main (argc=1, argv=0xbffff9e0) at /Users/sayrer/firefox/mozilla/browser/app/nsBrowserApp.cpp:61

Comment 2

[Robert Sayre]

Look like the fix for bug 119621 made it so that random libraries in the plugin directories cause us to bail from registering. This means the cache will *always* appear stale. 

Comment 3

[Robert Sayre]

When we encounter iPhotoPhotocast.plugin, we make it to line 4977, where we fail while checking for fMimeTypeArray. On Linux, I have seen some printing related plugins do the same thing.


4964       // load the plugin's library so we can ask it some questions, but not for Windows
4965 #ifndef XP_WIN
4966       if (pluginFile.LoadPlugin(pluginLibrary) != NS_OK || pluginLibrary == nsnull)
4967         continue;
4968 #endif
4969 
4970       // create a tag describing this plugin.
4971       nsPluginInfo info = { sizeof(info) };
4972       nsresult res = pluginFile.GetPluginInfo(info);
4973       if(NS_FAILED(res))
4974         continue;
4975 
4976       // if we don't have mime type -- don't proceed, this is not a plugin
4977       if(!info.fMimeTypeArray) {
4978         pluginFile.FreePluginInfo(info);
4979         continue;
4980       }

Comment 4

[Robert Sayre]

It seems safe to skip that fMIMETypeArray check and mark the plugin unwanted, since it's null checked everywhere, but nsPluginFile::GetPluginInfo in nsPluginsDirDarwin.cpp fails to read the resource fork for the iPhoto plugin, so we don't get a file path or plugin name. I was able to get the timestamp and a 0 length mime type array written to pluginreg.dat.

Comment 5

[Robert Sayre]

btw, if someone's looking for a JSON serializer use case, here it is.

Comment 6

[Robert Sayre]

(In reply to comment #4)
> nsPluginFile::GetPluginInfo in
> nsPluginsDirDarwin.cpp fails to read the resource fork for the iPhoto plugin,

This makes sense, because it doesn't have one. It does pass our IsPlugin tests, because it seems to have the right strings in Info.plist.

Comment 7

[Robert Sayre]

It also wants to listen for the unregistered, Apple-invented "application/photo". Evil.

	<key>WebPluginDescription</key>
	<string>iPhoto6</string>
	<key>WebPluginMIMETypes</key>
	<dict>
		<key>application/photo</key>
		<dict>
			<key>WebPluginTypeDescription</key>
			<string>iPhoto 600</string>
		</dict>
	</dict>
	<key>WebPluginName</key>
	<string>iPhotoPhotocast</string>

Comment 8

[Robert Sayre]

Attachment: Check for a resource fork in IsPluginFile

It looks like FSpOpenResFile return codes were being checked incorrectly. The docs I found said any negative number is an error, so I changed the checks.

Comment 9

[Josh Aas]

Comment on attachment 235308 [details] [diff] [review]
Check for a resource fork in IsPluginFile

+    // some safari plugins that we can't use don't have resource forks 

WebKit, not Safari

Approach looks good after IRC discussion.

Note for next reviewer - we won't have to do this once we fix bug 350109.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 235308 [details] [diff] [review]
Check for a resource fork in IsPluginFile

sr=jst

Comment 11

[Robert Sayre]

Checking in nsPluginsDirDarwin.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginsDirDarwin.cpp,v  <--  nsPluginsDirDarwin.cpp
new revision: 1.11; previous revision: 1.10
done

Comment 12

[Robert Sayre]

Attachment: Check for the file in the cache before we root around in the bundles and executables.

Comment 13

[Robert Sayre]

Comment on attachment 235502 [details] [diff] [review]
Check for the file in the cache before we root around in the bundles and executables.

This whole situation could be cleaned up quite a bit, but it's such a hairball that big changes aren't branch-safe.

Comment 14

[Mike Schroepfer]

Do we need both patches?

Comment 15

[Robert Sayre]

(In reply to comment #14)
> Do we need both patches?

No. Both would be nice, but they are independent.

Comment 16

[Mike Schroepfer]

Comment on attachment 235308 [details] [diff] [review]
Check for a resource fork in IsPluginFile

a=schrep for drivers

Comment 17

[Robert Sayre]

Comment on attachment 235308 [details] [diff] [review]
Check for a resource fork in IsPluginFile

Checking in modules/plugin/base/src/nsPluginsDirDarwin.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginsDirDarwin.cpp,v  <--  nsPluginsDirDarwin.cpp
new revision: 1.8.2.1; previous revision: 1.8
done

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 235502 [details] [diff] [review]
Check for the file in the cache before we root around in the bundles and executables.

Yes, quite a hairball. r+sr=jst

Comment 19

[Robert Sayre]

Checking in nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <--  nsPluginHostImpl.cpp
new revision: 1.561; previous revision: 1.560
done

Comment 20

[Robert Sayre]

Comment on attachment 235502 [details] [diff] [review]
Check for the file in the cache before we root around in the bundles and executables.

Drivers: risk for this fix is low (it just moves IsPluginFile() calls after the cache check).

Benefits: avoid inspecting executables we already know are plugins (because they're in the cache).

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 235502 [details] [diff] [review]
Check for the file in the cache before we root around in the bundles and executables.

a=beltzner on behalf of 181drivers

Comment 22

[Adam Guthrie]

The second patch was checked in by sayrer on 2006-09-07 00:27. Marking as FIXED.

Comment 23

[Robert Sayre]

they are no longer reloading, but we still scan them. Fixing this looks like it will require Docshell surgery. bug 353074 opened.