552163 - [OOPP]Can not start scroll page by mouse wheel when mouse cursor is over a flash video.

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100312 Minefield/3.7a3pre ID:20100312081036
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100312 Minefield/3.7a3pre ID:20100312081036

If enable OOPP(by default), I can not start scroll page by mouse wheel when mouse cursor is over a flash video.

If disabled OOPP(set dom.ipc.plugins.enabled to false),then I can start scroll page by mouse wheel even if mouse cursor is over a flash video.

Steps to Reproduce:
1. Start Minefield with new profile
2. Open URL youtube ( http://www.youtube.com/watch?v=GWykLTv3cRA )
3. Hover mouse cursor over a flash video.
4. Scroll by mouse wheel

Actual Results:
 I can not start scroll page by mouse wheel

Expected Results:
 I can start scroll page by mouse wheel

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Don't touch. This is going to be fixed by the patch of bug 483136 (same level as Fx3.5).

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Benjamin:

I recommend this bug to block the Lorentz final release (not b1). This is going to be fixed by bug 483136. But it also changes some behavior of mouse wheel event model. So, I'm not sure whether all of the patch of bug 483136 should be landed to Lorentz or not.

Basically, such changes shouldn't be landed on any branches, however, OOPP is bigger change, so, I guess that we can take all of the fix.

Comment 3

[Benjamin Smedberg]

Masayuki, is this a regression? It seems from this bug that it is a regression, but bug 483136 seems to imply that this is a longstanding bug.

In either case, I'm pretty reluctant to take event dispatch changes on a stable branch without being absolutely sure of them.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #3)
> Masayuki, is this a regression? It seems from this bug that it is a regression,
> but bug 483136 seems to imply that this is a longstanding bug.
> 
> In either case, I'm pretty reluctant to take event dispatch changes on a stable
> branch without being absolutely sure of them.

In some cases, plug-ins always consume the mouse wheel messages even if it's not actually handled (depends on the plug-in's instance). That is a long standing bug. But in OOPP, we don't process any mouse wheel messages when the mouse cursor is on the plug-in window because it is another process's window. This is the regression of OOPP.

The patch of bug 483136 is trying to fix both bug. Of course, I can port a part of the patch only for this bug. But it will be a new patch, so, only the branch testers can test it.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I recommend this bug should be nominated. See comment 2 - comment 4.

Comment 6

[Benjamin Smedberg]

I do not think this needs to block. It seems like a minor regression to me. I would happily take a patch that was appropriately small-risk.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #6)
> I do not think this needs to block. It seems like a minor regression to me. I
> would happily take a patch that was appropriately small-risk.

I don't think that the regression isn't minor because all Windows' users cannot scroll web pages by turning mouse wheel on plug-ins. E.g., when a plug-in scrolled to under mouse cursor, the scrolling is stopped and the user needs to move the cursor outside the plug-in.

The patch of bug 483136 is pretty big, so, if you don't think that we shouldn't take the patch on branch, I can agree. However, even if we don't take the patch on branch, I should create the smallest patch only for this bug. And this bug should be fixed on branch too.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I don't think that the regression isn't minor

I think that...

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Masayuki: is there something about OOPP which makes bug 483136 worse? If not, is there any reason why you think we need to block Firefox 3.6.4 on this if it's no worse than in Firefox 3.5?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Essentially I'm asking: is this a straight duplicate of bug 4831136 or is there something about OOPP that makes it worse / more commonly felt.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is not a dup of bug 483136. Current code aborts the process of the mouse wheel messages if the cursor is on another process's window. The old plug-in window is on same process but OOPP's window is on another process. This is the cause of this bug.

Bug 483136 changes mouse wheel handling process. Therefore, it *includes* a fix of this bug. So, if we need to fix this bug only on branch, I need to create a smaller patch but I need to arrange the part for current mouse wheel message handling code. In other words, I need to create a new patch by another approach for this bug. I think the patch is smaller (I guess, maybe about 20-30 lines will be added and about 10 lines will be removed). I'll write the ported patch.

Comment 12

[Benjamin Smedberg]

