578868 - [OOPP] OOPP are also loaded in main process

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benoit Girard (:BenWa)]

As described by Steven Michaud in Bug 574904:

1) Run Minefield (a recent nightly, like today's) on OS X 10.6.4.

2) Load a Java applet
   (e.g. http://java.sun.com/applets/jdk/1.4/demo/applets/Clock/example1.html)
   or a Flash "movie"
   (e.g. http://www.playercore.com/bugfiles/146162/AddReturnHTML.html).

3) Find the pids of the main ("firefox-bin") process and the
   plugin-specific process(es).

4) Attach with gdb to each process:

   gdb [firefox-pid] firefox
   gdb [plugin-pid] plugin

5) In each gdb session run "info sharedlibrary".

Expected result:
The plug-in shared library should only appear under the plug-in's process and not the main process'.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Only load library if plugin is !OOP

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 460754 [details] [diff] [review]
Only load library if plugin is !OOP

The diffs for nsNPAPIPlugin.cpp are really misleading. What's really going on is I changed the namespace of RunPluginOOP to be part of 'nsNPAPIPlugin' so that it could be used in 'nsPluginHost'.

This is an important change if we want to support cross-architecture plug-ins since they can not be loaded in the main process.

Comment 3

[Josh Aas]

Attachment: fix v2.0 (incomplete)

Our library loading code is not pretty, I think we need to do a bit more to improve it. This is a start. Benoit's patch here does fix some cases but we still unnecessarily call LoadPlugin before getting plugin info. PRLibrary doesn't actually bring in the lib until we look up a symbol, so this isn't as bad as it first seems, but it is still ugly.

Comment 4

[Josh Aas]

Attachment: fix v2.1

This fix should work. It unifies our plugin loading and unloading processes (UNIX now does it in exactly the same way as Mac OS X) and only loads plugins when necessary. In addition to not loading for out-of-process plugins, it won't always load for getting plugin info on Mac OS X. It will also attempt to unload any libraries loaded while getting plugin info.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 462331 [details] [diff] [review]
fix v2.1

>-      result = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
>+      rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
>                                       getter_AddRefs(binDirectory));
optional: Indentation nit.

>diff --git a/modules/plugin/base/src/nsPluginsDir.h b/modules/plugin/base/src/nsPluginsDir.h
>--- a/modules/plugin/base/src/nsPluginsDir.h
>+++ b/modules/plugin/base/src/nsPluginsDir.h
>@@ -88,22 +88,23 @@ public:
> 	 */
> 	nsPluginFile(nsIFile* spec);
> 	virtual ~nsPluginFile();
> 
> 	/**
> 	 * Loads the plugin into memory using NSPR's shared-library loading
> 	 * mechanism. Handles platform differences in loading shared libraries.
> 	 */
>-	nsresult LoadPlugin(PRLibrary* &outLibrary);
>+	nsresult LoadPlugin(PRLibrary **outLibrary);
> 
> 	/**
> 	 * Obtains all of the information currently available for this plugin.
>+   * Has a library outparam which will be non-null if a library load was required.
> 	 */
>-	nsresult GetPluginInfo(nsPluginInfo &outPluginInfo);
>+	nsresult GetPluginInfo(nsPluginInfo &outPluginInfo, PRLibrary **outLibrary);

Here we modify the header but there is no changes for nsPluginsDirOS2/nsPluginsDirBeOS. They are still included in the Makefile so I assume we still support them.

