[OOPP] OOPP are also loaded in main process

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: BenWa, Assigned: jaas)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

9 years ago
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'.
(Reporter)

Comment 1

9 years ago
(Reporter)

Comment 2

9 years ago
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.
Attachment #460754 - Flags: review?(joshmoz)
(Reporter)

Updated

9 years ago
Assignee: nobody → b56girard
Status: NEW → ASSIGNED
(Assignee)

Comment 3

9 years ago
Posted patch fix v2.0 (incomplete) (obsolete) — Splinter Review
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.
(Reporter)

Updated

9 years ago
Assignee: b56girard → joshmoz
(Assignee)

Updated

9 years ago
Attachment #460754 - Attachment is obsolete: true
Attachment #460754 - Flags: review?(joshmoz)
(Assignee)

Comment 4

9 years ago
Posted patch fix v2.1 (obsolete) — Splinter Review
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.
Attachment #461902 - Attachment is obsolete: true
Attachment #462331 - Flags: review?(b56girard)
(Reporter)

Comment 5

9 years ago
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.
Attachment #462331 - Flags: review?(b56girard)
(Assignee)

Comment 6

9 years ago
Posted patch fix v2.2 (obsolete) — Splinter Review
Attachment #462331 - Attachment is obsolete: true
Attachment #463994 - Flags: review?(b56girard)
(Reporter)

Comment 7

9 years ago
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.
Attachment #463994 - Flags: review?(b56girard)
(Assignee)

Comment 8

9 years ago
Posted patch fix v2.3Splinter Review
Attachment #463994 - Attachment is obsolete: true
Attachment #464125 - Flags: review?(b56girard)
(Assignee)

Updated

9 years ago
Attachment #464125 - Flags: review?(benjamin)
(Reporter)

Updated

9 years ago
Attachment #464125 - Flags: review?(b56girard) → review+
(Assignee)

Updated

9 years ago
blocking2.0: --- → betaN+
(Assignee)

Updated

9 years ago
Blocks: 559142
(Assignee)

Updated

9 years ago
blocking2.0: betaN+ → beta4+

Comment 9

9 years ago
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?
Attachment #464125 - Flags: review?(benjamin) → review+
(Assignee)

Comment 10

9 years ago
Posted patch fix v2.4Splinter Review
Includes Linux compile fixes.
(Assignee)

Comment 11

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

Re-requesting review for significant Linux bustage fix and API change.
Attachment #464570 - Flags: superreview?(benjamin)
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.
Attachment #464570 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 13

9 years ago
Posted patch fix v2.5 (obsolete) — Splinter Review
(Assignee)

Comment 14

9 years ago
Posted patch fix v2.6 (obsolete) — Splinter Review
Attachment #464970 - Attachment is obsolete: true
(Assignee)

Comment 15

9 years ago
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
(Assignee)

Comment 16

9 years ago
Posted file 64-bit Linux tbox failure (obsolete) —
(Assignee)

Updated

9 years ago
blocking2.0: beta4+ → beta5+
(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?
(Assignee)

Comment 18

9 years ago
I filed bug 587378.
Depends on: 587378
(Assignee)

Comment 19

9 years ago
Posted patch fix 2.7 (obsolete) — Splinter Review
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.
Attachment #465150 - Attachment is obsolete: true
Attachment #465589 - Attachment is obsolete: true
(Assignee)

Comment 20

9 years ago
Attachment #466767 - Attachment is obsolete: true
(Assignee)

Comment 21

9 years ago
fix v2.7 pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/1cf25323d6ed
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 591019
You need to log in before you can comment on or make changes to this bug.