Isolate a presenting VR device from pages other than the one presenting.

Isolate a presenting VR device from pages other than the one presenting.

Prevents a page from reading or resetting the pose of a
device which is currently being presented to by a
different page, as that potentially creates an
information leak between pages.

Also prevented pages from beginning presentation if another page is already presenting to a device. This is a
little overly aggressive right now, as disallows
presentation to more than one device at a time from any
page, but that's a reasonable limitation at this point
given the rarity of VR hardware.

BUG=389343
Committed: https://crrev.com/73ed46b81d0a4da803a9ad25bb217f2aac28709a
Cr-Commit-Position: refs/heads/master@{#418771}

[bajones, 2016-09-13 17:59:25]

Description was changed from

==========
Enabled the VR service to accept or reject presentation requests.

Also enabled the VR service to arbitrarily end presentation at any time,
which may be important to do if the hardware or user agent determines
that presentation should stop.

BUG=389343
==========

to

==========
Isolate a presenting VR device from pages other than the one presenting.

Prevents a page from reading or resetting the pose of a
device which is currently being presented to by a
different page, as that potentially creates an
information leak between pages.

Also prevented pages from beginning presentation if another page is already
presenting to a device. This is a
little overly aggressive right now, as disallows
presentation to more than one device at a time from any
page, but that's a reasonable limitation at this point
given the rarity of VR hardware.


BUG=389343
==========

[bajones, 2016-09-13 18:01:56]

bajones@chromium.org changed reviewers:
+ klausw@chromium.org, mkwst@chromium.org, mthiesse@chromium.org

[bajones, 2016-09-13 18:01:57]

mkwst@: Could you review the changes to the .mojom file?

mthiesse@ and klausw@: Sanity checks on the VR code please!

[mthiesse, 2016-09-13 18:46:15]

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
device/vr/vr_device_manager.cc:103: if (service == presenting_service_) {
With these checks peppered all over the place, it might make sense to check this
at GetInstance() instead, and not allow the wrong service to get an instance of
this class at all. That would probably be much easier to maintain in the long
term.

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
device/vr/vr_device_manager.cc:176: if (presenting_device_ &&
presenting_device_->id() != index) {
Is it possible for there to be a presenting service but no presenting device?
Maybe DCHECK for this?

https://codereview.chromium.org/2329893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2329893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:209:
m_fullscreenCheckTimer.startRepeating(1.0, BLINK_FROM_HERE);
ew. I know this is already landed, so don't let my being grossed out stop this
from landing again... but is this really the best way to do this?

https://codereview.chromium.org/2329893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:257: updateLayerBounds();
This feels like it should go before the PresentChange notification

[bajones, 2016-09-13 19:42:57]

Thanks for the feedback, addressed or commented on everything.

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
device/vr/vr_device_manager.cc:103: if (service == presenting_service_) {
On 2016/09/13 18:46:15, mthiesse wrote:
> With these checks peppered all over the place, it might make sense to check
this
> at GetInstance() instead, and not allow the wrong service to get an instance
of
> this class at all. That would probably be much easier to maintain in the long
> term.

I see your point (and I already did something similar with GetAllowedDevice) but
there's valid reasons for a service to access this class even if it's not
presenting. (Removing itself from the manager when the associated page closes is
one example.)

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
device/vr/vr_device_manager.cc:176: if (presenting_device_ &&
presenting_device_->id() != index) {
On 2016/09/13 18:46:15, mthiesse wrote:
> Is it possible for there to be a presenting service but no presenting device?
> Maybe DCHECK for this?

Good Call. Done.

https://codereview.chromium.org/2329893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2329893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:209:
m_fullscreenCheckTimer.startRepeating(1.0, BLINK_FROM_HERE);
On 2016/09/13 18:46:15, mthiesse wrote:
> ew. I know this is already landed, so don't let my being grossed out stop this
> from landing again... but is this really the best way to do this?

NO! It's not anywhere close to the best way to do this! This is an evil ugly
gross no good temporary hack to approximate the presentation experience until
Klaus and I get a proper presentation pipeline wired up. The MOMENT that happens
this code is GONE.

https://codereview.chromium.org/2329893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:257: updateLayerBounds();
On 2016/09/13 18:46:15, mthiesse wrote:
> This feels like it should go before the PresentChange notification

Done.

[mthiesse, 2016-09-13 20:01:16]

lgtm % comment

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2329893002/diff/20001/device/vr/vr_device_man...
device/vr/vr_device_manager.cc:103: if (service == presenting_service_) {
On 2016/09/13 19:42:57, bajones wrote:
> On 2016/09/13 18:46:15, mthiesse wrote:
> > With these checks peppered all over the place, it might make sense to check
> this
> > at GetInstance() instead, and not allow the wrong service to get an instance
> of
> > this class at all. That would probably be much easier to maintain in the
long
> > term.
> 
> I see your point (and I already did something similar with GetAllowedDevice)
but
> there's valid reasons for a service to access this class even if it's not
> presenting. (Removing itself from the manager when the associated page closes
is
> one example.)

Is there something preventing you from only restricting access at GetInstance()
if somebody is already presenting?

Something like:
if (presenting_service_) {
  if (service != presenting_service_)
    return nullptr;
}
return instance;


This obviously won't work if clients can hold onto instances of this class and
call its functions at some point in the future, but it looks like that isn't the
case?

[haraken, 2016-09-14 00:08:19]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-09-14 00:08:20]

https://codereview.chromium.org/2329893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2329893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRController.cpp:151:
resolver->reject(exception);

Don't we need to add 'return' here?

[bajones, 2016-09-14 00:30:03]

https://codereview.chromium.org/2329893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2329893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRController.cpp:151:
resolver->reject(exception);
On 2016/09/14 00:08:20, haraken wrote:
> 
> Don't we need to add 'return' here?

Yes, we do. Thanks! Fixed.

[Mike West, 2016-09-14 12:54:50]

mojom LGTM

[bajones, 2016-09-14 15:09:53]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-09-14 15:09:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2329893002/#ps60001
(title: "Added suggested return")

[commit-bot: I haz the power, 2016-09-14 15:10:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 15:21:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 15:21:51]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-09-14 15:47:32]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-09-14 15:47:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2329893002/#ps80001
(title: "Fixed style guide violation")

[commit-bot: I haz the power, 2016-09-14 15:47:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bajones, 2016-09-14 16:13:00]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-09-14 16:13:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2329893002/#ps100001
(title: "Fixed Android compile bug")

[commit-bot: I haz the power, 2016-09-14 16:13:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 16:37:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 16:37:02]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[bajones, 2016-09-14 18:18:11]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-09-14 18:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2329893002/#ps120001
(title: "Unit tests")

[commit-bot: I haz the power, 2016-09-14 18:18:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 22:03:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 22:03:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bajones, 2016-09-14 22:10:26]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-09-14 22:10:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 02:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 02:07:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2016-09-15 02:13:06]

LGTM

[bajones, 2016-09-15 02:34:00]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-09-15 02:34:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 03:53:44]

Description was changed from

==========
Isolate a presenting VR device from pages other than the one presenting.

Prevents a page from reading or resetting the pose of a
device which is currently being presented to by a
different page, as that potentially creates an
information leak between pages.

Also prevented pages from beginning presentation if another page is already
presenting to a device. This is a
little overly aggressive right now, as disallows
presentation to more than one device at a time from any
page, but that's a reasonable limitation at this point
given the rarity of VR hardware.


BUG=389343
==========

to

==========
Isolate a presenting VR device from pages other than the one presenting.

Prevents a page from reading or resetting the pose of a
device which is currently being presented to by a
different page, as that potentially creates an
information leak between pages.

Also prevented pages from beginning presentation if another page is already
presenting to a device. This is a
little overly aggressive right now, as disallows
presentation to more than one device at a time from any
page, but that's a reasonable limitation at this point
given the rarity of VR hardware.


BUG=389343
==========

[commit-bot: I haz the power, 2016-09-15 03:53:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-15 03:55:49]

Description was changed from

==========
Isolate a presenting VR device from pages other than the one presenting.

Prevents a page from reading or resetting the pose of a
device which is currently being presented to by a
different page, as that potentially creates an
information leak between pages.

Also prevented pages from beginning presentation if another page is already
presenting to a device. This is a
little overly aggressive right now, as disallows
presentation to more than one device at a time from any
page, but that's a reasonable limitation at this point
given the rarity of VR hardware.


BUG=389343
==========

to

==========
Isolate a presenting VR device from pages other than the one presenting.

Prevents a page from reading or resetting the pose of a
device which is currently being presented to by a
different page, as that potentially creates an
information leak between pages.

Also prevented pages from beginning presentation if another page is already
presenting to a device. This is a
little overly aggressive right now, as disallows
presentation to more than one device at a time from any
page, but that's a reasonable limitation at this point
given the rarity of VR hardware.

BUG=389343

Committed: https://crrev.com/73ed46b81d0a4da803a9ad25bb217f2aac28709a
Cr-Commit-Position: refs/heads/master@{#418771}
==========

[commit-bot: I haz the power, 2016-09-15 03:55:50]

Patchset 7 (id:??) landed as
https://crrev.com/73ed46b81d0a4da803a9ad25bb217f2aac28709a
Cr-Commit-Position: refs/heads/master@{#418771}