418645 - Visible Region Notify message does not work when scrolling client window

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[KO Myung-Hun]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8.1.12) Gecko/20080202 SeaMonkey/1.1.8
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8.1.12) Gecko/20080202 SeaMonkey/1.1.8

When scrolling client window including plugin window, Visible Region Notify messages is not delivered to plugin window correctly. That is, in normal case the messages are delivered to plugin window in following order.

  WM_VRNDISABLED
  WM_MOVE
  WM_VRNENABLED

But, in case of plugin window, WM_MOVE is not wrapped by WM_VRNDISABLED and WM_VRNENABLED pair.

Or, is there a way to know when plugin window is about to be moved or client window try to scroll its area ?

Reproducible: Always

Steps to Reproduce:
1. Open html including plugin
2. Scroll up and down
3. 
Actual Results:  
WM_MOVE
WM_VRNDISABLED
WM_VRNENABLED

Expected Results:  
WM_VRNDISABLED
WM_MOVE
WM_VRNENABLED

Comment 1

[Peter Weilbacher]

Confirming that there is a problem. (I still haven't debugged this to see the messages, but I have seen that the plugin content smears onto the page outside the plugin area when scrolling.

My ignorance of PM development shows again. I looked up the WM_VRN* messages in the toolkit documentation where it is said that WM_VRNDISABLED happens when windows are resized and/or after a WinLockWindowUpdate() call. We don't have that call in our sources and we don't generate either of the WM_VRN* messages. We actually don't even handle or generate WM_MOVE, either. Should we?

Mike, you probably at some point understood the message processing. Do you have any ideas?

Comment 2

[Peter Weilbacher]

By chance I found bug 387701 which seems related to this problem. Maybe we can do something similar to the Windows patch in attachment 285811 [details] [diff] [review] to fix this problem?

Comment 3

[Peter Weilbacher]

Attachment: first try, doesn't work

OK, so this is the Windows patch adapted for OS/2. Unfortunately, it doesn't work as the MPlayer plugin window also returns a class name of "MozillaWindowClass". And I don't know what else could be used to detect if a window is ours or a plugins'.

But even if I remove the classname check and just repaint all child windows it still spills the movie over onto the surrounding page when scrolling. So back to the start...

Comment 4

[Peter Weilbacher]

(In reply to comment #3)
> work as the MPlayer plugin window also returns a class name of
> "MozillaWindowClass".

Stupid me, WinBeginEnumWindows only enumerates the direct children and the MPlayer window is two levels further down. Because when I do it recursively I get
   classname(child=0x8000021a)=MozillaWindowClass
   classname(child=0x80000258)=#1
   classname(child=0x8000025a)=WC_MPLAYER
But it still doesn't work. Just forcing a repaint of the whole window works but is of course horribly slow. We probably need to find a way to repaint the Mozilla window at the position where the plugin was before the scroll. That could get more complicated...

Could the reason for this bug be our use of SW_INVALIDATERGN? While browsing the toolkit docs I found this about it:
   The invalid region created as a result of the scroll is added to the
   update regions of those windows affected. This may result in sending
   WM_PAINT messages to CS_SYNCPAINT windows before the call returns.
The second sentence could mean some out-of-order messages as described in comment 0...

Comment 5

[KO Myung-Hun]

Hi/2.

How about WinBroadcastMsg() as the following

    WinBroadcastMsg( hwndPlugin, WM_VRNDISABLED, 0, 0, BMSG_SEND ) 

before about to scroll ?

Here, hwndPlugin is the window handle of plugin(MozillaWindowClass) not MPlayer(WC_MPLAYER).

KO Myung-Hun

(In reply to comment #4)
> (In reply to comment #3)
> > work as the MPlayer plugin window also returns a class name of
> > "MozillaWindowClass".
> 
> Stupid me, WinBeginEnumWindows only enumerates the direct children and the
> MPlayer window is two levels further down. Because when I do it recursively I
> get
>    classname(child=0x8000021a)=MozillaWindowClass
>    classname(child=0x80000258)=#1
>    classname(child=0x8000025a)=WC_MPLAYER
> But it still doesn't work. Just forcing a repaint of the whole window works but
> is of course horribly slow. We probably need to find a way to repaint the
> Mozilla window at the position where the plugin was before the scroll. That
> could get more complicated...
> 
> Could the reason for this bug be our use of SW_INVALIDATERGN? While browsing
> the toolkit docs I found this about it:
>    The invalid region created as a result of the scroll is added to the
>    update regions of those windows affected. This may result in sending
>    WM_PAINT messages to CS_SYNCPAINT windows before the call returns.
> The second sentence could mean some out-of-order messages as described in
> comment 0...
> 

Comment 6

[Peter Weilbacher]

(In reply to comment #5)
> How about WinBroadcastMsg() as the following
> 
>     WinBroadcastMsg( hwndPlugin, WM_VRNDISABLED, 0, 0, BMSG_SEND ) 
> 
> before about to scroll ?

I had thought about something like that but didn't actually try. Wouldn't you get two WM_VRNDISABLED messages, one before WM_MOVE, one afterwards? What effects do you think this would have?

> Here, hwndPlugin is the window handle of plugin(MozillaWindowClass) not
> MPlayer(WC_MPLAYER).

So, I would need to find the WC_MPLAYER window and then twice step upwards in the window tree to find the related MozillaWindowClass window? That sounds like an ugly solution but I can give it a try.

Comment 7

[KO Myung-Hun]

Hi/2.

(In reply to comment #6)
> (In reply to comment #5)
> > How about WinBroadcastMsg() as the following
> > 
> >     WinBroadcastMsg( hwndPlugin, WM_VRNDISABLED, 0, 0, BMSG_SEND ) 
> > 
> > before about to scroll ?
> 
> I had thought about something like that but didn't actually try. Wouldn't you
> get two WM_VRNDISABLED messages, one before WM_MOVE, one afterwards? What
> effects do you think this would have?
> 

Normally, WM_VRNDISABELD before WM_MOVE and WM_VRNENABLED after WM_MOVE.

But currently, this expect is not fulfilled. So we need to generate WM_VRNDISABLED message on purpose before scrolling plugin window.

Currently, MPlayer respond to WM_MOVE as well as WM_VRNDISABELD/WM_VRNENABLED pair as a workaround. So WM_DISABLED disable the output of MPlayer, and WM_MOVE enable it, again. In this case, smearing the outside of plugin window can be prevented. That is, WM_MOVE replace WM_VRNENABLED.

> > Here, hwndPlugin is the window handle of plugin(MozillaWindowClass) not
> > MPlayer(WC_MPLAYER).
> 
> So, I would need to find the WC_MPLAYER window and then twice step upwards in
> the window tree to find the related MozillaWindowClass window? That sounds like
> an ugly solution but I can give it a try.
> 

Huh ? Mozilla does not have a method to find plugin window, that is, child window directly ? The reason I suggested to use hwndPlugin is I thought Mozilla could know the plugin handle. If so, there is no need to enumerate windows in order to find MPlayer window.

Anyway, it is no matter the receiving window is the plugin window or MPlayer window. In either case, WM_VRNDISABLED is passed to MPlayer window.

Finally, invalidating the plugin window and the foreign window is irrelevant to this problem.

KO Myung-Hun

Comment 8

[Peter Weilbacher]

(In reply to comment #7)
> Normally, WM_VRNDISABELD before WM_MOVE and WM_VRNENABLED after WM_MOVE.
> 
> But currently, this expect is not fulfilled. So we need to generate
> WM_VRNDISABLED message on purpose before scrolling plugin window.

I understand. But as the WM_VRNDISABLED that you currently get _after_ the WM_MOVE is not generated by us/Mozilla, you will then get two. The one we generate on purpose before the scroll and the one that is automatically generated somewhere else, after the scroll. I was just wondering if that was a problem for your code or not.


> Huh ? Mozilla does not have a method to find plugin window, that is, child
> window directly ?

Could be that it knows this somewhere in the plugin code which I don't know at all. In the nsWindow class where I started debugging (it is the main class to deal with the PM messages) it does not know that.

> Finally, invalidating the plugin window and the foreign window is irrelevant to
> this problem.

Not entirely irrelevant, as a forced invalidation of the whole content window after a scroll fixes the visible problem (no smearing of movie content on the webpage outside the plugin area any more). I don't know if it affects the WM_VRN* messages or not.

Comment 9

[KO Myung-Hun]

Attachment: A test build of npmp.dll

A test build of npmp.dll to check VRN messages

Comment 10

[KO Myung-Hun]

Hi/2.

(In reply to comment #8)
> (In reply to comment #7)
> > Normally, WM_VRNDISABELD before WM_MOVE and WM_VRNENABLED after WM_MOVE.
> > 
> > But currently, this expect is not fulfilled. So we need to generate
> > WM_VRNDISABLED message on purpose before scrolling plugin window.
> 
> I understand. But as the WM_VRNDISABLED that you currently get _after_ the
> WM_MOVE is not generated by us/Mozilla, you will then get two. 

I know.

> The one we
> generate on purpose before the scroll and the one that is automatically
> generated somewhere else, after the scroll. 

No. WM_VRNDISABLED is not generated whenever WM_MOVE, but after a bunch of WM_MOVE.

> I was just wondering if that was a
> problem for your code or not.
> 

No problem.

> 
> > Huh ? Mozilla does not have a method to find plugin window, that is, child
> > window directly ?
> 
> Could be that it knows this somewhere in the plugin code which I don't know at
> all. In the nsWindow class where I started debugging (it is the main class to
> deal with the PM messages) it does not know that.
> 

Ok.

> > Finally, invalidating the plugin window and the foreign window is irrelevant to
> > this problem.
> 
> Not entirely irrelevant, as a forced invalidation of the whole content window
> after a scroll fixes the visible problem (no smearing of movie content on the
> webpage outside the plugin area any more). I don't know if it affects the
> WM_VRN* messages or not.
> 

I think, this is a timing problem. The content window would repaint after the outside of the plugin window is smeared as the following.

    Blitting
    Scrolling
    Blitting cause to smear
    WM_MOVE delivered to plugin window and its child windows
    Repainting cause to clear

This is just my guess.

But you can confirm whether VRN messages wrap around WM_MOVE or not by using the attachment. I'm sorry, however, that the zip attachment cannot be downloaded as its original name. You should rename it to npmp.zip.

KO Myung-Hun

Comment 11

[Peter Weilbacher]

Attachment: simple fix as suggested by KO Myung-Hun

For some reason the debug plugin DLL doesn't output anything else than the others already did.

But anyway, your suggestion with the broadcast as in this patch seems to work well enough.

I have uploaded a test DLL to
http://sourceforge.net/project/showfiles.php?group_id=183811&package_id=213528&release_id=580725
The patched file is in the sm200802291049_wdgtos2.zip package and it should be copied over the file of the same name from the seamonkey-2.0a1pre.en-US.os2.2008022910.zip nightly build.

Comment 12

[KO Myung-Hun]

Hi/2.

(In reply to comment #11)
> Created an attachment (id=307317) [details]
> simple fix as suggested by KO Myung-Hun
> 
> For some reason the debug plugin DLL doesn't output anything else than the
> others already did.
> 

There is no WM_MOVE, even ?

> But anyway, your suggestion with the broadcast as in this patch seems to work
> well enough.
> 
> I have uploaded a test DLL to
> http://sourceforge.net/project/showfiles.php?group_id=183811&package_id=213528&release_id=580725
> The patched file is in the sm200802291049_wdgtos2.zip package and it should be
> copied over the file of the same name from the
> seamonkey-2.0a1pre.en-US.os2.2008022910.zip nightly build.
> 

Strange. It does not work here as expect, still cause to smear.

Of course, WM_VRNDISABLED is passed before WM_MOVE.

Surely, does not corruption happen to you any more ?

I scrolled the content window by keyboard or mouse wheel.

KO Myung-Hun

Comment 13

[Peter Weilbacher]

No, I don't see any WM_* messages, not even WM_MOVE. I do see them in the binary, though...

Too bad that it doesn't work for you. I also tried several different ways to scroll. I will try to create a DLL with the idea I had about invalidation. Or perhaps I should try one with WinLockWindowUpdate().

Comment 14

[Peter Weilbacher]

A difference with our setups might be the smooth scrolling preference (Edit -> Preferences -> Content -> Use smooth scrolling in a current SeaMonkey trunk build). When that is switched off the problem is more visible and I indeed then see a bit of the movie spilling over onto the webpage surrounding it.

But I tried WinLockWindowUpdate() with catastrophic effect (it hung my PM so that I had to reboot), and the invalidation trick didn't work, either. So I'm not sure what else I could try.

Now that you get WM_VRNDISABLED before WM_MOVE, isn't this more a timing problem in mplayer than in the Mozilla code? Are you sure that you can stop the movie so fast that it really doesn't blit any more to the screen before WM_MOVE happens?

Comment 15

[KO Myung-Hun]

Hi/2.

(In reply to comment #14)
> A difference with our setups might be the smooth scrolling preference (Edit ->
> Preferences -> Content -> Use smooth scrolling in a current SeaMonkey trunk
> build). When that is switched off the problem is more visible and I indeed then
> see a bit of the movie spilling over onto the webpage surrounding it.
> 
> But I tried WinLockWindowUpdate() with catastrophic effect (it hung my PM so
> that I had to reboot), and the invalidation trick didn't work, either. So I'm
> not sure what else I could try.
> 

I think, it didn't hang PM, but prevented PM from being updated.

> Now that you get WM_VRNDISABLED before WM_MOVE, isn't this more a timing
> problem in mplayer than in the Mozilla code? Are you sure that you can stop the
> movie so fast that it really doesn't blit any more to the screen before WM_MOVE
> happens?
> 

Of course, MPlayer can do it. As soon as WM_VRNDISABLED is arrived, blitting is canceled. If this does not work, screen corruption should always occur when moving movie window, but it should not.

I have another suggestion. How about using WinSendMsg() to foreign window instead of WinBroadcastMsg() ? I suspect WinBroadcastMsg() wait until window procedure return.

KO Myung-Hun

Comment 16

[KO Myung-Hun]

Hi/2.

I've noticed that Flash v7 encounter the scroll problem.

Does Flash v7 use DIVE ? Or it uses Gpi ?

KO Myung-Hun

Comment 17

[KO Myung-Hun]

(In reply to comment #16)
> Hi/2.
> 
> I've noticed that Flash v7 encounter the scroll problem.
> 

This should be "I've noticed that Flash v7 does not encounter the scroll problem".

Sorry. ^^

> Does Flash v7 use DIVE ? Or it uses Gpi ?
> 
> KO Myung-Hun
> 

Comment 18

[Peter Weilbacher]

I have no idea what Flash7 really uses. I mean, it certainly uses the Win API internally but how that is translated to PM/GPI or DIVE I have no idea.

Yes, I can try WinSendMsg() which is the function that waits for the message to be processed, see <http://www.warpspeed.com.au/cgi-bin/inf2html.cmd?..\html\book\Toolkt40\PM2.INF+2712>.

Comment 19

[KO Myung-Hun]

Hi/2.

What is the result of trying WinSendMsg() ?

KO Myung-Hun

(In reply to comment #18)
> I have no idea what Flash7 really uses. I mean, it certainly uses the Win API
> internally but how that is translated to PM/GPI or DIVE I have no idea.
> 
> Yes, I can try WinSendMsg() which is the function that waits for the message to
> be processed, see
> <http://www.warpspeed.com.au/cgi-bin/inf2html.cmd?..\html\book\Toolkt40\PM2.INF+2712>.
> 

Comment 20

[Peter Weilbacher]

There wasn't any result yet, I was away...

Comment 21

[Peter Weilbacher]

Sorry for the long delay, I'm getting more and more distracted by other things!

(In reply to comment #19)
> What is the result of trying WinSendMsg() ?

It seems to work. I uploaded a DLL for you to test as <http://temp.weilbacher.org/wdgtos2_sm200804102240_WinSendMsg.zip>. This DLL should work with the latest SeaMonkey nightly from <http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/contrib/2008-04-10-22-trunk/seamonkey-2.0a1pre.en-US.os2.zip>.

At first I was not sure if this is a good idea, in case the plugin hangs the browser would hang, too. But then if a plugin hangs the browser is doomed anyway...

Comment 22

[Peter Weilbacher]

Attachment: simple fix but using WinSendMsg()

This is the patch that I used for that DLL.

Comment 23

[KO Myung-Hun]

Hi/2.

(In reply to comment #21)
> Sorry for the long delay, I'm getting more and more distracted by other things!
> 

No problem.

> (In reply to comment #19)
> > What is the result of trying WinSendMsg() ?
> 
> It seems to work. I uploaded a DLL for you to test as
> <http://temp.weilbacher.org/wdgtos2_sm200804102240_WinSendMsg.zip>. This DLL
> should work with the latest SeaMonkey nightly from
> <http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/contrib/2008-04-10-22-trunk/seamonkey-2.0a1pre.en-US.os2.zip>.
> 

Ok. It works fine here, too.

The best is to support VRN message fully, but it's fortunate we could find a workaround.

Thanks.

> At first I was not sure if this is a good idea, in case the plugin hangs the
> browser would hang, too. But then if a plugin hangs the browser is doomed
> anyway...
> 

Good.

KO Myung-Hun

Comment 24

[Peter Weilbacher]

Comment on attachment 315402 [details] [diff] [review]
simple fix but using WinSendMsg()

Mike, does this workaround look OK to you, too?

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 315402 [details] [diff] [review]
simple fix but using WinSendMsg()

This looks ok to me.

Comment 26

[Peter Weilbacher]

OK, great. Patch checked into trunk.

Comment 27

[Peter Weilbacher]

Patch checked into 1.8 branch.

Comment 28

[Peter Weilbacher]

Attachment: HTML testcase

For the same of reproducing the problem when regressions occur (like now in bug 501618), here is a HTML testcase that shows the bug. The movie file listed in the <embed> statement has to be replaced by one that is actually present on the system (after saving the file to disk).