135737 - [viewpoint] windowless plugin receives incorrect SetWindow calls

Status: VERIFIED FIXED

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

Description

[Ari Berger]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    trunk or .9.9 branch

when the plugin is started, we get correct coordinates in the SetWindow call.  
(For this content, something about (8,8) is reasonable).
Then we start getting SetWindow calls before each paint message, and the 
numbers don't make any sense.  Example (-137, -58) for the top-left corner.

Reproducible: Always
Steps to Reproduce:
1.Install Viewpoint plugin (http://cole.viewpoint.com/~aberger/builds/vpt.zip)
2. View content:
http://cole.viewpoint.com/~aberger/primitive/


Actual Results:  we get incorrect window coordinates.  Mouse interactivity is 
confused.

Comment 1

[rubydoo123]

assigning to av and setting to 1.0.1

Comment 2

[Peter Lubczynski]

-->taking bug

This is a serious problem with the Viewpoint plugin (and other windowless
plugins) with painting and will be important to embedding.

This problem maybe caused because now on the trunk we recreate the context each
time we tell the plugin to paint and we are trying to do a translation to tell
it to paint in the right place:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsObjectFrame.cpp#1699

Comment 3

[rubydoo123]

Just had a conversation with peterl in regards to this problem. It seems that 
what happens is that the plug-in writes all over the window, including hte 
chrome. That can cause all sorts of unexpected behavior, including the 
possibility of not being able to access objects under the plug-in. This is 
probably not limited to ViewPoint, but rather global in nature. That would mean 
that the exposure is to a very large audience (think PDF file viewing). Due to 
the large impact of this issue, I am setting to ADT1

Comment 4

[David Brittain]

This problem was introduced by the fix to bug 118721 . It should only affect
windowless plugins and I'm pretty sure that the PDF plugin doesn't use
windowless mode. There are not many plugins around that use windowless mode at
all,  my company has not released a new windowless plugin that works with
Mozilla (our currently released NS4.x plugin will work with Mozilla in
windowless mode, but none of the scripting works so it is basically useless). As
far as I know the only current windowless plugin for Mozilla is Viewpoint's.

The reason for this problem is that Mozilla has an optimised redraw process, if
only one region has been invalidated then a DC is created for that region alone
and the plugin is only asked to repaint that region.

I haven't got access to a debug build at the moment, so can you confirm that the
plugin draws correctly and that the problem is that you have no way of
determining the position of the mouse coordinates relative to your windowless
plugin?

The difficulty with this problem is that in NS4.x this never happened, the
drawing process was not so well optimised, so looking at the docs it is not
clear what the correct way of doing things is.

NPWindow is defined here:

http://developer.netscape.com/docs/manuals/communicator/plugin/refstrc.htm#npwindow

The line:

"x, y The x and y coordinates for the top left corner of the plug-in relative to
the page (and hence, relative to the origin of the drawable). These values
should not be modified by the plug-in."

the "and hence" bit no longer makes sense as it is not necessarily the case that
the origin of the drawable is the same as the origin of the page. So, how do we
pass the info to the plugin to tell it what the origin of the drawable is?

One solution is to leave this as it is now implemented so that SetWinow passes
the origin of the plugin relative to the drawable which leads to the negative
coordinates that you are seeing. And change the windowless mouse event handling
functions to fixup the coordinates so that they are relative to the drawable
passed on the last paint. Your mouse handling code may even work unchanged with
this approach?

Comment 5

[Ari Berger]

Your explanation makes a lot of sense. You are correct that we seem to draw in 
the right place but our mousing is wrong.
Let me make sure I understand.  Before this fix, SetWindow had the origin of 
the plugin relative to the page.  Paint messages would have the offset within 
the window where we should paint.  After the fix, we receive SetWindow messages 
before each paint.  We always Paint at (0,0), and the SetWindow gives us the 
offset FROM the paint DC to the origin.
Mouse events used to be (and still are) relative to the origin of the plugin 
embed (so 0,0 would be a mouse event at the topleft of the embed).  However, 
now, we think that the embed is half off the page (because its we got SetWindow 
with a negative origin) and a (0,0) mouse event doesn't make too much sense.
Is that correct?
The other problem that I have to look into is our efficiency.  I think (but I 
need to check) that we may assume SetWindow is being called to tell us that the 
window has really moved (scrolled, probably) and invalidate our entire window.  
I need to look into this.

Comment 6

[David Brittain]

>Your explanation makes a lot of sense. You are correct that we seem to draw in 
>the right place but our mousing is wrong.
>Let me make sure I understand.  Before this fix, SetWindow had the origin of 
>the plugin relative to the page.  Paint messages would have the offset within 
>the window where we should paint.

Do you mean with the offset that it is contained in the NPRect that is passed 
via the lParam of the WM_PAINT message. If so then yes that's right.

>After the fix, we receive SetWindow messages 
>before each paint.  We always Paint at (0,0), and the SetWindow gives us the 
>offset FROM the paint DC to the origin.

I don't think that the lParam will always specify a rect at (0,0). I still 
haven't got access to a debug build or my plugin source to verify the actual 
behaviour, but this is what I'd guess would happen. Say your plugin is at (50, 
50) and is 100x100 in size.
On first draw:
SetWindow will pass (50, 50, 100, 100) for (x, y, width, height) and the dirty 
rect will be (50, 50, 150, 150) for (left, top, right, bottom)
If the plugin called InvalidateRect on its whole region then:
SetWindow will pass (0, 0, 100, 100) and the dirty rect will be (0, 0, 100, 100)
If something were dragged over the bottom right corner of the plugin from 50, 
50 then:
SetWindow will pass (-50, -50, 100, 100) and the dirty rect will be (0, 0, 50, 
50)

