Closed Bug 543405 Opened 10 years ago Closed 10 years ago

improve NPAPI plugin loading code (nsNPAPIPlugin), it's inefficient and error-prone

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

Details

Attachments

(1 file, 4 obsolete files)

NPAPI plugin loading is inefficient and error-prone (I'm not talking about loading instances here, I'm talking about plugins themselves). It is hard to understand how plugin creation works on each platform, including what the similarities and differences are. The problems are mostly in how these two functions are written:

- nsNPAPIPlugin::nsNPAPIPlugin
- nsNPAPIPlugin::CreatePlugin
Attached patch fix v0.9 (obsolete) — Splinter Review
This patch isn't done yet but it works on Mac OS X so far. Provided this approach works out it will be a big improvement. Faster, less duplicated code, better error handling, and much easier to understand.

This does make some functional changes but they are changes we probably should have made a while ago and didn't due to being overly cautious. They may prove to be problematic in the end but I think they should be safe and we should try them.

One change is that on Windows we'll call NP_Initialize before NP_GetEntryPoints instead of after. It shouldn't matter if we initialize before getting the plugin's entry points and it makes Windows match the Mac OS X behavior, where calling NP_Initialize first is important.

Another change is that on Mac OS X we'll no longer copy the plugin's function pointers. We'll give the plugin the buffer we plan to use which avoids the copy and allows the plugin to change pointers later on (we already allow that on other platforms).
Attached patch fix v1.0 (obsolete) — Splinter Review
Works on Linux too. I still need to test on Windows and with OOP plugins.
Attachment #424544 - Attachment is obsolete: true
This will break BeOS and probably OS/2, but BeOS is already broken and OS/2 probably is too. I have no way to make OS/2 work in this patch, someone else will have to fix it later if they care.
Turns out we can't call NP_Initialize before NP_GetEntryPoints on Windows, it breaks Flash.
Attached patch fix v1.1 (obsolete) — Splinter Review
Windows and Mac OS X fixes.
Attachment #424546 - Attachment is obsolete: true
Attachment #424555 - Flags: review?(jst)
Comment on attachment 424555 [details] [diff] [review]
fix v1.1

- In nsNPAPIPlugin::CreatePlugin():

+  nsNPAPIPlugin *plugin = new nsNPAPIPlugin();

*Please* make this be an nsRefPtr<nsNPAPIPlugin> and get rid of every single manual addref/release in this method while you're doing this. And move the declaration of |plugin| to the top of this method since every platform needs this code AFAICT. With this the bottom of this function (and maybe one more case, see comment below) can simply do *aResult = plugin.forget().get() and you can avoid all reference counting here.

- in the unix section:

-  // Do not initialize if the file path is NULL.
-  if (!aFilePath)
-    return NS_OK;

nsPluginFile::GetPluginInfo() in nsPluginsDirUnix.cpp seems to depend on us not initializing a plugin if a null path is passed in.
Attachment #424555 - Flags: review?(jst) → review-
(In reply to comment #6)
> (From update of attachment 424555 [details] [diff] [review])
> -  // Do not initialize if the file path is NULL.
> -  if (!aFilePath)
> -    return NS_OK;
> 
> nsPluginFile::GetPluginInfo() in nsPluginsDirUnix.cpp seems to depend on us not
> initializing a plugin if a null path is passed in.

This is taken care of in the patch already, I just didn't copy the comment.
Attached patch fix v1.2Splinter Review
Attachment #424555 - Attachment is obsolete: true
Attachment #437221 - Flags: review?(jst)
Attachment #437221 - Flags: review?(jst) → review+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/2c07a29e5920
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attached patch 64-bit bustage fix (obsolete) — Splinter Review
This seems to have broken 64-bit OS X builds.

From http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270709260.1270709378.15469.gz:

/builds/slave/mozilla-central-macosx64/build/modules/plugin/base/src/nsNPAPIPlugin.cpp:243: error: no 'void nsNPAPIPlugin::SetPluginRefNum(short int)' member function declared in class 'nsNPAPIPlugin'
/builds/slave/mozilla-central-macosx64/build/modules/plugin/base/src/nsNPAPIPlugin.cpp: In member function 'void nsNPAPIPlugin::SetPluginRefNum(short int)':
/builds/slave/mozilla-central-macosx64/build/modules/plugin/base/src/nsNPAPIPlugin.cpp:245: error: 'mPluginRefNum' was not declared in this scope
/builds/slave/mozilla-central-macosx64/build/modules/plugin/base/src/nsNPAPIPlugin.cpp: In member function 'virtual nsresult nsNPAPIPlugin::Shutdown()':
/builds/slave/mozilla-central-macosx64/build/modules/plugin/base/src/nsNPAPIPlugin.cpp:470: error: 'mPluginRefNum' was not declared in this scope

This patch fixes it for me locally.
Attachment #437790 - Flags: review?(joshmoz)
Thanks Simon! I hope we'll soon have 64-bit Mac OS X try server going :)

I checked in a patch very similar to yours:

http://hg.mozilla.org/mozilla-central/rev/657bebceeb18
Attachment #437790 - Attachment is obsolete: true
Attachment #437790 - Flags: review?(joshmoz)
You need to log in before you can comment on or make changes to this bug.