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

VERIFIED FIXED in mozilla1.9.3a5

Status

()

VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: alice0775, Assigned: masayuki)

Tracking

(Blocks: 1 bug, {regression, verified1.9.2, verified1.9.3})

Trunk
mozilla1.9.3a5
x86
Windows 7
regression, verified1.9.2, verified1.9.3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking1.9.2 .4+, status1.9.2 .4-fixed, status1.9.1 unaffected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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
Don't touch. This is going to be fixed by the patch of bug 483136 (same level as Fx3.5).
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Depends on: 483136
Ever confirmed: true

Updated

9 years ago
Blocks: 478976
No longer blocks: 531142
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

9 years ago
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 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.
I recommend this bug should be nominated. See comment 2 - comment 4.
blocking1.9.2: --- → ?
status1.9.1: --- → unaffected

Comment 6

9 years ago
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.
(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.
> I don't think that the regression isn't minor

I think that...
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?
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.
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

9 years ago
Codefreeze is Monday and this sounds risky enough that we want to let it bake first!
(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.
Created attachment 437708 [details] [diff] [review]
Patch v1.0

Porting from the patch of bug 483136 for 1.9.2 branch.

I pushed this to tryserver.
Keywords: regression
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.
Attachment #437708 - Flags: review?(jmathies)
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.
blocking1.9.2: ? → .4+

Comment 17

9 years ago
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

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

Also, what caller are you unblocking here?
Created attachment 438060 [details] [diff] [review]
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.
Attachment #437708 - Attachment is obsolete: true
Attachment #438060 - Flags: review?(jmathies)
Attachment #437708 - Flags: review?(jmathies)

Comment 20

9 years ago
Masayuki, I think you posted the wrong patch. :)
Created attachment 438216 [details] [diff] [review]
Patch v2.0

oops, sorry.
Attachment #438060 - Attachment is obsolete: true
Attachment #438216 - Flags: review?(jmathies)
Attachment #438060 - Flags: review?(jmathies)
Jim: Would you review this quickly? This should be landed tomorrow. This is a blocker of 1.9.2.4.
oops,
s/tomorrow/today

Updated

9 years ago
Attachment #438216 - Flags: review?(jmathies) → review+
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.
status1.9.2: --- → .4-fixed
landed on trunk too.

http://hg.mozilla.org/mozilla-central/rev/3bcd237e52a6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Blocks: 483136
No longer depends on: 483136
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.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2, verified1.9.3
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.
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.
Status: VERIFIED → REOPENED
Keywords: verified1.9.2, verified1.9.3
Resolution: FIXED → ---
Juan: are you trying in a VM? There may be some difference between how mouse events are translated.
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.
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
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Got it.

Just for reference http://www.youtube.com/watch?v=6gq2U0tgmrQ also shows the problem. Marking verified per comment #31
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2, verified1.9.3
You need to log in before you can comment on or make changes to this bug.