Closed Bug 558629 Opened 11 years ago Closed 11 years ago

[OOPP] Windows: IPC severely degrades mouse input responsiveness

Categories

(Core :: Plug-ins, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
blocking1.9.2 --- .4+
status1.9.2 --- .4-fixed

People

(Reporter: fehe, Assigned: jimm)

References

()

Details

(Keywords: perf, Whiteboard: [part 2 of fix in bug 563847])

Attachments

(1 file, 2 obsolete files)

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.
Meant to state: I'm on a 933 MHz P3.
Blocks: OOPP
Keywords: perf
Summary: IPC severely degrades mouse input responsiveness → [OOPP] IPC severely degrades mouse input responsiveness
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: IPC → Plug-ins
QA Contact: ipc → plugins
This issue also occurs for mac OOPP. I reproduce the issue by playing some flash games on kongregate.com that require mouse input.
OS: Windows XP → All
Hardware: x86 → All
blocking1.9.2: --- → ?
blocking2.0: --- → ?
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.
blocking2.0: ? → -
OS: All → Windows XP
Hardware: All → x86
Summary: [OOPP] IPC severely degrades mouse input responsiveness → [OOPP] Windows: IPC severely degrades mouse input responsiveness
What windows version are you using?
Win7.
Better to try with XP.  Maybe even 32-bit.
It's especially noticeable on slower computers. I don't think the OS matters much (at least windows wise).
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.
blocking1.9.2: ? → .4+
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?
QA Contact: plugins → bent.mozilla
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?
Yes, the VMs we use for record/replay are all single-core.
I wonder then if the problem is how the VM emulates the mouse.  Maybe you would have better success on slower real hardware.
I will try the STR in this bug on some of my lab machines and report back.
(In reply to comment #14)
> https://build.mozilla.org/tryserver-builds/bturner@mozilla.com-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?
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.
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?
Assignee: nobody → bent.mozilla
QA Contact: bent.mozilla → plugins
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.
(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
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.
Assignee: bent.mozilla → jmathies
(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.
(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.
@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?
Attached patch patch v.1 (obsolete) — Splinter Review
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 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.
(In reply to comment #27)
> Try builds - 
> 
> https://build.mozilla.org/tryserver-builds/jmathies@mozilla.com-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)
Attached patch updated (obsolete) — Splinter Review
- addressed bent's comments
- added 5 msec delay for delivery
Attachment #441168 - Attachment is obsolete: true
- addressed bsmedberg's comments
Attachment #441492 - Attachment is obsolete: true
Attachment #441504 - Flags: review+
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.
http://hg.mozilla.org/mozilla-central/rev/9cb02fce4c57
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #441504 - Flags: approval1.9.2.4?
Wait, is this actually fixed for the windowless case?
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.
Err, please do not land on mozilla-1.9.1, bad paste :/
Attachment #441504 - Flags: approval1.9.2.4? → approval1.9.2.4+
Windowless case not resolved.  Filed bug 561818
(In reply to comment #36)
> Windowless case not resolved.  Filed bug 561818

What build were you testing with IU? Latest hourly?
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.
(In reply to comment #37)
> What build were you testing with IU? Latest hourly?

Hourly build: http://hg.mozilla.org/mozilla-central/rev/9cb02fce4c57
(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?
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.
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
(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.
I thought of Cairo.  Anyway, everything seems to be working OK now with OOPP on.  Go figure.
Depends on: 561818
Gary, can you see if this looks to be fixed for you in the nightly 1.9.2 build?
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?
No longer depends on: 563847
Whiteboard: [part 2 of fix in bug 563847]
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?
You need to log in before you can comment on or make changes to this bug.