Codefreeze is Monday and this sounds risky enough that we want to let it bake first!

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #12)
> Codefreeze is Monday and this sounds risky enough that we want to let it bake
> first!

Yes. Therefore, I asked you a week ago. Bug 483136 patch was blocked by a couple of hang up bugs of OOPP. So, the coming patch has same risk.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Porting from the patch of bug 483136 for 1.9.2 branch.

I pushed this to tryserver.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 437708 [details] [diff] [review]
Patch v1.0

Jimm, would you review this? This is ported from bug 483136's patch.

We abort the wheel message handling when the cursor is on another process's window. If we find another process window and it's ancestor is our process's window, that should be a plug-in's window. Then, we should send the wheel messages to the plug-in window whose class name is "GeckoPluginWindow".

::ReplyMessage() are needed for bug 547136.

# I'm confused, I thought you reviewed the patch of bug 483136, but not so. I'll request review in the bug after this.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

As of now I think this blocks; if the cure is worse than the disease, we may need to reconsider.

Masayuki: thanks for writing the smaller patch for branch so quickly.

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 437708 [details] [diff] [review]
Patch v1.0

   // We don't care about windows belonging to other processes.
+  if (!IsOurProcessWindow(destWnd)) {
+    destWnd = FindOurProcessWindow(destWnd);
+    if (!destWnd) {
+      // Somebody elses window
+      return PR_FALSE; // break, but continue processing
+    }

Can you update this with a better comment explaining why we walk up the parent chain checking thread ids?

+#ifdef MOZ_IPC
+        if (destWindow && destWindow->mWindowType == eWindowType_plugin) {
+          // When OOPP is enabled, we need to send the message to the child
+          // window whose class name is "GeckoPluginWindow".
+          HWND pluginWnd =
+            ::FindWindowExW(destWnd, NULL, L"GeckoPluginWindow", NULL);
+          if (pluginWnd) {
+            destWnd = pluginWnd;
+          }
+          ::ReplyMessage(aMsg == WM_MOUSEHWHEEL ? TRUE : 0);
+        }
+#endif
         if (0 == ::SendMessageW(destWnd, aMsg, aWParam, aLParam))

I'm concerned about this. You're sending a synchronous event to the child window from parent which could result in an ipc incall on parent from the child. The parent thread is in SendMessage waiting on the response, so there's risk here of a deadlock. Maybe we need to add a ReplyMessage in the subclass of PluginInstanceChild for this event, or possibly post it instead?

Comment 18

[Jim Mathies [:jimm]]

+          ::ReplyMessage(aMsg == WM_MOUSEHWHEEL ? TRUE : 0);

Also, what caller are you unblocking here?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

(In reply to comment #17)
> (From update of attachment 437708 [details] [diff] [review])
> +#ifdef MOZ_IPC
> +        if (destWindow && destWindow->mWindowType == eWindowType_plugin) {
> +          // When OOPP is enabled, we need to send the message to the child
> +          // window whose class name is "GeckoPluginWindow".
> +          HWND pluginWnd =
> +            ::FindWindowExW(destWnd, NULL, L"GeckoPluginWindow", NULL);
> +          if (pluginWnd) {
> +            destWnd = pluginWnd;
> +          }
> +          ::ReplyMessage(aMsg == WM_MOUSEHWHEEL ? TRUE : 0);
> +        }
> +#endif
>          if (0 == ::SendMessageW(destWnd, aMsg, aWParam, aLParam))
> 
> I'm concerned about this. You're sending a synchronous event to the child
> window from parent which could result in an ipc incall on parent from the
> child. The parent thread is in SendMessage waiting on the response, so there's
> risk here of a deadlock. Maybe we need to add a ReplyMessage in the subclass of
> PluginInstanceChild for this event, or possibly post it instead?

Post cannot be used because we need to handle next wheel message if plug-in doesn't consume it. Another wheel message shouldn't be handled before it.

(In reply to comment #18)
> +          ::ReplyMessage(aMsg == WM_MOUSEHWHEEL ? TRUE : 0);
> 
> Also, what caller are you unblocking here?

Good point. I find a bug. The deadlock is caused by I send the message to a plug-in which *was* already handled. So, it handled twice per a message.

When the destWindow which is found from the cursor position is handling the message, it means the message came from its child plug-in window except that the message was sent to the window directly. Even if the message was sent to the plug-in window directly, the message was already handled by plug-in. So, in this case, the widget should redirect the message to its parent window rather than that it resend the message to the plug-in window.

However, I think that we should call ReplyMessage() here because it's not risky, but it prevents the deadlock bug if other part has some bugs. Therefore, this patch still calls it.

I changed two logic:

> -        if (0 == ::SendMessageW(destWnd, aMsg, aWParam, aLParam))
> -          aHandled = PR_TRUE;
> +        ::SendMessageW(destWnd, aMsg, aWParam, aLParam);
>          sIsProcessing = PR_FALSE;
> -        return PR_FALSE; // break, but continue processing
> +        aHandled = PR_TRUE;
> +        aQuitProcessing = PR_TRUE;
> +        return PR_FALSE; // break, and stop processing

The SendMessage() result cannot be trusted, so, we should ignore it. And it should always consume the message.

> +  // We don't handle the message if the found window belongs to another
> +  // process's top window.  If it belongs window, that is a plug-in's window.
> +  // Then, we need to send the message to the plug-in window.
> +  if (!IsOurProcessWindow(destWnd)) {
> +    HWND ourPluginWnd = FindOurProcessWindow(destWnd);
> +    if (!ourPluginWnd) {
> +      // Somebody elses window
> +      return PR_FALSE; // break, but continue processing
> +    }
> +    destWindow = GetNSWindowPtr(ourPluginWnd);
> +  } else {
> +    destWindow = GetNSWindowPtr(destWnd);
> +  }

The old patch sent the message to ourPluginWnd, but it created a regression. If the destWnd which is under the cursor is created by plug-in (e.g., Adobe reader), we cannot scroll any widgets on the plug-in when the plug-in lost focus. This patch sends the message to destWnd always.

Comment 20

[Jim Mathies [:jimm]]

Masayuki, I think you posted the wrong patch. :)

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

oops, sorry.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jim: Would you review this quickly? This should be landed tomorrow. This is a blocker of 1.9.2.4.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

oops,
s/tomorrow/today

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Jim.

I landed the patch to 1.9.2 branch.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1c16bdc50547

I'll land the patch to trunk after I checked the patch on trunk and I'll close this bug.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

landed on trunk too.

http://hg.mozilla.org/mozilla-central/rev/3bcd237e52a6

Comment 26

[juan becerra [:juanb]]

Verified on a Windows 7 (32bit) machine using:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.4pre) Gecko/20100412 Namoroka/3.6.4pre
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100412 Minefield/3.7a5pre

I can scroll using the mouse wheel while the cursor hovers over the Flash video.

Comment 27

[juan becerra [:juanb]]

Is this not supposed to work on certain videos? I've just noticed that when I play http://www.youtube.com/watch?v=Cklb7L0OA1c the mouse wheel scrolling does not work when I hover over the Flash content. It works fine with other videos.

I'll let someone else comment before removing verified keywords and such.

Comment 28

[juan becerra [:juanb]]

Retried the video on the nightlies from comment #26 as well as on tonight's nightlies and on that particular video the mouse scroll wheel does not scroll the page when hovering over it.

I'm not sure what is the difference between this video and others, yet.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Juan: are you trying in a VM? There may be some difference between how mouse events are translated.

Comment 30

[juan becerra [:juanb]]

I used the same hardware machine on which I ran the nightly builds that I used to verify the bug. I just happened to be in the mood for that particular song, and that's when I noticed the problem. With the majority of the videos the mouse scrolling works.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

because the plug-in consumes the wheel messages. that is bug 483136.

this bug fixes only the OOPP's regression. check the URL with 3.6.3 or 3.5.x. you cannot scroll on the video non-OOPP builds too.

-> FIXED

Comment 32

[juan becerra [:juanb]]

Got it.

Just for reference http://www.youtube.com/watch?v=6gq2U0tgmrQ also shows the problem. Marking verified per comment #31