Closed
Bug 578868
Opened 14 years ago
Closed 14 years ago
[OOPP] OOPP are also loaded in main process
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(blocking2.0 beta5+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta5+ |
People
(Reporter: BenWa, Assigned: jaas)
References
Details
Attachments
(3 files, 8 obsolete files)
30.07 KB,
patch
|
BenWa
:
review+
benjamin
:
review+
|
Details | Diff | Splinter Review |
32.93 KB,
patch
|
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
30.64 KB,
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Reporter | ||
Comment 2•14 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•14 years ago
|
Assignee: nobody → b56girard
Status: NEW → ASSIGNED
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•14 years ago
|
Assignee: b56girard → joshmoz
Attachment #460754 -
Attachment is obsolete: true
Attachment #460754 -
Flags: review?(joshmoz)
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•14 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)
Attachment #462331 -
Attachment is obsolete: true
Attachment #463994 -
Flags: review?(b56girard)
Reporter | ||
Comment 7•14 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)
Attachment #463994 -
Attachment is obsolete: true
Attachment #464125 -
Flags: review?(b56girard)
Attachment #464125 -
Flags: review?(benjamin)
Reporter | ||
Updated•14 years ago
|
Attachment #464125 -
Flags: review?(b56girard) → review+
Comment 9•14 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•14 years ago
|
||
Includes Linux compile fixes.
Assignee | ||
Comment 11•14 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 12•14 years ago
|
||
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•14 years ago
|
||
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #464970 -
Attachment is obsolete: true
Assignee | ||
Comment 15•14 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•14 years ago
|
||
Comment 17•14 years ago
|
||
(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 19•14 years ago
|
||
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•14 years ago
|
||
Attachment #466767 -
Attachment is obsolete: true
Assignee | ||
Comment 21•14 years ago
|
||
fix v2.7 pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/1cf25323d6ed
Status: ASSIGNED → RESOLVED
Closed: 14 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
•