Earle Castledine Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141020030200

Steps to reproduce:

Run either the Pointer Lock Demo (http://mdn.github.io/pointer-lock-demo/ - linked from the MDN PointerLock API page), or the Pointer Lock Controls example from Three.js (http://threejs.org/examples/#misc_controls_pointerlock). Tested in Firefox Nightly, on a Mac.


Actual results:

The controls behave very erratically: tiny movements of the mouse result in huge movements on screen. If I log the values, then the mozMovementX and mozMovementY values are very large for small movements. 


Expected results:

mozMovementX and mozMovementY report the difference between movements: and the pointer/camera moves in a natural way.

	Paul Silaghi, QA [:pauly]
	Comment 1 • 10 years ago

Looks fine in 36.0a1 (2014-10-27) Win 7 x64

	Jukka Jylänki
	Comment 2 • 10 years ago

I can verify this when e10s is active. Modified STR:

1. Get the latest Firefox Nightly channel (tested on 36.0a1 (2014-10-28) on Windows 7)
2. Navigate to about::support and click "Refresh Nightly.." to reset all prefs.
3. After the browser restarts, choose "Enable and Restart" on the "Would you like to help test e10s dialog?".
4. Navigate to https://dl.dropboxusercontent.com/u/40949268/dump/pointer-lock-demo/index.html
5. Open Web Console (Ctrl+Shift+K on Windows)
6. Click on the canvas, choose "Hide pointer"
7. Move the mouse cursor a tiny amount to try to move the ball by one pixel.
8. Observe the prints in console log.

Observed: When the mouse cursor is moved by one pixel, the page prints something like

"Got mouse movement of X: -386 and Y:-104" app.js:105
"Got mouse movement of X: -386 and Y:-102" app.js:105
"Got mouse movement of X: -388 and Y:-101" app.js:105
"Got mouse movement of X: -389 and Y:-98" app.js:105
"Got mouse movement of X: -391 and Y:-98" app.js:105
....

Expected: The page should print something like

"Got mouse movement of X: 1 and Y:0" app.js:105
"Got mouse movement of X: 2 and Y:1" app.js:105
"Got mouse movement of X: 0 and Y:1" app.js:105
"Got mouse movement of X: 3 and Y:1" app.js:105
"Got mouse movement of X: 0 and Y:2" app.js:105

@Earle Castledine, any chance you had e10s enabled? To verify whether e10s is active, navigate to "about:config" and search for "browser.tabs.remote.autostart". If that says true, e10s is enabled.

	Loic
	Comment 3 • 10 years ago

Confirmed with e10s enabled.

	Earle Castledine Reporter
	Comment 4 • 10 years ago

Yep, sorry I forgot I had enabled it. Well spotted, by the way.

	Jukka Jylänki
	Comment 5 • 10 years ago

Thanks, good to know, now we are sure this is definitely just e10s specific. You had this on OSX, and I'm seeing it on Windows, so marking OS to all.

	Alon Zakai (:azakai)
	Comment 6 • 10 years ago

This looks like a dupe of bug 1044228.

	Jukka Jylänki
	Comment 7 • 10 years ago

They definitely are reporting issue with the same, but given that the STRs to verify that the bug is fixed, and that the user stories are different (1044228 just says pointer lock doesn't trigger, this bug says pointer lock does trigger, but values reported are bogus), I'd recommend keeping both open until both STRs are verified to have been fixed.

	Alon Zakai (:azakai)
	Comment 11 • 9 years ago

Relaying a suggestion from an email thread, perhaps we should disable pointer lock when e10s is on, until we can find time to fix this? At least that way apps get a clear error, instead of the API appearing to work but actually being broken.

	Jim Mathies [:jimm] Assignee
	Comment 14 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd3fd6f6476

	Jim Mathies [:jimm] Assignee
	Comment 15 • 9 years ago

This fixes the code that makes pointer lock mouse lock work, which involves repositioning the mouse on every user mouse move to the center of the window and then filtering these synthetic events in esm when they loop around. It's really a pretty awful implementation which needs to be re-written. Also tests for pointer lock are disabled pretty much everywhere because they are very prone to failure.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 16 • 9 years ago

Comment on attachment 8576020 [details] [diff] [review]
patch

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

Can you talk to handyman about how this will interact with bug 1075670? Hopefully it will make this patch a little simpler.

Also, I'm worried about the security implications here. Is there a way we could only allow this if we're in fullscreen mode?

	Jim Mathies [:jimm] Assignee
	Comment 17 • 9 years ago

(In reply to Bill McCloskey (:billm) from comment #16)
> Comment on attachment 8576020 [details] [diff] [review]
> patch
> 
> Review of attachment 8576020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you talk to handyman about how this will interact with bug 1075670?
> Hopefully it will make this patch a little simpler.

Hmm, thought that landed. Will do.

> Also, I'm worried about the security implications here. Is there a way we
> could only allow this if we're in fullscreen mode?

Don't think so, the api doesn't specify you have to be in fullscreen to use this. Single process definitely does not enforce a restriction like it.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 18 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #17)
> (In reply to Bill McCloskey (:billm) from comment #16)
> Don't think so, the api doesn't specify you have to be in fullscreen to use
> this. Single process definitely does not enforce a restriction like it.

At first I thought maybe we could use the permission manager to handle this. TabParent could get the current URL and check if it has pointer lock permissions. However, we currently allow iframes to do pointer locking, so TabParent can't even say for sure what the right URL is.

I'd really like to have pointer locking and this patch seems pretty nice. Maybe we should ask the security team for ideas. I found the original security review for non-fullscreen pointer locking here:
https://bugzilla.mozilla.org/show_bug.cgi?id=822654

	Jim Mathies [:jimm] Assignee
	Comment 19 • 9 years ago

(In reply to Bill McCloskey (:billm) from comment #18)
> (In reply to Jim Mathies [:jimm] from comment #17)
> > (In reply to Bill McCloskey (:billm) from comment #16)
> > Don't think so, the api doesn't specify you have to be in fullscreen to use
> > this. Single process definitely does not enforce a restriction like it.
> 
> At first I thought maybe we could use the permission manager to handle this.
> TabParent could get the current URL and check if it has pointer lock
> permissions. However, we currently allow iframes to do pointer locking, so
> TabParent can't even say for sure what the right URL is.
> 
> I'd really like to have pointer locking and this patch seems pretty nice.
> Maybe we should ask the security team for ideas. I found the original
> security review for non-fullscreen pointer locking here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=822654

I'm a little confused here, a are your security concerns specific to this patch and e10s?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 20 • 9 years ago

Well, my concerns are related to sandboxing. With sandboxing, we assume that the content process has been compromised and we want to ensure that it still can't do anything bad. This patch would allow it to move the pointer to arbitrary places on the screen. That's a pretty serious exploit.

I looked at how Chrome deals with this and they seem to restrict how pointer locking is used in iframes:
http://www.chromium.org/developers/design-documents/mouse-lock
I don't entirely understand the restrictions though.

My fear is that if we land this for e10s then it will be forgotten for sandboxing, so it would be nice to get it right the first time.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 21 • 9 years ago

I talked to Chris Pearce and he's okay with only allowing pointer locking for top-level frames or for fullscreen. However, it looks like my permission manager check idea won't work. I filed bug 1142256 about that.

I guess we should just proceed without any security checking for now since bug 1142256 will be some work. I'll review this once we figure out how this relates to the screenX bug.

	Jim Mathies [:jimm] Assignee
	Comment 23 • 9 years ago

Comment on attachment 8588439 [details] [diff] [review]
patch

One minor change, in TabParent::RecvSynthesizeNativeMouseMove there was no need for the added chrome offset. Puppet widget is now returning the correct value for WidgetToScreenOffset.

	Jim Mathies [:jimm] Assignee
	Comment 24 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eda062070a2b

	Jim Mathies [:jimm] Assignee
	Comment 25 • 9 years ago

I just noticed a comment in here that needed an edit as well.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 26 • 9 years ago

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

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

::: dom/ipc/PBrowser.ipdl
@@ +337,5 @@
> +     * events.
> +     *
> +     * aPoint a synth point relative to the window
> +     */
> +    sync SynthesizeNativeMouseMove(nsIntPoint aPoint);

I think you can use LayoutDeviceIntPoint here by declaring it at the top of the file, similar to LayoutDeviceIntRect, which is already there. Then you can drop some casts elsewhere in the patch.

	Jim Mathies [:jimm] Assignee
	Comment 27 • 9 years ago

Updated per comments. The previous try push is still good for landing.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa38fd43c148

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/aa38fd43c148

	Luke
	Comment 30 • 9 years ago

FIXED?

Has this patch landed in the  40.0a1 (2015-04-08) Nightly? The the pointer lock is still broken there. An easy way to test is the bananabread demo. 
https://developer.mozilla.org/en-US/demos/detail/bananabread

When you press ` to open the menu, the mouse cursor does not work with e10s enabled. Tested with Ubuntu 14.10.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 31 • 9 years ago

No, it hasn't.  It was merged to mozilla-central this morning, so it will be in tomorrow's nightly.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 32 • 9 years ago

Jim, fyi I've been working on making these native event synthesization functions async and also wiring them up so they work in B2G (i.e. in content processes, which should work on e10s as well). Bug 1146349 has my work so far, and it's getting close to complete. If you have any other changes like this one pending please let me know.

	Jim Mathies [:jimm] Assignee
	Comment 33 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> Jim, fyi I've been working on making these native event synthesization
> functions async and also wiring them up so they work in B2G (i.e. in content
> processes, which should work on e10s as well). Bug 1146349 has my work so
> far, and it's getting close to complete. If you have any other changes like
> this one pending please let me know.

Not working on anything else related to synth events, this bug was a one-off for this weird pointer lock implementation we have.

	Alon Zakai (:azakai)
	Comment 34 • 9 years ago

Verified working on Unity DT2 and the two links in the first comment. Great!

(I did however see that on the three.js demo there is another e10s issue with the cursor not vanishing, filed 1152866.)

	u528881
	Comment 35 • 9 years ago

Thanks for fixing! It works in everything I've tried, including Em-DOSBox. Control via locked mouse works both in a normal tab and in full screen mode. The cursor was always properly hidden in a normal tab, but always failed to hide in full screen mode as Alon Zakai reports in 1152866.