>Mouse events used to be (and still are) relative to the origin of the plugin 
>embed (so 0,0 would be a mouse event at the topleft of the embed).  However, 
>now, we think that the embed is half off the page (because its we got 
>SetWindow with a negative origin) and a (0,0) mouse event doesn't make too 
>much sense.
>Is that correct?
That's correct.

Comment 7

[David Brittain]

It seems like we have three possible solutions to this:

1) Switch of optimised drawing on the view containing the plugin. Would this be
achieved by disabling double-buffering? See, this code for how it's done on the mac:

http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsObjectFrame.cpp#753

It would be a bit of a shame to do this as it would remove the benefit of the
optimised drawing.

2) Call SetWindow (again!) with the offset of the plugin before firing a mouse
events.

This could be quite ineffecient, it depends what processing gets done in
SetWindow by the plugin.

3) Modify the mouse event that gets passed to the plugin so that it is relative
to the window.x and window.y last passed to SetWindow.

This would lead to some weird values for the mouse coords being passed through
to the plugin. But, the code in a typical windowless plugin would subtract the
last value passed in window.x and window.y from the mouse coords, which would
magically produce the correct mouse coordinate.

Any opinions?

Comment 8

[Peter Lubczynski]

Well, I think we've got two choices:

1) Go back to the way we used to do it and pass a DC the size of the page

2) Relnote this issue and provide a way for windowless plugins to find out their
absolute location

For #1, here's a possibility of what we can do:
1. When a windowless plugin is told to paint, create a scratch buffer the size
of the document -- like we did in 0.9.4 and 4.x.
2. Copy the translated bits of the current backbuffer into this new scratch bufffer
3. Pass the scratch buffer to the windowless plugin to Paint (call
SetWindow/Paint like before)
4. Copy and translate the bits from the scratch buffer back over the old back buffer

For #2, if the plugin couldn't determine it's location with just math, maybe
implement a new NPN_GetValue variable or possibly add info to the NPWindow struct.

The question is what is the right thing to do here: closer compatibility or
optimized painting?

Comment 9

[Ari Berger]