>-nsresult nsPluginFile::GetPluginInfo(nsPluginInfo& info)
>+nsresult nsPluginFile::GetPluginInfo(nsPluginInfo& info, PRLibrary **outLibrary)
> {
>   nsresult rv = NS_OK;
> 
>   // clear out the info, except for the first field.
>   memset(&info, 0, sizeof(info));
> 
>-  // First open up resource we can use to get plugin info.
>-
> #ifndef __LP64__
>-  // Try to open a resource fork.
>+  // Try to open a resource fork in case we have to use it.
>   nsAutoCloseResourceObject resourceObject(mPlugin);
>   bool resourceOpened = resourceObject.ResourceOpened();
> #endif
> 
>   // Try to get a bundle reference.
>   nsCAutoString path;
>   if (NS_FAILED(rv = mPlugin->GetNativePath(path)))
>     return rv;
>@@ -423,24 +421,29 @@ nsresult nsPluginFile::GetPluginInfo(nsP
>   // The last thing we need to do is get MIME data
>   // fVariantCount, fMimeTypeArray, fExtensionArray, fMimeDescriptionArray
> 
>   // First look for data in a bundle plist
>   if (bundle) {
>     ParsePlistPluginInfo(info, bundle);
>     ::CFRelease(bundle);
>     if (info.fVariantCount > 0)
>-      return NS_OK;    
>+      return NS_OK;
>   }

I don't like having an out parameter, outLibrary, that we leave unchanged when returning success under some branches relying on the caller to initialize it before calling. I know that it's current usage sets it to nsnull before invoking this method so there is no problem at the moment but I still find this to be dangerous. Is this considered acceptable?

>-nsresult nsPluginFile::GetPluginInfo(nsPluginInfo& info)
>+nsresult nsPluginFile::GetPluginInfo(nsPluginInfo& info, PRLibrary **outLibrary)
> {
>   nsresult rv = NS_OK;
>   DWORD zerome, versionsize;
>   WCHAR* verbuf = nsnull;
> 
>   if (!mPlugin)
>     return NS_ERROR_NULL_POINTER;
> 

Similarly, outLibrary should be set to null if it is never used.

Comment 6

[Josh Aas]

Attachment: fix v2.2

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 463994 [details] [diff] [review]
fix v2.2

-	nsresult LoadPlugin(PRLibrary* &outLibrary);
+	nsresult LoadPlugin(PRLibrary **outLibrary);

This signature change still needs to be reflected in BeOS/OS2, the rest looks good.

Comment 8

[Josh Aas]

Attachment: fix v2.3

Comment 9

[Benjamin Smedberg]

Comment on attachment 464125 [details] [diff] [review]
fix v2.3

So the basic idea of this patch is that we'll still load it in-process if necessary to extract MIME type info (first run). But afterwards, we won't?

Comment 10

[Josh Aas]

Attachment: fix v2.4

Includes Linux compile fixes.

Comment 11

[Josh Aas]

Comment on attachment 464570 [details] [diff] [review]
fix v2.4

Re-requesting review for significant Linux bustage fix and API change.

Comment 12

[Benjamin Smedberg]

Comment on attachment 464570 [details] [diff] [review]
fix v2.4

Can we reuse or move PluginPRLibrary NP_GetValueFunc instead of repeating the whole type for npGetValue? Otherwise, this looks fine.

Comment 13

[Josh Aas]

Attachment: fix v2.5

Comment 14

[Josh Aas]

Attachment: fix v2.6

Comment 15

[Josh Aas]

pushed to mozilla-central and then backed out due to a 64-bit Linux failure I did not get on try server

http://hg.mozilla.org/mozilla-central/rev/452db8c688ba

http://hg.mozilla.org/mozilla-central/rev/60a5b0d62bcc

Comment 16

[Josh Aas]

Attachment: 64-bit Linux tbox failure

Comment 17

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #15)
> pushed to mozilla-central and then backed out due to a 64-bit Linux failure I
> did not get on try server
> 
> http://hg.mozilla.org/mozilla-central/rev/452db8c688ba
> 
> http://hg.mozilla.org/mozilla-central/rev/60a5b0d62bcc

josh: 

This is something we'd like to investigate. Can you please file a bug in mozilla.org:ReleaseEngineering giving specific details of:

- what changesets/pushes on TryServer and mozilla-central you saw the discrepancy?
- I see the attachment above, which I guess is the error you saw on mozilla-central but not on TryServer? If so, was this the only error?

Comment 18

[Josh Aas]

I filed bug 587378.

Comment 19

[Josh Aas]

Attachment: fix 2.7

Fix a bug in UNIX plugin loading where we'd incorrectly report the result of failing to load a library. This fixes the Talos Tp4 crash that came up when I pushed the previous patch to mozilla-central.

Comment 20

[Josh Aas]

Attachment: fix v2.7 (export)

Comment 21

[Josh Aas]

fix v2.7 pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/1cf25323d6ed