Last Comment Bug 741246 - simplify plugin instance creation and initialization
: simplify plugin instance creation and initialization
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Josh Aas
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-01 15:30 PDT by Josh Aas
Modified: 2012-04-03 02:03 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (7.62 KB, patch)
2012-04-01 15:30 PDT, Josh Aas
jst: review+
Details | Diff | Splinter Review

Description Josh Aas 2012-04-01 15:30:00 PDT
Created attachment 611321 [details] [diff] [review]
fix v1.0

- Better initialization method names/signatures.

- Remove unnecessary current directory initialization hack for Windows (doesn't do anything any more, were around for OJI I believe).
Comment 1 Josh Aas 2012-04-01 15:34:31 PDT
try server run:

https://tbpl.mozilla.org/?tree=Try&rev=0d7c620a14fb
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-02 07:54:26 PDT
Comment on attachment 611321 [details] [diff] [review]
fix v1.0

- In nsPluginHost::TrySetUpPluginInstance():

>-#if defined(XP_WIN)
>-  static BOOL firstJavaPlugin = FALSE;
>-  BOOL restoreOrigDir = FALSE;
>-  WCHAR origDir[_MAX_PATH];
>-  if (pluginTag->mIsJavaPlugin && !firstJavaPlugin) {
>-    DWORD dw = GetCurrentDirectoryW(_MAX_PATH, origDir);
>-    NS_ASSERTION(dw <= _MAX_PATH, "Failed to obtain the current directory, which may lead to incorrect class loading");
>-    nsCOMPtr<nsIFile> binDirectory;
>-    nsresult rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
>-                                         getter_AddRefs(binDirectory));
>-
>-    if (NS_SUCCEEDED(rv)) {
>-      nsAutoString path;
>-      binDirectory->GetPath(path);
>-      restoreOrigDir = SetCurrentDirectoryW(path.get());
>-    }
>-  }
>-#endif

I suspect this code is still important in subtle ways to Java, and I think it's in our interest to leave this in place. IIRC this is important from a security point of view inside the Java plugin.

r=jst if you leave the above code, plus its counter part, in place.
Comment 3 Josh Aas 2012-04-02 09:39:39 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #2)

> I suspect this code is still important in subtle ways to Java, and I think
> it's in our interest to leave this in place. IIRC this is important from a
> security point of view inside the Java plugin.

The only thing that happens when we have the current directory switched is:

nsRefPtr<nsNPAPIPluginInstance> instance = new nsNPAPIPluginInstance(plugin.get());

The nsNPAPIPluginInstance constructor doesn't do anything except set some member variables to basic values - it doesn't call into the plugin at all. I'm not seeing how the current directory could matter for any of that. When I looked up the history of the code it seemed to be there for some old Java XPCOM/OJI plugin initialization that has been since removed.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-02 12:07:46 PDT
Yup, you're right, this is no longer relevant, so remove away...
Comment 5 Josh Aas 2012-04-02 12:18:27 PDT
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/029a32f787e9
Comment 6 Marco Bonardo [::mak] 2012-04-03 02:03:34 PDT
https://hg.mozilla.org/mozilla-central/rev/029a32f787e9

Note You need to log in before you can comment on or make changes to this bug.