To expand on Peter's point #2:
The reason that we need to know the offset of where our plugin is on the page 
is that sometimes we don't get mouse messages from the browser (these should be 
offset correctly).
The problem is that for various reasons, we sometimes need to capture the 
mouse.  In this case we get the events from the OS (relative to screen).  To 
find the location relative to the plugin embed, we subtract 2 offsets: the 
browser view window (which we get from GetWindowRect() on the window we get 
from NPN_GetValue(NPNVnetscapeWindow)) and the offset of the plugin from the 
browser (which is what USED to come in from SetWindow).
To make this work, we would need to get that value- the offst of the plugin 
relative to the browser window- in some other way.
There are 2 ways we can get it:
1) You could provide a NPN_GetValue for it (however, we would rather not have 
to ask all the time)
2) We could do the math based on the offsets we get in SetWindow and Paint and 
calculate it.  However, what if we get a SetWindow not followed by a Paint (or 
what if the OS sends us a mouse message between setwindow and paint)?
I will continue to look at this issue.

Comment 10

[David Brittain]

Ari, having had a chance to look at our code again I now understand the problem
with SetCapture.

Instead of NPN_GetValue being called all the time how about we use NPN_SetValue
instead and pass a pointer to an NPRect, if this value has been set by the
plugin then the browser should fill it with the offset of the plugin (and update
it every time this offset changes). If we did this then we could also write it
so that if this value isn't set then optimised drawing is also disabled. That
way we get the best of both worlds, compatibility and optimised drawing if you
choose it.

Comment 11

[Ari Berger]

that sounds like a great solution to me.

Comment 12

[David Brittain]

Attachment: Disable optimised drawing for windowless plugins

I think that this patch disables optimised drawing for windowless plugins - I
haven't tested it that much. I am working on a complete patch which includes
the ability for the plugin to re-enable optimised drawing and pass in a pointer
to a rectangle to get the plugin position. That's going to take a few more days
though.

Comment 13

[Ari Berger]

I applied the patch.
It does solve the mouse problem- things behave properly.  However, the drawing 
pipe is not correct.  Everything is in the correct place, but there is some 
double-blitting going on.  Every time we draw, our content blinks.  It looks 
like you blit to the screen before we get a chance to draw.

Comment 14

[David Brittain]

Ari, thanks for testing that.

I also tried using the following in the same place (not fully understanding what
it was meant to do!):

viewMan->SetViewBitBltEnabled(view, PR_FALSE);

but that doesn't seem to make any difference at all.

I'm afraid I'm out of ideas as to how we disable optimised drawing.

Comment 15

[Peter Lubczynski]

hm....I've been taking example from nsWindow::OnPaint() and trying to do
something with CopyOffScreenBits:

nsRect bounds;
nsCOMPtr<nsDrawingSurfaceWin> myBuffer;
aPresContext->GetVisibleBounds(bounds);
aRenderingContext.CreateDrawingSurface(&bounds,
NS_CREATEDRAWINGSURFACE_FOR_PIXEL_ACCESS, (void*&)*getter_AddRefs(myBuffer));
HDC hDC;
myBuffer->GetDC(&hDC);
plugin->Paint(hDC, 0.9.4 window math);
aRenderingContext.CopyOffScreenBits(myBuffer, aDirtyRect.x, aDirtyRect.y,
NS_COPYBITS_TO_BACK_BUFFER);
aRenderingContext.DestroyDrawingSurface(myBuffer);

It's not working quite yet. Maybe it would be better just to hand back an NPRect?

Comment 16

[David Brittain]

I think that passing an NPRect is probably the way to go. Given that as AFAIK
the only thing that is broken by this bug is getting the mouse position when it
is captured. Adding all that code for creating a new surface etc. seems like
overkill. 

Comment 17

[Ari Berger]

Additional info regarding the patch:
I do not always see the double blitting problem.  In fact, when my dirty rect 
(the area that I invalidate) is entirely within the embed window, things work 
perfectly: drawing and mousing.
Once any part of the dirty rect extends outside the embed window, I see the 
flickering.  
To see our dirty rects, open the MetaStreamConfig.ini file in the Program 
Files\Viewpoint Media Player (or Viewpoint Experience Technology) folder.  In 
the [ComponentMgrOptions] section, add DirtyRects=1.  I don't know if this 
helps.

Comment 18

[Peter Lubczynski]

Attachment: new variable patch, v.1

This patch adds a new way through NPN_GetValue for plugins to get back a
pointer to an NPRect struct which has their location. I'm adding
NPNVpluginWindowLocation/nsPluginInstancePeerVariable_PluginWindowLocation.

