72017 - dual xpcom/npapi plugins are not initialized properly

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[sean echevarria]

due to the fix made for bug 45009, xpcom plugins which implement the 4x NPAPI 
entry points for binary backwards compatibility with nn4x, the mozilla plugin 
host now wraps these xpcom plugins in the 4x legacy compatibility classes.

Comment 1

[sean echevarria]

Attachment: patch to nsPluginHostImpl.cpp

Comment 2

[sean echevarria]

Attached patch that addresses shortcoming of 
nsPluginHostImpl::GetPluginFactory.  It previously incorrectly assumed that if 
a plugin module did not export "NSGetFactory" then the module was a 4x plugin.

Added code to properly retrieve the plugin factory from an xpcom plugin module.

Comment 3

[sean echevarria]

geez - had the summary right the first time...

Comment 4

[sean echevarria]

adding mozilla0.8.1 keyword in case there's still time to get the fix in.

Comment 5

[karnaze (gone)]

Moving to m0.9

Comment 6

[Peter Lubczynski]

Sean,

Does this have anything to do with bug 65661?

Comment 7

[sean echevarria]

Peter: No, this is strictly a regression that only started happening after the 
patch for bug 45009 went in this week.

I've only started experiencing symptoms similar to bug 65661 this week but 
haven't had time to dig into that particular one.  A lot has changed recently 
that is causing many plugin problems (ie recent cache changes have resulted in 
many cases of OnFileAvailable() not getting called consistently)...

Comment 8

[edburns]

I haven't verified this, but I have a suspicion that this breaks the Java 
plugin.  

I'm told that the java plugin doesn't work with trunk builds right now.

Comment 9

[edburns]

Unfortunately, applying this patch doesn't fix the Java plugin not working in 
the trunk problem.

Comment 10

[sean echevarria]

This bug only affects xpcom plugins that export NSGetModule (for xpcom 
compatibility) and NP_Initialize/NP_Shutdown (for 4x NPAPI compatibility).

xpcom plugins that export the deprecated NSGetFactory instead of NSGetModule 
are not affected.

Comment 11

[sean echevarria]

Andrei, can you please review the attached patch?

Comment 12

[av (gone)]

Looks like this is going to fix it. r=av

Anybody tested it?

Comment 13

[sean echevarria]

yes - I've tested it with the Beatnik xpcom plugin.  Are you aware of any other 
plugins that are binary compatible with both n4x and mozilla?

Comment 14

[av (gone)]

No. Most likely if it is good for your plugin it is good for 100% dual type 
plugins :)

a=av

Comment 15

[Peter Lubczynski]

Sean, do you have check-in rights?

Marc, can you super-review?

Comment 16

[sean echevarria]

Peter: yep.  I sent mail to waterson and reviewers on Monday asking for 
review.  Haven't heard back.

Comment 17

[Marc Attinasi]

I'll admit I'm a bit naive here, but can't you just use
CreateInstanceByContractID instead?

http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsComponentManager.cpp#1230

Comment 18

[sean echevarria]

Marc: I don't think so.  We only have the ContractID for a particular type of 
plugin instance.  Given that ContractID, calling CreateInstanceByContractID 
will create an nsIPluginInstance object vs the nsIPlugin object (the factory 
for the nsIPluginInstance object).  The patch is for 
nsPluginHostImpl::GetPluginFactory and should not result in an 
nsIPluginInstance getting created - just the singleton nsIPlugin (which is 
derived from nsIFactory) for the nsIPluginInstance(s) that will later be 
created.  Clear as mud, no?

Comment 19

[sean echevarria]

I just saw a user agent string in another bug:
Mozilla/5.0 (Windows; U; Win98; en-US; 0.8.1) Netscape6/6.5b0

If Netscape 6.5 is going to be based on Moz 0.8.1 then this REALLY needs to be 
fixed on the branch.  Can anyone alleviate my concerns?

Comment 20

[Marc Attinasi]

sean - Tiyr explanation is very clear, thanks, and sorry for the confusion. I
should have caught that you want to instantiate the factory and not an instance.
sr=attinasi

(ps. you will probably have to get information about NS6.5 in a more private
forum...)

Comment 21

[sean echevarria]

patch checked into trunk.
/cvsroot/mozilla/modules/plugin/nglsrc/nsPluginHostImpl.cpp,v  <--  
nsPluginHostImpl.cpp
new revision: 1.214; previous revision: 1.213

Comment 22

[shrirang khanzode]

verifd patch is in the tree.