45162 - nsIWidget::GetNativeData(NS_NATIVE_PLUGIN_PORT) should return native XID on unix

Status: VERIFIED FIXED

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

Description

[Christopher Blizzard (:blizzard)]

For backwards compatibility and to allow the greatest flexibility we should be
returning the native XID for plugin ports.  Gtk plugins get get the associated
GdkWindow from that port and from there get the GtkWidget if they care.  Other
toolkits ( Xt for example ) can use that as a parent and create their own
application context.

Comment 1

[Stuart Parmenter]

nominating for nsbeta3.  I think blizzard's comment says enough.

Comment 2

[Peter Trudelle]

nsbeta3-, don't see any reason to hold NS6 for this

Comment 3

[Judson Valeski]

if we ship with this, we're bound to a broken api and plugin developers aren't
going to want to write plugins for both apis.

Comment 4

[Peter Trudelle]

Pav, please take a look at this. Embedders are stuck getting Flash plugin
support because of this, because it adds a dependency on GTK, which they remove
to reduce footprint. Marking embed+, will reconsider for rtm+.  cc jrgm

Comment 5

[Stuart Parmenter]

this has nothing to do with plugin developers.  this is a mozilla specific issue
that requires either this bug to be fixed or people such as the xlib guys to add
a patch to our plugin code that makes plugins work with their toolkit.  The xlib
guys have a bug and a patch that makes the xlib port work with plugins again
(55278).

Comment 6

[Judson Valeski]

I'm not following how an xlib patch would solve this problem. the new plugin api
requires a gdksuperwin obj, which some environments might not have. We need to
nail *this* bug to reduce dependencies on plugin writers *and* to maintain
platform partity w/ windows and mac (which provide lower level window object
access).

Comment 7

[Stuart Parmenter]

this has *NOTHING* to do with plugin *WRITERS*

Comment 8

[Christopher Blizzard (:blizzard)]

Pav, are you sure about that?  From just a cursory examination of the interfaces
it looks like we pass information about the window back to a plugin instance in
at least one place nsIPluginInstance::SetWindow():

http://lxr.mozilla.org/seamonkey/source/modules/plugin/public/nsIPluginInstance.h#142

Have a look at SimplePluginInstance::PlatformSetWindow() in the sample code
which is exactly what people writing plugins have to do:

http://lxr.mozilla.org/seamonkey/source/modules/plugin/test/npsimple.cpp#934

Although I think the size issue that keeps getting brought up is pretty bogus,
asking plugin writers to know or care about the superwin code is a big problem.
 I want to get away from it and will with gtk 2.0 but we need to get away from
this for plugin writers.

Comment 9

[Stuart Parmenter]

if plugin writers had to care about it, then flash, etc wouldn't work.

Comment 10

[Stuart Parmenter]