Please test this out to see if it fits your needs.

Comment 19

[Peter Lubczynski]

So I talked with Ari and I think I'm going to try a little simplier approach. In
DidReflow and ScrollPositionChange I'm going to pass an NPRect through
HandleEvent. That should keep the plugin notified of where it is (the old
purpose of SetWindow) and be close to the old behavior.

Comment 20

[Peter Lubczynski]

Attachment: patch to send WM_WINDOWPOSCHANGED events v.1

Gota run to a meeting, I haven't had time to test it out, but here's a patch
that sends an NPRect in lParam through HandleEvent when either we scroll of
have finished (re)layout. Let me know if you find any issues.

Comment 21

[Peter Lubczynski]

Comment on attachment 79904 [details] [diff] [review]
patch to send WM_WINDOWPOSCHANGED events v.1

I don't think this patch is calculating the offset from the widget correctly
and I don't think it's working as desired with scrolling.

Comment 22

[David Brittain]

I think that you need to use the code that was backed out in bug 132262 and call
it from inside Paint to check if the plugin has moved. I think that DidReflow
doesn't get called if the plugin is in a table (see bug 114667).

Comment 23

[Peter Lubczynski]

Attachment: patch v.2

Okay, this patch sends rects just like before (0.9.4) except now through
HandleEvent. This patch looks like it's doing the right thing, even when
scrolling.

Comment 24

[Ari Berger]

OK, this looks like it will work for us.
Do you need to send us a HandleEvent WM_WINDOWPOSCHANGED with every paint 
though?
We used to get a Paint message to paint, now we get a WM_WINDOWPOSCHANGED, 
SetWindow, and then Paint.
Would it make sense to pass us this event only when the window has actually 
changed?
I need to play with it some more, see what it means efficiency-wise for us, but 
the fix works.

Comment 25

[Ari Berger]

One problem- the window you send in through handleevent is not clipped as I 
would expect.
If you resize the browser window so the plugin embed is clipped, you still pass 
us the full original size of the window, not the actual size.

Comment 26

[Ari Berger]

One problem:
In nsObjectFrame.cpp:
NPRect rect = {origin.y, origin.x, origin.y + window->height, origin.x + window-
>width};

NPRect is a set of uint16, origin is int32.  Negative origins don't behave 
properly.

Comment 27

[Peter Lubczynski]

Ooops, can't use NPRect. Okay, how about this:

* I pass you an NPWindow* in lParam through HandleEvent
* I cache a nsPluginWindow, only calling you when changed (not on every paint)
* I intersect your pluign window rect with the visible rect to get right clipping

Comment 28

[Jaime Rodriguez, Jr.]

Changing this to [ADT1 RTM]. Let's get it on the trunk, and let it bake, then
take it for RTM.

Comment 29

[Nicholas B]

Just so you know, we need this fixed asap, meaning this week.
Jaime: I will talk to you about this issue offline.

Comment 30

[Peter Lubczynski]

Attachment: patch v.3

Here's a better patch, however I think we are incorrectly calculating the
visible width. I'll try to ask the layout folks on how to get the right values.
Let me know if there are any other problems.

Comment 31

[Ari Berger]

Thanks Peter- I will try this.
If there is time preasure to get this checked in, the width is not a big deal.  
I noticed that we seem to get the wrong width on IE in many cases- that is the 
most minor piece.  You have to know what you are looking for to see something 
is wrong.  The key is having the right origin- with that we can calculate 
offsets properly.  Since we only need this for mousing and not drawing, the 
width isn't crucial.
The other 2 (especially the nprect vs npwindow) are the important part (for us 
at least- it would obviously be good to get this right in general).

Comment 32

[Ari Berger]

Looks great! I tried tables, divs, scrolling- it all looks good.  The height 
and width are wrong, but those are pretty unimportant.

Comment 33

[Peter Lubczynski]

Attachment: patch v.4

Updated patch: Now I'm getting the bounds of the root widget as my intersected
visible area. 

Ari, let me know if this patch fixes the width/height problems.

Comment 34

[Ari Berger]

Height and width look perfect.

Comment 35

[Peter Lubczynski]

Attachment: patch v.5

