62248 - nsPluginInstancePeerImpl::GetValue is unimplemented

Status: VERIFIED FIXED

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

Description

[sean echevarria]

nsPluginInstancePeerImpl::GetValue is unimplemented (return NS_ERROR_FAILUR):
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginInstancePe
er.cpp#127

It needs to return the native window handle of the browser window when the 
nsPluginInstancePeerVariable argument is 
nsPluginInstancePeerVariable_NetscapeWindow (which appears to be the only valid 
argument value).

Comment 1

[David Brittain]

Can the interface to this function be changed from:

GetValue(nsPluginInstancePeerVariable variable, void* value)

to:

GetValue(nsPluginInstancePeerVariable variable, void** value)
or
GetValue(nsPluginInstancePeerVariable variable, void*& value)

as otherwise you need to implement this function separately for each platform. 
For example, there is a function available on nsIWidget called GetNativeData. 
If you call this then you get a void* returned. You can't just assign the 
returned data to the 'value' parameter as it won't get returned. But you don't 
know it's real type as this will vary from platform to platform and so you 
can't dereference 'value' and assign to it.

Comment 2

[sean echevarria]

The plugin interface is frozen, so this particular method can't be changed, but 
a new one could be added.  On the other hand, the OS specific piece of the 
implementation should only be a single line (at least in the case of the 
nsPluginInstancePeerVariable_NetscapeWindow argument):

// somehow get the frame that owns the pluginInstance, get the
// widget from the frame and then get the native window
// from the widget
void *nativeWnd = widget->GetNativeData(NS_NATIVE_WINDOW);
#ifdef XP_WIN
*(HWND *) value = (HWND)nativeWnd;
#endif
#ifdef XP_UNIX
// unix equivalent
#endif
#ifdef XP_OS2
// OS2 equivalent
#endif

Comment 3

[Braden]

I don't think methods can be added to frozen interfaces, as they could change
the vtbl layout. A new interface would have to be defined.

(At some point it would be nice to have a plug-in API that is less of a crack
baby, but a thorough requirements analysis--hopefully including windowless
plugins--should precede that.)

Comment 4

[sean echevarria]

Attachment: a start at a patch for GetValue

Comment 5

[sean echevarria]

I've attached a start of a patch for this.  Note the huge TODO comment.
I think the only way to get from the peer to the browser window is via the 
nsPluginInstanceOwner implementation of nsIPluginInstanceOwner.  The 
implementation has 2 members that we need - the frame (on which to call 
GetWindow) that owns the plugin and the context (which is passed in to the 
GetWindow call).

nsPluginInstanceOwner (the impl of nsIPluginInstanceOwner) is declared and 
defined in layout/html/base/src/nsObjectFrame.cpp.

Comment 6

[karnaze (gone)]

Moving to m1.0 and reassigning to sean. sean, if you can't fix this by m1.0, 
please reassign to peterl.

Comment 7

[Arun Ranganathan]

peterl, sean,
this issue is actually resurfacing.  certain vendors deploy frames extensively, 
and want to be able to access a plugin instance residing in a frame.  classic 
example: what if a plugin instance resides in the sidebar?  then, if we can't 
access plugin instances in the sidebar from other pages, plugins in sidebar are 
stymied.  Have I missed the implications of this bug?  I think this is the bug 
fix that can make this happen.  i'm actually thinking of xpcom plugins that 
reside in the components folder, not 4xp plugins (in the plugins folder).

Comment 8

[Arun Ranganathan]

adding momoi and aruner

Comment 9

[sean echevarria]

Not sure I follow the sidebar argument, but scripting of scriptable plugins in 
frame sets is not affected by this bug - it already works.  

Accessing a plugin embedded in the sidebar from a loaded html page is another 
issue - but one that would not benefit from resolution of this bug.

This bug is simply about a plugin being able to get the native window widget of 
the browser in which it is embedded.  It does not affect scriptability and 
applies equally to 4x and xpcom plugins.

Comment 10

[Katsuhiko Momoi]

Hi, sean,

We have heard of a case where a developer tried to implement
control of a plugin from multiple open windows. Can plugin be controlled,
e.g. stop the playing, from multiple windows (open concurrently) under 
the current Plugin code? 
Can a plugin know which browser window is currently which plugin files?

Comment 11

[Katsuhiko Momoi]

> Can a plugin know which browser window is currently which plugin files?

Correct the wording to:

"Can a plugin know which browser window is currently playing which plugin files
 so that it can control playing of a specific plugin file?"