ok.. um.  I think that there are some issues here.. (mainly I'm really confused
about how plugins work now).  I don't understand how we manage to get the
correct window from old plugins.  Somewhere (which I can't seem to find) we are
creating a superwin that wraps the native XWindow (I assume) with either a
superwin or an nsIWidget.. i'm going to guess an nsIWidget since I don't see any
calls to gdk_superwin_new outside of widget...  Blizzard, can you create an
nsIWidget with a XID with the gtk implimentation?  I think now that we should
definatly fix this, but I've got to get a clear understanding of what is going
on with the plugin code and why things are working now before I can do this.

Comment 11

[Christopher Blizzard (:blizzard)]

Uhh...you're thinking of the 4.x plugin compatibility stuff which takes care of
all of this for you.  I'm talking about the new plugin apis.  We obviously can't
change the 4.x api at all.

Comment 12

[Stuart Parmenter]

we pass out a GdkSuperWin in the old 4.x code... who added the superwin stuff to
the new plugin apis?  I don't know anything about that code... the only plugin
code I have worked on is the 4.x compat code.

Comment 13

[Christopher Blizzard (:blizzard)]

No, it isn't.  What is passed to the _plugin_ is the X11 window is in the
ws_info member for 4.x compatibility.

What is passed to the _4.x plugin instance code_ is the superwin.  You create
the X11 window from that.  This means that the plugin instance code for 4.x has
to be changed as well, but not in a way that breaks any compatibility with 4.x
plugins at all.

I say again, what does change and what the embedding people are whining about is
the fact that the new style plugins are passed the superwin and not the raw X
window of the inner window.

Comment 14

[Stuart Parmenter]

Attachment: something like this

Comment 15

[Stuart Parmenter]

this seems to work for me.. i've tested it with flash and the simple plugin...
it probably needs some more error checking though.

Comment 16

[Christopher Blizzard (:blizzard)]

See, easy as pie.  Yeah, you might want to do some more error checking.  For
example, in GetNativeData() make sure that mSuperWin is non-null before
dereferencing it.  I've seen some bad users in the past that try to use it
before Init() or after Destroy().

Comment 17

[Judson Valeski]

I don't see any more room for error checking (based on the new diffs). blizzard,
the mSuperWin null-check is already made (few lines above it's deref). Can the
#include "gdksuperwin.h" line be removed from the 4xplugininstance file?

Comment 18

[Christopher Blizzard (:blizzard)]

OK, I didn't look at the source code around it just the diff.  I wondered if
that check was already there.  And to answer your question, I think that you can
remove the include from the ns4x plugin file.  However, it still requires gtk as
you can tell, just not the superwin structure.

Comment 19

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 20

[ekrock's old account (dead)]

IIUC you've got a patch for this, it's already super-reviewed, and it should be
marked rtm+ for PDT review and approval shortly. If this bug is *not* going to
make RTM for any reason, please email [e-mail] and
[e-mail]. Thanks!

Comment 21

[Judson Valeski]

marking rtm+.

Comment 22

[Judson Valeski]

checked into the embedding branch.

Comment 23

[ekrock's old account (dead)]

PDT: If you're considering minusing this, call ekrock so I can rope in 
engineering and explain why we have to accept this for RTM. The issue is that we 
*have* to accept this for the embedding branch as this bug totally horks 
plug-ins in the embedded context. If we don't accept it on the branch and trunk 
as well, we've just forked the plug-in API between Netscape 6 and the 
trunk/embedding branch, and plug-in developers won't be willing to create, 
distrtibute, and maintain two versions of their plug-in. In a nutshell, this is 
one of those situations where if we don't get this right in the first release, 
we've given ourselves an irreparable self-inflicted wound forever. So please 
accept the patch into the branch.

Comment 24

[Phil Peterson]

rtm++

Comment 25

[Stuart Parmenter]

its on the netscape branch.  now for the trunk

Comment 26

[Stuart Parmenter]

checked in on the trunk, branch, embedding branch.. hope thats it.  marking fixed

Comment 27

[drapeau]

pavlov (just reading this bug traffic after some time off): Java Plug-In is a 
new-style plugin, and it broke as a result of this bug fix.  So, to address your 
 comment of 2000-10-05 22:48, plugin writers *do* care about this bug; at least, 
new-style plugin writers.  I don't know why the Flash plugin continued to work; 
perhaps it's just an old-style plugin.

ekrock: Sun agrees that this change should be made, but for the record, this 
broke Java Plug-In working in recent builds.  I learned today that while I was 
gone, Ed Burns brought some Java bits to Netscape RE last week.  Those bits no 
longer work with current builds.  The fix is minor and easily understood, I 
believe, but still requires a re-spin of Java Plugin.  I hope we get accepted!

For more info, please see bug 57046 about the breakage on Linux/Solaris.

Comment 28

[Christopher Blizzard (:blizzard)]

The flash plugin was an older 4.x plugin so was unaffected by this change.
However, I thought that the Java plugin was also a 4.x style plugin as well
otherwise I would have brought the java people into this.  Sorry about that.

Comment 29

[shrirang khanzode]

verified that the patch is in the branch (version 1.34). Adding vtrunk keyword.

Comment 30

[Nikolay Igotti]

Well, maybe I don't understand smth, but why Blizzard's fix should work on big
endian 32 bits platforms (really on all, but on little endian
it occasionally works). Let's see
/usr/openwin/include/X11/X.h:typedef unsigned long XID;
sizeof(void*) = 4
nsWindow.cpp: return (void *)GDK_WINDOW_XWINDOW(mSuperWin->bin_window);
So we just lost 4 bytes (on little endian it's upper, on big endian
lower). I think this bug must be reopened as it prevents all new style plugins
from working on big endian platforms. Dixi.

Comment 31

[drapeau]

blizzard: that's okay, unless you're Mr. Guardian Of All New-Style Plugins, I
don't see how you could really know how all of us could be affected.  I don't
know of anybody keeping track of all the new-style plugins on the Unix
platforms.

Nikolay: I don't think the problem you're seeing is a byte truncation problem. 
It could be byte-ordering, but not truncation.  I don't know what's causing that
problem.  Sounds like you have some new info to post to this bug report, though,
that happens when you're running on a big-endian CPU but displaying on an X
server that is running on a little-endian CPU.

Comment 32

[Nikolay Igotti]

Yeah, George you are right. Sorry Blizzard :). Really I've got strange problem -
it's OK for big-endian prog on big-endian display, but broken when big-endian
prog on little-endian display. 
I just don't understand why it happens.

Comment 33

[Christopher Blizzard (:blizzard)]

Aww, crap.  This is a very serious problem.  I hadn't considered that the X
window ID might be an unsigned long.  This means that it isn't going to fit into
a pointer on some platforms.  We pass it through a pointer size in two places -
as a result from GetNativeData() and through nsPluginWindow.window.  We can't
change this since I think that the 4.x compatibility stuff on all platforms
requires this alignment of variables in the struct.

Comment 34

[Nikolay Igotti]

Blizzard, I don't think it's serious problem, as X headers takes care that size
of XID == 32 (_XSERVER64 define). But I yet don't understand why I have a
problem with endiannes mixing. Really it seems XReparentWindow hack doesn't 
work and window isn't reparented on Linux display (althoug XID is correct).

Comment 35

[Stuart Parmenter]

so long as an XID is 32bits, we should be ok.  I don't know why you would be
having endianness problems.  the size problem would only kick in on 64bit
machines if it was just an unsigned long on them.

Comment 36

[Christopher Blizzard (:blizzard)]

Maybe it's what's being discussed in bug #57046?

Comment 37

[Nikolay Igotti]

Yes blizzard, it's true. XSync() really helps. I think it happens as JDK and
Mozilla uses different X server connections, so until you'll XSync()
parent window XID is invalid for another X connection(JDK).

Comment 38

[Christopher Blizzard (:blizzard)]

Does this help with the problem that you describe in 57046 ( Applets crash
mozilla )?

Comment 39

[shrirang khanzode]

v