58491 - ns4xplugin interface incorrect

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Mike Kaply [:mkaply]]

The ns4xplugin interface was written using a C++ class.

This incorrectly assumes that the compiler being used uses the same calling 
convention for C and C++.

This would be OK if this were a new interface, but this is a legacy interface so 
it must work the same way it used to.

The calls that the plugin makes back to Mozilla should be genuine C calls, they 
should NOT be member functions of a class.

I already have this fixed and will be attaching a patch in a few moments.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Fixes to ns4xplugin.* - note there are some tab fixes in here as well.

Comment 2

[Mike Kaply [:mkaply]]

This is a very urgent problem for getting plugins working on OS/2.

scc has agreed that this interface is incorrect.

Comment 3

[Mike Kaply [:mkaply]]

av, can I please get a review on this?

Comment 4

[av (gone)]

I don't fully understand the patch you have attached. There is still a class 
declaration in the header file, is this correct?

Comment 5

[Mike Kaply [:mkaply]]

Unfortunately, the CVS diff really doesn't give any info about this change.

Sorry about that.

Basically what I have done is move all the callback functions (_geturl, etc.) 
out of the plugin class.

They are now standalone C functions.

This causes them to have C calling convention.

Comment 6

[av (gone)]

Would you please attach the modified versions of the whole files?

Comment 7

[Mike Kaply [:mkaply]]

Attachment: ns4xPlugin.h

Comment 8

[Mike Kaply [:mkaply]]

Attachment: ns4xPlugin.cpp

Comment 9

[av (gone)]

OK, I applied your patch, and it seems to work on Windows. r=av
You will need a super review to get it in.

Comment 10

[Scott Collins]

prefer the idiom for pointer tests, e.g.,

  if ( gServiceMgr )
    return NPERR_GENERIC_ERROR;

as opposed to an explicit comparision against my personal enemy, |nsnull|.  In
your assertions as well.

Don't use old-style casts (yeah, I know that's what was already there, but if
you're touching the code...).  I see |(nsISupports**)&pm| et al.  Use
|NS_STATIC_CAST|, it should be a habit --- it catches errors.

It looks like the code started out with some questionable practices, so I won't
bug you to fix them all.  I don't see any real problems with this code ... just
minor language technicalities.  I'd appreciate it if you fixed the casts, and
the comparisons.  Other than that, sr=scc.

Comment 11

[Mike Kaply [:mkaply]]

Attachment: Per scc - basically a rewrite of ns4xplugin - tabs and all

Comment 12

[Mike Kaply [:mkaply]]

Attachment: H file corresponds to above changes

Comment 13

[Scott Collins]

ouch.  Can I have it as a diff -u please? :-)

Comment 14

[Mike Kaply [:mkaply]]

Attachment: Pre scc, -u diffs of h and c.

Comment 15

[Mike Kaply [:mkaply]]

Attachment: Whee this is fun. diff with -uw for scc to ignore whitespace

Comment 16

[Scott Collins]

Reviewed with mkaply on IRC.  For the benefit of other readers, here were my
comments:

  first thing is, if you have to touch |QueryInterface|, you might as well
  replace  it with the macro |NS_IMPL_QUERY_INTERFACE3| or whatever is
  appropriate

  and you got one of your tests backwards
  should be |if (aServiceMgr && !gMalloc) ...| not |!aServiceMgr|

  in |_destroystream|, since you have to QI anyway, you should use an |nsCOMPtr|
  which will make the code shorter and safer

  but since you have to call |GetService| anyway [in |reloadplugins|, et al],
  you should use an |nsCOMPtr| in this situation, and use |do_GetService|

  most of these routines would benefit from appropriate use of |nsCOMPtr|

Comment 17

[Mike Kaply [:mkaply]]

Could you be more specific on the _destroystream thing?

here's the code:

  nsISupports* stream = NS_STATIC_CAST(nsISupports*, pstream->ndata);
  nsIPluginStreamListener* listener;

  if(stream->QueryInterface(kIPluginStreamListenerIID, 
NS_STATIC_CAST(void**, &listener)) == NS_OK)
  {
    // XXX we should try to kill this listener here somehow
    NS_RELEASE(listener);	
    return NPERR_NO_ERROR;
  }

Would it look like this?

  nsISupports* stream = NS_STATIC_CAST(nsISupports*, pstream->ndata);
  nsComPtr<nsIPluginStreamListener> listener = 
do_QueryInterface(stream);

  if(listener)
  {
    // XXX we should try to kill this listener here somehow
    NS_RELEASE(listener);	
    return NPERR_NO_ERROR;
  }

That doesn't make a lot of sense to me. Maybe I'm missing something.

Comment 18

[karnaze (gone)]

Moving to m0.9.1 and reassigning to scc.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why shouldn't mkaply own this?

Comment 20

[Mike Kaply [:mkaply]]

Actually, I think someone in plugin land should own this.

What is left is a cleanup of ns4xplugin.cpp because it does stuff "the old way" 
per scc.

I could certainly take a look at it, but I am way backlogged right now

Comment 21

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tonight. 
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 22

[chris hofmann]

looks like we could live without these for 0.9.4.   let me know if this is a
mistake.  moving out...

Comment 23

[Asa Dotzler [:asa]]

0.9.5 is out the door. bumping TM up by one.

Comment 24

[chris hofmann]

unsetting the target milestone...  
doesn't seem like this is too critical.  is anybody really waiting on this?

peter, is this something that you could pick up?

Comment 25

[Peter Lubczynski]

wow, this is an old bug. Looking at ns4xPlugin today, it looks like most of them
have already been converted to C functions. 

-->over to mkaply to see if he wants to do anything else with this bug...

Comment 26

[Mike Kaply [:mkaply]]

The only thing left on here is the scc comment:

prefer the idiom for pointer tests, e.g.,

  if ( gServiceMgr )
    return NPERR_GENERIC_ERROR;

as opposed to an explicit comparision against my personal enemy, |nsnull|.  In
your assertions as well.

Don't use old-style casts (yeah, I know that's what was already there, but if
you're touching the code...).  I see |(nsISupports**)&pm| et al.  Use
|NS_STATIC_CAST|, it should be a habit --- it catches errors.

It looks like the code started out with some questionable practices, so I won't
bug you to fix them all.  I don't see any real problems with this code ... just
minor language technicalities.  I'd appreciate it if you fixed the casts, and
the comparisons.  Other than that, sr=scc.

This file uses old style.

The actual bug has been fixed.

Comment 27

[Peter Lubczynski]

-->FIXED

Comment 28

[Roland Mainz]

If this is FIXED - why do a lot of NS4.x plugins incl. the X.org Broadway plugin
still not work ?

Comment 29

[Mike Kaply [:mkaply]]

This defect was about a specific issue that the 4.x plugin interface was written 
in C++ rather than C so there were calling convention issues.

This issue is fixed.

Comment 30

[shrirang khanzode]

v