Closed Bug 1363290 Opened 3 years ago Closed 3 years ago

64-bit Flash "Mouse Lock" is broken

Categories

(Core :: Plug-ins, defect, P2)

55 Branch
x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- unaffected
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: etsai, Assigned: handyman)

References

()

Details

(Whiteboard: sb?)

Attachments

(4 files, 4 obsolete files)

Origin issue from https://webcompat.com/issues/5845

Step to reproduce:
1. Navigate to: http://www.gamemaker3d.com/editor/GM3DPlayer.html?pid=01626685
2. Click somewhere on the screen and allow go fullscreen

Expected Behavior:
The flash mouse lock feature acts correctly.

Actual Behavior:
The flash mouse lock behaves not normal and the user can't control the game.
Is this 32-bit or 64-bit Firefox?

Probably need to test with asyncdrawing enabled/disabled.
Flags: needinfo?(stefan.georgiev)
Flags: needinfo?(etsai)
I've tested the reported issue on latest Nightly build (20170413192749) and manage to reproduce it. When dom.ipc.plugins.asyncdrawing.enabled is set to true the mouse is locked and the user can`t control the game as mentioned. 
When I flip the pref the game stuck at the loading bar and I can't check how it will behave in this case. 

However the game is working correctly on Firefox release and Beta builds even with dom.ipc.plugins.asyncdrawing.enabled = true.
Flags: needinfo?(stefan.georgiev)
I wasn't able to reproduce the issue on latest Nightly 32-bits build. With dom.ipc.plugins.asyncdrawing.enabled = false the game loads correctly and user can control the game.
Hardware: Unspecified → x86_64
I can reproduce this using latest Nightly 64-bits build with dom.ipc.plugins.asyncdrawing.enabled = true (default)
Flags: needinfo?(etsai)
NI cpeterson for asyncdrawing tracking and jimm for engineering ownership.
Flags: needinfo?(jmathies)
Flags: needinfo?(cpeterson)
Priority: -- → P2
Flags: needinfo?(jmathies)
Jim, I suspect this misbehaving mouse lock is a 64-bit bug, unrelated to async drawing. The game's mouse lock works correctly in 32-bit Firefox regardless of whether async drawing is enabled or not.

* 32-bit Firefox with async drawing *disabled* = game works correctly
* 32-bit Firefox with async drawing *enabled* = game works correctly
* 64-bit Firefox with async drawing *disabled* = game is stuck on loading screen. (This is a common windowless issue on 64-bit that is fixed by async drawing.)
* 64-bit Firefox with async drawing *enabled* = game loads, but mouse input doesn't work when clicking the game!

Here is another Flash fullscreen test that works in 32-bit but not 64-bit. You can see that the mouse coordinates (labeled "here:") are much more sensitive to mouse input in 64-bit than 32-bit.

http://renaun.com/flash/fullscreeninteractivemouselock/
Flags: needinfo?(cpeterson) → needinfo?(jmathies)
OS: Unspecified → Windows
Summary: No "Mouse Lock" feature → 64-bit Flash "Mouse Lock" is broken
No longer blocks: flash-async-drawing
Flags: needinfo?(jmathies)
Whiteboard: sb?
This is caused by the 64-bit flash sandbox We'll triage this today.
Flags: needinfo?(davidp99)
TL;DR: The issue is caused by the sandbox blocking SetCursorPos, which Flash uses for its relative mouse positioning.  This patch series remotes the method to the main process.

----

There is no simple way to know that this method is the cause of the bug -- there is no log of sandboxed methods or anything.  I ran this in API Monitor 64 and watched the system calls to see what Adobe was doing.  You can see there that mouse cursor positions returned by GetCursorPos are all over the place when the sandbox is on but they are pretty localized when its not.  But the patch works so...

Part 1 is a refactor of a class that is currently used to remote the GetKeyState method.  The refactor is used in part 3 to remote GetCursorPos as well.
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Attachment #8868309 - Flags: review?(bobowencode)
movxsd is used to copy from register to register, sign extending the value.  Its used by SetCursorPos.
Attachment #8868313 - Flags: review?(aklotz)
Run SetCursorPos on the main process.  It would be better if this weren't so open but I'm not sure how to do that.  The best idea I had was to limit it so that SetCursorPos only ran in the parent if the plugin were fullscreen, but I don't see how to know...?
Attachment #8868315 - Flags: review?(bobowencode)
This came up when I was hooking another method while I tracked down the cause of the bug.  This isn't needed for SetCursorPos (IIRC it was for GetCursorPos).  But its simple enough that I figured I'd throw it in.  This just extends our current mov r32, imm32 handler since it only recognized the eax register (the 3 low bits -- 0xb8-0xbf).

http://sparksandflames.com/files/x86InstructionChart.html
Attachment #8868317 - Flags: review?(aklotz)
Comment on attachment 8868315 [details] [diff] [review]
3. Proxy win32's SetCursorPos for plugins in chrome process

This is all NPAPI, so redirecting to jimm.
(sync-messages.ini will need an IPC peer as well.)
Attachment #8868315 - Flags: review?(bobowencode) → review?(jmathies)
Attachment #8868309 - Flags: review?(bobowencode) → review?(jmathies)
Attachment #8868309 - Flags: review?(jmathies) → review+
Comment on attachment 8868315 [details] [diff] [review]
3. Proxy win32's SetCursorPos for plugins in chrome process