New patch addressing Andrei's comments that we should be passing a WINDOWPOS
struct instead of an NPWindow. Ari, is that going to work for you?

Comment 36

[Ari Berger]

Sure- anything with an x, y, height, and width will work fine.
Let's get this checked in.

Comment 37

[av (gone)]

Comment on attachment 81908 [details] [diff] [review]
patch v.5

Peter, did you mean instead of 

+		     winpos.cx = NS_STATIC_CAST(PRUint32,
mWindowlessWindow.width);
+		     winpos.cy = NS_STATIC_CAST(PRUint32,
mWindowlessWindow.height);

to write

+		     winpos.cx = NS_STATIC_CAST(PRInt32,
mWindowlessWindow.width);
+		     winpos.cy = NS_STATIC_CAST(PRInt32,
mWindowlessWindow.height);

It is |int| in the Windows struct and |PRUint32| in our nsPluginWindow struct.
If I got it right and you fix it, r=av. We can probably also fill the hwnd
field of this struct.

Comment 38

[Peter Lubczynski]

Attachment: patch v.6

Oh right, thanks Andrei. Here's an updated patch. I added a memset but could
not fill in hwnd because, naturally, windowless plugins don't get a
widget/window.

Comment 39

[av (gone)]

Comment on attachment 81948 [details] [diff] [review]
patch v.6

r=av

Comment 40

[Marc Attinasi]

Comment on attachment 81948 [details] [diff] [review]
patch v.6

sr=attinasi

Comment 41

[Peter Lubczynski]

Attachment: patch v.7

New patch: No need to cache a whole nsPluginWindow if all we need is what an
nsRect can provide. Also gets rid of compiler warnings and the NS_STATIC_CAST.

Comment 42

[Peter Lubczynski]

Attachment: patch 7.1

Comment 43

[Peter Lubczynski]

Comment on attachment 82057 [details] [diff] [review]
patch 7.1

Talked with Marc, copying reviews from previous patch.

Comment 44

[Ari Berger]

Does this have the problems mentioned in comment 26- NPRect is a set of uints, 
we need ints?

Comment 45

[av (gone)]

|nsRect| unlike |NPRect| has it all |int32|, so it should be fine.

Comment 46

[Peter Lubczynski]

Patch in trunk, marking FIXED and nominating adt1.0.0.

Comment 47

[Jaime Rodriguez, Jr.]

Let's get this one in after RC2. adt1.0.0- [adt1 RTM]

Comment 48

[Ari Berger]

I've tried the nightly trunk build and it is working great.  Our QA has started 
looking at it and has found no problems.  We'd love to see this in the branch 
as soon as possible- without this bug fixed, our plugin doesn't work properly.

Comment 49

[Judson Valeski]

*** Bug 143142 has been marked as a duplicate of this bug. ***

Comment 50

[shrirang khanzode]

looks ok on trunk 0509.

Comment 51

[rubydoo123]

Justification for the branch:
Viewpoint is a windowless plugin and is important to embedding. This patch only
touches windowless plugins. The way we paint has changed (we recreate drawing
surfaces dynamically) and this fix is important so that ALL windowless plugins
know where they are on the screen. Otherwise the plug-in content will draw
incorrectly, and in many cases will overlay the window controls.

Comment 52

[scottputterman]

Adding adt1.0.0+ for 1.0 branch checkin approval.  Please get drivers approval
before checking in.

Comment 53

[Terri Preston]

This is fixed on Win XP trunk build 2002051408

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 82057 [details] [diff] [review]
patch 7.1

a=[e-mail] for branch checkin under the assurance from valeski that the
fact that the patch basically only touches XP_WIN is ok - no other platforms
need this fix.

Comment 55

[Peter Lubczynski]

fix in branch

Comment 56

[Peter Lubczynski]

Ari,

Is there a new version of your plugin that uses this new event that our QA can
grab to test and verify this bug with?

Thanks!

Comment 57

[Ari Berger]

I'll toss one up on our server for testing purposes first thing Monday morning.

Comment 58

[Christine Wozniak]

Adding custrtm-; no impact on customization.

Comment 59

[shrirang khanzode]

has been long verified..needed to add kwd.