[e10s] PointerLock API reports very large values for mozMovementX/Y

VERIFIED FIXED in Firefox 40

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mrspeaker, Assigned: jimm)

Tracking

(Blocks 1 bug)

36 Branch
mozilla40
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm6+, firefox40 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

5 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.

Updated

5 years ago
Component: Untriaged → DOM: Events
Product: Firefox → Core
Looks fine in 36.0a1 (2014-10-27) Win 7 x64

Comment 2

5 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
tracking-e10s: --- → ?

Comment 3

5 years ago
Confirmed with e10s enabled.
Blocks: e10s
Flags: needinfo?(mrspeaker)
Summary: PointerLock API reports very large values for mozMovementX/Y → [e10s] PointerLock API reports very large values for mozMovementX/Y
Reporter

Comment 4

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

Comment 5

5 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.
Flags: needinfo?(mrspeaker)
OS: Mac OS X → All
This looks like a dupe of bug 1044228.

Comment 7

5 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.

Updated

5 years ago
Duplicate of this bug: 1127637
Duplicate of this bug: 1107554
Duplicate of this bug: 1129484
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.
Assignee

Updated

4 years ago
Assignee: nobody → jmathies
Assignee

Updated

4 years ago
Blocks: 633602
Assignee

Comment 12

4 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee

Comment 13

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8576007 - Attachment is obsolete: true
Assignee

Comment 15

4 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #8576008 - Attachment is obsolete: true
Attachment #8576020 - Flags: review?(wmccloskey)
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?
Assignee

Comment 17

4 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.
Assignee

Updated

4 years ago
Depends on: 1075670
(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
Assignee

Comment 19

4 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?
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.
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.
Assignee

Comment 22

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8576020 - Attachment is obsolete: true
Attachment #8576020 - Flags: review?(wmccloskey)
Assignee

Comment 23

4 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.
Attachment #8588439 - Flags: review?(wmccloskey)
Assignee

Comment 25

4 years ago
Posted patch patch v.2 (obsolete) — Splinter Review
I just noticed a comment in here that needed an edit as well.
Attachment #8588439 - Attachment is obsolete: true
Attachment #8588439 - Flags: review?(wmccloskey)
Attachment #8588444 - Flags: review?(wmccloskey)
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.
Attachment #8588444 - Flags: review?(wmccloskey) → review+
Assignee

Comment 27

4 years ago
Updated per comments. The previous try push is still good for landing.
Attachment #8588444 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa38fd43c148
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Comment 30

4 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.
No, it hasn't.  It was merged to mozilla-central this morning, so it will be in tomorrow's nightly.
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.
Flags: needinfo?(jmathies)
Assignee

Comment 33

4 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.
Flags: needinfo?(jmathies)
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.)
Status: RESOLVED → VERIFIED

Comment 35

4 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.
You need to log in before you can comment on or make changes to this bug.