Review of attachment 8868315 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2292,5 @@
>                                   (void**) &sGetKeyStatePtrStub);
>      }
>  
> +    if (!sSetCursorPosPtrStub) {
> +      fprintf(stderr, "Hooking SetCursorPos\n");

nit - debugging printf
Attachment #8868315 - Flags: review?(jmathies) → review+
Hey Paul, this patch gives the plugin process the ability to set the current cursor position on windows. We need this to fix a flash api that's broken. Wanted to be sure you were aware of it. From a security perspective I'd prefer not allowing this but I don't see a way around it.
Flags: needinfo?(ptheriault)
> nit - debugging printf

Doh.  Fixed.
Attachment #8868315 - Attachment is obsolete: true
Comment on attachment 8868313 [details] [diff] [review]
2. Add movxsd instruction to WindowsDllInterceptor

David says he has cycles for these.
Attachment #8868313 - Flags: review?(aklotz) → review?(dmajor)
Attachment #8868317 - Flags: review?(aklotz) → review?(dmajor)
Comment on attachment 8868313 [details] [diff] [review]
2. Add movxsd instruction to WindowsDllInterceptor

It doesn't seem to be two bytes in all cases. I tried some random values for the byte following 0x63:

0:000> eb user32!setcursorpos+2 0c; u user32!setcursorpos L1
user32!SetCursorPos:
00000001`80029990 48630c41        movsxd  rcx,dword ptr [rcx+rax*2]

0:000> eb user32!setcursorpos+2 0d; u user32!setcursorpos L1
user32!SetCursorPos:
00000001`80029990 48630d41b89300  movsxd  rcx,dword ptr [00000001`809651d8]

I also took a peek at the TestDllInterceptor change in the next patch since it's logically together with this. You might want to test the return value of patchedSetCursorPos there.
Attachment #8868313 - Flags: review?(dmajor)
Attachment #8868317 - Flags: review?(dmajor) → review+
Beta 54 is affected now that we re-enabled Flash async drawing for 64-bit Firefox in bug 1363876.
(In reply to David Major [:dmajor] from comment #18)
> Comment on attachment 8868313 [details] [diff] [review]
> 2. Add movxsd instruction to WindowsDllInterceptor
> 
> It doesn't seem to be two bytes in all cases.

Yup.  Added ModR/M register-mode check.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea30e232515cc9e64ae74aa271d55900c8f700b2
Attachment #8868313 - Attachment is obsolete: true
Attachment #8870206 - Flags: review?(dmajor)
Comment on attachment 8868678 [details] [diff] [review]
3. Proxy win32's SetCursorPos for plugins in chrome process (r=jimm)

Jed, can you IPC-review this sync message addition to the plugin process, added to remote an API blocked by the sandbox?
Attachment #8868678 - Flags: review?(jld)
Comment on attachment 8870206 [details] [diff] [review]
2. Add movxsd instruction to WindowsDllInterceptor

Review of attachment 8870206 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I checked with:
> .for (r @$t0=0xc0; @$t0 <= 0xff; r @$t0=@$t0+1;) { eb user32!setcursorpos+2 @$t0; u user32!setcursorpos L1 }
Attachment #8870206 - Flags: review?(dmajor) → review+
Comment on attachment 8868678 [details] [diff] [review]
3. Proxy win32's SetCursorPos for plugins in chrome process (r=jimm)

Review of attachment 8868678 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.  I know we try to avoid sync IPC, but it's necessary for intercepting a sync API like this.
Attachment #8868678 - Flags: review?(jld) → review+
I took your advice from comment 18 and ran with it -- I'm now maintaining the cursor position as well as checking the return error code.  I was concerned about the API potentially being blocked on these tests in automation but its fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9163012df662142796d5b03c504cb9a0fc3f048&selectedJob=101082579

TestSetCursorPos is the only change.  Look good to you?
Attachment #8868678 - Attachment is obsolete: true
Attachment #8870240 - Flags: feedback?(dmajor)
Comment on attachment 8870240 [details] [diff] [review]
3. Proxy win32's SetCursorPos for plugins in chrome process (r=jimm,jld)

If it passes, sure!
Attachment #8870240 - Flags: feedback?(dmajor) → feedback+
Thanks for the heads up Jimm. I assume it can send 'click' events too?
Flags: needinfo?(ptheriault)
Keywords: checkin-needed
Part 3 needs rebasing.
Keywords: checkin-needed
Keywords: checkin-needed
I went ahead and rebased part 4 for you. In the future, you should probably double-check the other patches when you're already rebasing another one.
This is hitting the commit hook for sync-messages.ini. Needs r+ from an IPC peer to land.
Keywords: checkin-needed
That's stupid or I'm confused. I thought the rule about sync messages was only about content processes. Plugin messages are sync by API design and there's not a damn thing we can do about it.

What's the magic marking I need to bypass this hook?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(ryanvm)
Actually, I missed that Jed already did r+ Part 3 earlier, so I'll just use that to land it.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da418fade04
Part 1: Refactor PluginModuleChild's GetFileNameTask to extract PluginThreadTask. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/4147cf200069
Part 2: Add movxsd instruction to WindowsDllInterceptor. r=dmajor
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a1b8c5ed85
Part 3: Proxy win32's SetCursorPos for plugins in chrome process. r=jimm, r=jed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0402c2243506
Part 4: Extend DLL interceptor to handle mov r32, imm32. r=dmajor
I guess we could make the hook apply only to protocols under PContent, but I don't think people change plugin code enough for it to be worth it.
Flags: needinfo?(wmccloskey)
Depends on: 1367806
Given the size of this fix and that we won't be making 64-bit Firefox the installer default until 55, we probably don't want to uplift this fix to Beta 54.
You need to log in before you can comment on or make changes to this bug.