106435 - NPN_SetValue causes a crash because it works differently between NS4.x and Mozilla

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[David Brittain]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.5) Gecko/20011011
BuildID:    latest trunk build

In NS4.x the plugin is initialised as windowless using the following two calls:

NPN_SetValue(m_instance, NPPVpluginWindowBool, reinterpret_cast<void*>(FALSE));

NPN_SetValue(m_instance, NPPVpluginTransparentBool, reinterpret_cast<void*>(TRUE));

This causes a crash (in fact "the plugin has performed an illegal operation"
dialog) in the latest trunk build of Mozilla.

Debugging reveals that the code expects something like the following:

NPBool value = FALSE;
NPN_SetValue(m_instance, NPPVpluginWindowBool, reinterpret_cast<void*>(&value));
value = TRUE;
NPN_SetValue(m_instance, NPPVpluginTransparentBool,
reinterpret_cast<void*>(&value));

However, if this code is used in NS4.x it doesn't work, the plugin is never
initialised as windowless.


Reproducible: Always
Steps to Reproduce:
1.Get a windowless plugin that uses the NS4.x API.
2.Open an html page that uses this plugin.

Actual Results:  A dialog appears saying the plugin has performed an illegal
operation dialog.

Expected Results:  A windowless plugin should display.

I don't have a simple NS4.x windowlss plugin available otherwise I would attach
it to the bug so that this could be tested. Do you have one?

Comment 1

[David Brittain]

The code in npglue.cpp in the classic codebase shows that NPN_SetValue was
implemented by directly comparing the void* parameter to 0:

http://lxr.mozilla.org/classic/source/modules/plugin/src/npglue.cpp#1755

As opposed to the new code where it is cast to a NPBool* and then dereferenced:

http://lxr.mozilla.org/mozilla/source/modules/plugin/base/src/ns4xPlugin.cpp#1271

Comment 2

[av (gone)]

Attachment: patch, first version

Comment 3

[David Brittain]

Attachment: Updated patch

Comment 4

[David Brittain]

I've attached a version of the patch that works for me. The bool in the call to 
setwindowless was the wrong way round. It is different from the code in npglue 
because, rather misleadingly, the call to "npn_SetWindowless" takes true to 
make it windowed and false to make it windowless.

Comment 5

[av (gone)]

Are you saying that 4x works the way opposite to the spec:
http://developer/docs/manuals/communicator/plugin/pgfns.htm#1007222 where it 
says: 'NPPVpluginWindowBool: Sets windowless mode for display of a plug-in; 
true=windowless, false=not windowless'? 

To me it looks like the first patch is equivalent to the code in npglue.c. 
Compare first patch:

  NPBool bWindowless = (result != nsnull);
  return inst->SetWindowless(bWindowless);

and npglue.c:

  ret = npn_SetWindowless(instance, (PRBool)(0 != r_value));

Comment 6

[David Brittain]

According the documentation at:

http://developer.netscape.com/docs/manuals/communicator/plugin/draw.htm#1005656

"To specify that a plug-in is windowless, use NPN_SetValue with
NPPVpluginWindowBool as the value of variable and false as the value of value."

Although, in npglue.c:

  ret = npn_SetWindowless(instance, (PRBool)(0 != r_value));

if you look at the implementation of npn_SetWindowless, you will see that if the
second parameter is true, then the plugin is windowed, and if it is false then
its windowless, which is the opposite of what you would expect.

Comment 7

[av (gone)]

Comment on attachment 55570 [details] [diff] [review]
Updated patch

You are right, r=av

Comment 8

[av (gone)]

Peter, would you please look at this too? Just as a fresh eye.

Comment 9

[Peter Lubczynski]

Oh wow, so we've had this backwords for all this time?

Comment 10

[David Brittain]

Sorry for not responding sooner - I've been on holiday for the last week.

I think that the logic for the method was the correct way round in Mozilla, it's
just that the parameter was treated as a pointer to a boolean instead of
directly as a boolean.

I haven't spotted this before as I have only now tried to use the 4.x API for
windowless plugins. Until now I have always used XPCOM style plugins. But, as
posts on the newsgroups seem to indicate these are deprecated I've been giving
the 4.x API a go.

Comment 11

[Peter Lubczynski]

Comment on attachment 55570 [details] [diff] [review]
Updated patch

okay, looks fine to me r=peterl

Comment 12

[Chris Waterson]

Please comment the bejesus out of this. sr=waterson

Comment 13

[av (gone)]

Checked in.

Comment 14

[Peter Lubczynski]

I think the branch would like this.

Comment 15

[shrirang khanzode]

dunno how to verify this since there is no windowless plugin testcase. Can 
anyone help testing this? thx

Comment 16

[David Brittain]

shrirang: When bug 109041 gets fixed then I can give you a build of our plugin 
along with a test page and a screenshot of how it looks in NS4.x. However, 
until that bug gets fixed nothing will render.

Comment 17

[shrirang khanzode]

Thanks,David !

Comment 18

[Judson Valeski]

minusing. please re-nominate if this affects mass distro plugins.

Comment 19

[shrirang khanzode]

awesome, verified with Dave's test files that thisis working now. The plugin 
renders fine and no illegal operation dialog appears.

Comment 20

[Peter Lubczynski]

> please re-nominate if this affects mass distro plugins.

Renominating....this does effect all windowless plugins and it would be bad for
our embedding customers if the API wasn't the same as 4.x and MachV, especially
since this problem was brought to my attention by an embedding customer's plugin
vendor (Viewpoint).

Comment 21

[av (gone)]

Looks like windowless train is gaining the speed. We should keep our API as 
consistent as possible. Embedding will definitely benefit from this.

Comment 22

[Judson Valeski]

thanks for the splash of cold water on the face. plussing. please checkin to the
0.9.4 branch and add the "fixed0.9.4" keyword once it's in.

Comment 23

[Peter Lubczynski]

we're in the 0.9.4 branch now!

Comment 24

[David Brittain]

Did the fix for 109041, get checked into the branch? This fix on its own is not
enough for 4.x windowless plugins to work in Mozilla.

Comment 25

[Peter Lubczynski]

No, the fix for bug 109041 did not make it into the branch. Since this bug
depends on that one, I'm going to nominate it now.

Are there any other bugs that have patches that you feel should be checked into
the branch? If so, add "edt0.9.4" to the keywords to nominate.