558629 - [OOPP] Windows: IPC severely degrades mouse input responsiveness

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[IU]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100410 BetterPrivacy-1.47 Minefield/3.7a5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100410 Minefield/3.7a5pre ID:20100410180554

With IPC plugins enabled, responsiveness to mouse input degrades severely.  When IPC plugins is disabled, there is no issue. I compared with Google Chrome 5 and Chrome is very responsive (does not suffer from this issue).

With Flash Player 10.1 installed, this degradation in responsiveness can also be observed when trying to pause a video and having it take several seconds before the video is paused.  My STR focuses on the scroll wheel, but the issues should have the same origin.

To demonstrate the issue, my STR requires installation of a program called Volumouse.  It allows for audio volume control using shortcut key(s) + the mouse wheel.  This is for illustration purposes only, as I know of no other easy way to show the performance issue. Note: I'm on a 933 MHz; thus, the relative impact of the degradation is really bad.


Reproducible: Always

Steps to Reproduce:
1. Download and install Volumouse (use the self-install executable): http://www.nirsoft.net/utils/volumouse.html
2. Start Volumouse and configure it as follows:
a) Use the wheel when: Ctrl+Shift are down
d) Component: Play Control; Channels: All Channels; Steps: 2000
c) Set all the other "Use the wheel when:" options to "Disabled"
d) Run Volumouse application in high priority
e) OK
3. Create a new profile and launch Minefield with it
4. Disable IPC plugins and restart
5. Visit the linked URL
6. Once the advertisement finishes playing and the "View" video is playing, use the hotkey from Step 2a and your mouse wheel to scroll volume up and down.  Take note of the responsiveness
7. Close the tab with the video loaded or click the home button
8. Reenable IPC plugins and restart
9. Visit the linked URL
10. Again, once the advertisement finishes playing and the video is playing, use the hotkey from Step 2a and your mouse wheel to change volume up and down.  Note the relative degradation in responsiveness.

Comment 1

[IU]

Meant to state: I'm on a 933 MHz P3.

Comment 2

[Benoit Girard (:BenWa)]

This issue also occurs for mac OOPP. I reproduce the issue by playing some flash games on kongregate.com that require mouse input.

Comment 3

[Benjamin Smedberg]

Can we keep the mac and windows bugs separate, unless there's clear evidence that they are the same bug? I can't reproduce this on Windows, even in a resource-constrained VM. If there's an obvious mac bug, let's deal with that separately.

Comment 4

[IU]

What windows version are you using?

Comment 5

[Benjamin Smedberg]

Win7.

Comment 6

[IU]

Better to try with XP.  Maybe even 32-bit.

Comment 7

[Martijn Wargers (dead)]

It's especially noticeable on slower computers. I don't think the OS matters much (at least windows wise).

Comment 8

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

This needs to be assigned - Benjamin, can you suggest someone?

Degrading mouse responsiveness would be a showstopper (flash games are pretty popular) so blocking on this.

Comment 9

[Benjamin Smedberg]

It's pretty clear that it's not a generic issue. None of my developers have been able to reproduce it, so it's hard to assign. We could try reducing the priority of the plugin process a notch in a tryserver build and IU can try that.

bent, can you do a tryserver run with that idea?

Comment 10

[IU]

Have the devs tried on a single core system or on a multi-core system with all Firefox processes forced to use only one core?

Comment 11

[Benjamin Smedberg]

Yes, the VMs we use for record/replay are all single-core.

Comment 12

[IU]

I wonder then if the problem is how the VM emulates the mouse.  Maybe you would have better success on slower real hardware.

Comment 13

[Marcia Knous [:marcia]]

I will try the STR in this bug on some of my lab machines and report back.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://build.mozilla.org/tryserver-builds/[e-mail]-lowpriorityplugin/ has a windows build you could try.

Comment 15

[IU]