Comment 12

[sean echevarria]

Attachment: first part of testcase for recent comments

Comment 13

[sean echevarria]

Attachment: second part of testcase for recent comments

Comment 14

[sean echevarria]

I've posted a testcase that shows that a plugin can be controlled from another 
window.  This has nothing to do with the plugin code - it's all DOM.  To try 
the testcase on mac or win, install the beatnik player plugin 
(http://www.beatnik.com/software/player.html) and then load the second part of 
the testcase (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35726).  
If you decide to try the testcase, do so in N6 or N6.01.  The current release 
of the Beatnik plugin will not work in Mozilla 0.9.1.


>We have heard of a case where a developer tried to implement
>control of a plugin from multiple open windows. Can plugin be controlled,
>e.g. stop the playing, from multiple windows (open concurrently) under 
>the current Plugin code? 

Any limitations here would be due to the DOM and security, not plugin code.  I 
posted an example of a single window controlling a plugin in another window.  
Works fine.  I'm not sure what is meant by 'control of a plugin from multiple 
open windows'.  As long as whatever is trying to control the plugin has access 
to the plugin then everything's fine.


>Can a plugin know which browser window is currently playing which plugin files
>so that it can control playing of a specific plugin file?

I don't follow the second half of the question.  Due to this bug, a plugin 
cannot use nsPluginInstancePeerImpl::GetValue to determine what browser it is 
embedded in.
A plugin might be able to use nsPluginInstancePeerImpl::GetJSWindow to identify 
what plugins are embedded in the same page (if GetJSWindow is unique to each 
browser window).

Comment 15

[Arun Ranganathan]

sean,
i was wrong. you're right, and thanks for the concise explanation.  this bug 
clearly doesn't affect the cross-frame scriptability of a plugin.  i've tested 
using your attachments, and what i want to determine is:

>Due to this bug, a plugin cannot use nsPluginInstancePeerImpl::GetValue to 
>determine what browser it is embedded in.

Do you mean, what browser "window" it is embedded in?  So a plugin can't figure 
out currently which physical browser window contains the embed tag which spawned 
it?  
Also, you mentioned that Beatnik doesn't work on 0.9.1.  I recently tried 
2001051804 and you're right :-( I get a crash, and also regxpcom.exe behaves 
strangely when your installer tries to register the component.  Could this 
behavior be connected to bug 46775 ?

Comment 16

[sean echevarria]

>Do you mean, what browser "window" it is embedded in?  

Yes.


>So a plugin can't figure out currently which physical browser window contains 
>the embed tag which spawned it?

Right.  But on Windows at least, if the plugin is visible, it can use native
Win32 calls to determine the handle of the browser window.  If the plugin is
hidden, then it's out of luck until GetValue is implemented.


>Also, you mentioned that Beatnik doesn't work on 0.9.1.  I recently tried 
>2001051804 and you're right :-( I get a crash, and also regxpcom.exe behaves
>strangely when your installer tries to register the component.  Could this 
>behavior be connected to bug 46775 ?

The incompatibility was introduced recently - hadn't even looked at regxpcom. 
Bug 46775 is about statically linked builds - is that the bug number you meant?

Comment 17

[Arun Ranganathan]

that bug 46775 is implicated in Beatnik's problems with regxpcom.exe is doubtful
-- i threw it in to make sure that you aren't bitten by any 'changing entry
points' type problem, e.g. NSGetFactory dropped for NSGetModule and any kludgy
macros using NSGetModule are dropped in favor of implementing your own
NSGetModule method,returning your own implementation of nsIModule, and not rely
on the (fragile) binary interface that underlies macros such as
NS_IMPL_NSGETMODULE.  These are Shaver's (correct) recommendations.  If
beatnik's issues are related to these, experiment and log a separate bug -- this
discussion is alien to this particular bug log.

Comment 18

[Arun Ranganathan]

Guess what :-) I've just received another question about this, and I'd like to 
open the floor to workaround suggestions.  Here are the latest comments I've 
received: (it's clear that this is a desired feature)
"I've been playing around with windowless plugins and Mozilla lately, and my
latest task is to get an HWND pointer of the Netscape browser into my
plugin.  In the past (i.e. 4.x), a windowless plugin was able to obtain a
pointer to the window it was embedded in by calling NPN_GetValue() and
passing in NPNVnetscapeWindow.

However, in Mozilla, the GetValue() function returns an error whenever this
variable is asked for.  So I've been pawing through the code trying to
figure out where I can obtain the window pointer.  I'm slowly getting
somewhere, but I was wondering if you knew of an easy way to obtain it, or
what class I should be looking into to find it."

Comment 19

[sean echevarria]

bug 44322 is opened to address issues with the windowless plug-in API.  I don't 
have any workarounds for GetValue if the plugin itself doesn't have an HWND :(

Comment 20

[Peter Lubczynski]

Okay, I found why it returns an error. (You've gota love Open Source)

http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/ns4xPlugin.cpp#932

But.....I don't quite understand what the problem is here. For 4.x plugins when 
SetWindow() is called, a struct containing the HWND of the embedded window is 
always passed in on Windows.

As for NPNVnetscapeWindow, I think it's for the main Navigator window. We may be 
able to hook up nsPluginInstancePeerVariable_NetscapeWindow for new style 
plugins. I think there is a way to get the top level window from a widget by 
walking up some linked list. We can then hook up the 4.x call.

Isn't there a WINAPI call to do thiis?

Comment 21

[sean echevarria]

The native API would work for visible windowed plugins, but not for plugins 
that get called with a NULL HWND arg for SetWindow().

Comment 22

[karnaze (gone)]

Moving to m0.9.2 and reassigning to av.

Comment 23

[Kevin Tieskoetter]

Attachment: Possible patch for sending the window back to a windowless plugin

Comment 24

[Kevin Tieskoetter]

I attached a possible patch for providing the Netscape window to a windowless 
plugin.  I'm afraid I'm not terribly familiar with the Netscape source tree, 
and my patch may not what's needed here, but it seems pretty straightforward. 
Basically, in the ns4xPlugin.cpp's GetValue() function, it passes a request for 
nsPluginInstancePeerVariable_NetscapeWindow to the peer, and in the peer, it 
passes the GetValue() message onto mOwner.  Is this correct?

Comment 25

[Kevin Tieskoetter]

One other note; I think my patch may depend on Dave Brittain's patch for bug 
44322...that patch added some GetValue() functionality, but not everything I 
needed to get windows in windowless plugins.

Comment 26

[Peter Lubczynski]

Nice patch, but totally dependant on bug 44322.

Comment 27

[av (gone)]

Am I missing something? I am looking at the nsIPluginInstanceOwner interface and 
don't see the GetValue method.

Comment 28

[av (gone)]

OK. The patch looks good to me now as 44322 has an updated patch with 
nsIPluginInstanceOwner::GetValue added. But we should obviously wait till that 
patch goes in. Otherwise, r=av

Comment 29

[Marc Attinasi]

I'd like to request a couple of small changes to the patch:

1) use 'NS_SUCCEEDED' instead of 'NS_OK ==' in the test of your call to
inst->GetPeer(&peer)
2) Also, 'inst' needs to be checked or asserted not to be null before it is
dereferenced (can it be null? if not, just assert so we catch possible bugs later)
3) in nsPluginInstancePeerImpl::GetValue you need to check / assert on 'mOwner'
before dereferencing it too.
4) can the 'printf' in nsPluginInstancePeerImpl::GetValue be #ifdef DEBUG'd or
commented out? It is probably not appropriate for release code.

Assuming those are addressed, sr=attinasi

Comment 30

[av (gone)]

Attachment: modified patch with latest comments taken

Comment 31

[av (gone)]

Note that in the latest patch I left out the implementation of 
nsPluginInstancePeerImpl::GetValue since this is going to be addressed by 44322. 
So this patch should go in only after 44322 is checked in.

Comment 32

[Kevin Tieskoetter]

The last change to the patch is checking to see if peer is valid, but I think 
it should be checking if inst is valid first - also, the NS_SUCCEEDED macro is 
already checking if peer is valid.  I would propose this:

if (inst && NS_SUCCEEDED(inst->GetPeer(&peer)))

Comment 33

[av (gone)]

This is addressed in the beginning of the function by

-  if(!npp)
+  if(!npp || !npp->ndata)
     return NPERR_INVALID_INSTANCE_ERROR;

Comment 34

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for the trunk

Comment 35

[av (gone)]

Please note that this fix will not be functional before 44322 is fixed. I 
checked it in because with the latest patch it was not going to break the build.
44322 is ready to go and just waiting for the approval.

Comment 36

[av (gone)]

44322 is now checked in. Marking fixed.

Comment 37

[shrirang khanzode]

verif (stamp)