Only set MouseLockDispatcher unlocked_by_target_ after mouse lock confirmed.

Only set MouseLockDispatcher unlocked_by_target_ after mouse lock confirmed.

When a target application voluntarily unlocks the mouse we permit
relocking the mouse silently and with no user gesture requirement.
Check that the lock request is not currently pending and not yet
accepted by the browser process before setting |unlocked_by_target_|.

This corrects an ommision in https://chromiumcodereview.appspot.com/10443045/.

It is very difficult to test this possible failure, as the requests must be processed by the renderer process before the browser process can respond to the IPCs. An attempt was made to create this exploit, but was not able to produce the failure.

BUG=113460
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140598

[yzshen1, 2012-06-04 21:40:30]

https://chromiumcodereview.appspot.com/10512011/diff/1/content/renderer/mouse...
File content/renderer/mouse_lock_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10512011/diff/1/content/renderer/mouse...
content/renderer/mouse_lock_dispatcher.cc:48: // When a target application
voluntarily unlocks the mouse we permit
[I don't know the exact answer, but would like to make sure that you have
considered it.]
According to the code, we are assuming that a MouseLockDispatcher object will
only be used by one "application": we don't distinguish target that sets
|unlocked_by_target_| and the one that uses it. Is that intended? Can we
guarantee that?

[scheib, 2012-06-04 23:25:40]

https://chromiumcodereview.appspot.com/10512011/diff/1/content/renderer/mouse...
File content/renderer/mouse_lock_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10512011/diff/1/content/renderer/mouse...
content/renderer/mouse_lock_dispatcher.cc:48: // When a target application
voluntarily unlocks the mouse we permit
On 2012/06/04 21:40:30, yzshen1 wrote:
> [I don't know the exact answer, but would like to make sure that you have
> considered it.]
> According to the code, we are assuming that a MouseLockDispatcher object will
> only be used by one "application": we don't distinguish target that sets
> |unlocked_by_target_| and the one that uses it. Is that intended? Can we
> guarantee that?

I considered this when designing the first patch. A web page is composing code
and plugins that may call this API. Before and after this new feature (silent
acceptance) code on a page is able to fight with other code on the page after
the user has accepted mouse lock the first time. The difference is that
previously it would fight only on user gestures, and the user would see exit
instructions.

I don't think we can put protections in place to fix poorly composed pages with
multiple sources trying to use mouse lock. It is similar to fullscreen, or
audio, or other apis: a poorly composed page can result in a poor experience.

[yzshen1, 2012-06-04 23:50:55]

LGTM

Thanks!

[scheib, 2012-06-05 00:10:13]

piman, owners review, please.

[piman, 2012-06-05 00:21:36]

lgtm

[commit-bot: I haz the power, 2012-06-05 02:41:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/10512011/1

[commit-bot: I haz the power, 2012-06-05 02:41:11]

Failed to apply patch for content/renderer/mouse_lock_dispatcher.cc:
While running patch -p1 --forward --force;
patching file content/renderer/mouse_lock_dispatcher.cc
Hunk #1 FAILED at 44.
1 out of 1 hunk FAILED -- saving rejects to file
content/renderer/mouse_lock_dispatcher.cc.rej