(In reply to comment #14)
> https://build.mozilla.org/tryserver-builds/[e-mail]-lowpriorityplugin/
> has a windows build you could try.

That's definitely not the solution.  Lowering the priority actually made things much worse.

It you can get your hands on a slower single-core system with XP (no VMs), that would probably be the best option to identifying what's slowing things down.  Is there any reasonable debugging mechanism I can run to capture the difference between IPC and non-IPC mouse input?

Comment 16

[Martijn Wargers (dead)]

Indeed, that build makes things worse.
When I open http://bubblemark.com/flex.htm in 2 tabs, with 128 as # of balls, then trying to scroll in another page in a third tab doesn't work after a while anymore.

Comment 17

[IU]

Some more information that might help with isolating this:

1. If any Firefox window is focused when performing the scroll wheel steps in the STR, mouse responsive is slow.
2. If no Firefox window is focused the scroll wheel steps are fluid and unimpeded.

So if the assumption behind lowering the plugin process priority was that it was slowing down some non-Mozilla component, that is not so.

Is mouse data somehow being routed through the IPC mechanism, when it's enabled?  If so how and what overhead is involved?

Comment 18

[Benjamin Smedberg]

The ABC video here is a windowless video. Does this same problem exist for windowed videos, such as youtube?

youtube: windowed, hulu/abc: windowless

The only other thing I can think of here is that Flash is flooding the WM_USER+1 messages which we throttle in Firefox and we can't get a message in edgewise.

Comment 19

[IU]

(In reply to comment #18)
> The ABC video here is a windowless video. Does this same problem exist for
> windowed videos, such as youtube?

Yes.  Reproducible with this video: http://www.youtube.com/watch?v=iDk1TIXK90w

Comment 20

[Benjamin Smedberg]

ok, next theory. We didn't port the workaround for bug 132759 to the plugin process, because we figured Flash could starve itself as long as it didn't starve Firefox. But it may be that Flash is actually starving itself and we should implement the WM_USER + 1 throttling mechanism from that bug for both windowless and windowed plugins.

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> ok, next theory. We didn't port the workaround for bug 132759 to the plugin
> process, because we figured Flash could starve itself as long as it didn't
> starve Firefox. But it may be that Flash is actually starving itself and we
> should implement the WM_USER + 1 throttling mechanism from that bug for both
> windowless and windowed plugins.

We don't throttle windowless wm_user messages in non-oopp since they're sent to a hidden window flash creates. We only throttle with windowed, because the events hammer our event dispatching in the browser. Which leads me to believe this is caused by something else. We can test the theory on windowed though to see.

Comment 22

[IU]

(In reply to comment #20)
> ok, next theory. We didn't port the workaround for bug 132759 to the plugin
> process, because we figured Flash could starve itself as long as it didn't
> starve Firefox. But it may be that Flash is actually starving itself and we
> should implement the WM_USER + 1 throttling mechanism from that bug for both
> windowless and windowed plugins.

To my n00bish mind, this seems to make sense, based on my reading of bug 132759.  If the plugin process becomes "starved" due to Flash, it should no doubt affect Firefox, because it will slow communication between the two.  So I would imagine that the bug 132759 workaround still needs to be in the equation.

Comment 23

[IU]

@Jim:

The following might be relevant information, but I'm not sure if you'll be able to reproduce.  The following happens even in non-OOPP mode and I have long experienced the issue:

With the video I linked in comment 19, if (with my STR for the scroll wheel) I try changing the volume up and down, just after I've clicked play and the video is beginning to start, I get very slow mouse responsiveness.  After that, every time a Firefox tab is loading, the mouse is very laggy.  Basically any Firefox event now causes the laggy mouse pointer movement.  It continues this way even though the video has long been stopped/closed, for as long as that particular Firefox session is running.

While in this state, only Firefox is affected, and I can launch other programs -- even opening the same types of pages in IE or Chrome -- and there is no mouse lag.  The only remedy is to restart Firefox.

So is there a potential leak with the Firefox code that handles mouse events?  Has anyone else ever experienced this?  If not, is there something I can do to capture this event?

Comment 24

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 25

[Jim Mathies [:jimm]]

We're going to do a test here based on bsmedberg's theory. I'll have try server builds in a bit with the above patch applied.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 441168 [details] [diff] [review]
patch v.1

>+          if (mQuirks & QUIRK_FLASH_THROTTLE_WMUSER_EVENTS)
>+            SetupFlashMsgThrottle();

Nit: indent is off

>+    if (message == WM_NCDESTROY) {
>+        LRESULT res = CallWindowProc(self->mWinlessThrottleOldWndProc,
>+                                     hWnd, message, wParam, lParam);

Hm, shouldn't you save the wndproc here and then unhook before actually calling this? Seems dangerous to do much after WM_NCDESTROY.

>+    if (!wcscmp(className, L"SWFlash_PlaceholderX")) {
> ...
>+        if (oldWndProc != WinlessHiddenFlashWndProc) {
> ...
>+            return FALSE;
>+        }
>+    }
>+    return TRUE;

Don't you want to return FALSE if the class name matches regardless of whether or not you have subclassed it already?

>+PluginInstanceChild::FlashThrottleAsyncMsg::Run()

Seems like this could be easier by simply doing:

  mData = (void*)this;
  ChildAsyncCall::Run();

>+  if (mFunc) {
>+    mInstance->mPendingAsyncCalls.RemoveElement(this);

You already did this.

>+    PluginThreadChild::current()->message_loop()->PostTask(FROM_HERE, task);

I think you can just use MessageLoop::current()->PostTask. But you could certainly clean this all up and just use NPN_AsyncCall...

>@@ -204,16 +204,20 @@ private:
>         // Win32: Translate mouse input based on WM_WINDOWPOSCHANGED
>         // windowing events due to winless shared dib rendering. See
>         // WinlessHandleEvent for details.
>         QUIRK_SILVERLIGHT_WINLESS_INPUT_TRANSLATION = 1,
>         // Win32: Hook TrackPopupMenu api so that we can swap out parent
>         // hwnds. The api will fail with parents not associated with our
>         // child ui thread. See WinlessHandleEvent for details.
>         QUIRK_WINLESS_TRACKPOPUP_HOOK = 2,
>+        // Win32: Throttle flash WM_USER+1 heart beat messages to prevent
>+        // flooding chromium's dispatch loop, which can cause ipc traffic
>+        // processing lag.
>+        QUIRK_FLASH_THROTTLE_WMUSER_EVENTS = 4,

Mind changing this to 1 << 2? And then fixing the others to 1 << 1, 1 << 0? Slightly more obvious that this is a bitfield and not a simple enum.

Comment 27

[Jim Mathies [:jimm]]

Try builds - 

https://build.mozilla.org/tryserver-builds/[e-mail]-try-87215914a152/install/sea/

Comment 28

[IU]

(In reply to comment #27)
> Try builds - 
> 
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-87215914a152/install/sea/

TEST RESULTS:

1. Windowless (the ABCNews link): No improvement whatsoever
2. Windowed (the Youtube link): Huge improvement (not quite as fast as with IPC off, but pretty close)

Comment 29

[Jim Mathies [:jimm]]

Attachment: updated

- addressed bent's comments
- added 5 msec delay for delivery

Comment 30

[Jim Mathies [:jimm]]

Attachment: updated patch v.2

- addressed bsmedberg's comments

Comment 31

[Benjamin Smedberg]

Comment on attachment 441504 [details] [diff] [review]
updated patch v.2

It might be nice to refactor so that the parts of ChildAsyncCall which are specific to NPN_AsyncCall (mFunc, mClosure) are in a different class than the generic bits, but this is ok for now. File a followup.

Comment 32

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/9cb02fce4c57

Comment 33

[Emanuel Hoogeveen [:ehoogeveen]]

Wait, is this actually fixed for the windowless case?

Comment 34

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

Comment on attachment 441504 [details] [diff] [review]
updated patch v.2

Is this the same as bug 561049?

This gets a=beltzner, please land on mozilla-1.9.1 default, mozilla 1.9.2 default AND mozilla-1.9.2 GECKO1924_20100413_RELBRANCH

If this isn't fixed for the windowless case, please file a follow-up bug.

Comment 35

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

Err, please do not land on mozilla-1.9.1, bad paste :/

Comment 36

[IU]

Windowless case not resolved.  Filed bug 561818

Comment 37

[Jim Mathies [:jimm]]

(In reply to comment #36)
> Windowless case not resolved.  Filed bug 561818

What build were you testing with IU? Latest hourly?

Comment 38

[Gary [:streetwolf52]]

Not sure this bug fix is to blame but if you go to msnbc.com or cnn.com some of the pictures/videos don't display right away.  Some do if you mouse over them .

Only happens with OOPP enabled.

Comment 39

[IU]

(In reply to comment #37)
> What build were you testing with IU? Latest hourly?

Hourly build: http://hg.mozilla.org/mozilla-central/rev/9cb02fce4c57

Comment 40

[Jim Mathies [:jimm]]

(In reply to comment #38)
> Not sure this bug fix is to blame but if you go to msnbc.com or cnn.com some of
> the pictures/videos don't display right away.  Some do if you mouse over them .
> 
> Only happens with OOPP enabled.

Were you testing with an hourly Gary? (This just landed on mozilla-central about an hour ago.)

Regardless, can you provide more specifics on what to look for so we can try to reproduce?

Comment 41

[Gary [:streetwolf52]]

I am using the latest hourly.  go to msnbc.com and check out the picture on the upper left of the chimp.  It is sometimes just an empty box for a few seconds then sometimes fills in.  If it doesn't, mousing over it or scrolling will bring it back.  Same thing on CNN, some of the pictures that are flash videos when you click on them also come up as an empty box.  This is with OOPP enabled and also dw/d2d enabled.

Comment 42

[Gary [:streetwolf52]]

using changeset http://hg.mozilla.org/mozilla-central/rev/9cb02fce4c57

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre Firefox/3.6 - Build ID: 20100426095541

Comment 43

[Jim Mathies [:jimm]]

(In reply to comment #42)
> using changeset http://hg.mozilla.org/mozilla-central/rev/9cb02fce4c57
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426
> Minefield/3.7a5pre Firefox/3.6 - Build ID: 20100426095541

That *could* be the ciaro landing that preceded it, there's already some other fallout from that.

Comment 44

[Gary [:streetwolf52]]

I thought of Cairo.  Anyway, everything seems to be working OK now with OOPP on.  Go figure.

Comment 45

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e793e52b99e
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf9c51dc2eeb

Comment 46

[Al Billings [:abillings - ex-MoCo]]

Gary, can you see if this looks to be fixed for you in the nightly 1.9.2 build?

Comment 47

[Benjamin Smedberg]

Jim, the matching Chromium code uses RealGetWindowClass:

http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/plugins/webplugin_delegate_impl.cc?annotate=9873&pathrev=9873#l624

Is there a reason you used GetClassNameW instead, and are they always going to return the same results?

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Marcia, can you try the STR on a machine with a mouse and scrollwheel and see if you can verify it fixed on build 4 of 3.6.4?