Ensure navigator.getVRDisplays always resolves.

Ensure navigator.getVRDisplays always resolves.

Second attempt at landing. See https://codereview.chromium.org/2614873002/

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=rockot@chromium.org, alexmos@chromium.org

Review-Url: https://codereview.chromium.org/2630003002
Cr-Commit-Position: refs/heads/master@{#443791}
Committed: https://chromium.googlesource.com/chromium/src/+/a74a47aec12401d3ea3643aea1d8b66d908c32cc

[bajones, 2017-01-13 23:20:35]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 23:21:08]

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

[bajones, 2017-01-13 23:21:57]

Description was changed from

==========
Ensure navigator.getVRDisplays always resolves.

Second attempt at landing. See https://codereview.chromium.org/2614873002/

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure navigator.getVRDisplays always resolves.

Second attempt at landing. See https://codereview.chromium.org/2614873002/

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=rockot@chromium.org, alexmos@chromium.org
==========

[commit-bot: I haz the power, 2017-01-14 01:18:41]

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

[commit-bot: I haz the power, 2017-01-14 01:18:42]

Dry run: This issue passed the CQ dry run.

[bajones, 2017-01-14 05:52:54]

TBRed because this is practically identical to previous version of patch with
one minor dependency change to fix issue that caused revert last time.

[bajones, 2017-01-14 05:53:03]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2017-01-14 05:53:14]

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, 2017-01-14 05:57:01]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484373183783120, "parent_rev":
"c052db270b2e8d0b4388f545395005d7d302ecce", "commit_rev":
"a74a47aec12401d3ea3643aea1d8b66d908c32cc"}

[commit-bot: I haz the power, 2017-01-14 05:58:07]

Description was changed from

==========
Ensure navigator.getVRDisplays always resolves.

Second attempt at landing. See https://codereview.chromium.org/2614873002/

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=rockot@chromium.org, alexmos@chromium.org
==========

to

==========
Ensure navigator.getVRDisplays always resolves.

Second attempt at landing. See https://codereview.chromium.org/2614873002/

In the case that the backing VR service is not available the call will now
resolve to an empty array.

BUG=678283
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=rockot@chromium.org, alexmos@chromium.org

Review-Url: https://codereview.chromium.org/2630003002
Cr-Commit-Position: refs/heads/master@{#443791}
Committed:
https://chromium.googlesource.com/chromium/src/+/a74a47aec12401d3ea3643aea1d8...
==========

[commit-bot: I haz the power, 2017-01-14 05:58:08]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a74a47aec12401d3ea3643aea1d8...

[haraken, 2017-01-16 01:17:42]

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

[haraken, 2017-01-16 01:17:43]

https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRController.cpp:110: onGetDisplays();

This doesn't really seem right to me. If we resolve/reject promises in a
pre-finalizer or a destructor:

- It will expose a GC-behavior to the web; i.e., a web-exposed behavior will
change depending on when a GC is triggered. This should be avoided; i.e., GC
should not be observable.

- There's no guarantee about when a GC is triggered (although a GC will be
eventually triggered in practice). The resolve/reject may not be called.

For the above reasons, resolve/reject should be called at a deterministic timing
in a way that doesn't rely on GCs.

(In more general, it's not a good idea to do something complicated in a
pre-finalizer.)

[haraken, 2017-01-19 00:55:42]

On 2017/01/16 01:17:43, haraken wrote:
>
https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/vr/VRController.cpp (right):
> 
>
https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/vr/VRController.cpp:110: onGetDisplays();
> 
> This doesn't really seem right to me. If we resolve/reject promises in a
> pre-finalizer or a destructor:
> 
> - It will expose a GC-behavior to the web; i.e., a web-exposed behavior will
> change depending on when a GC is triggered. This should be avoided; i.e., GC
> should not be observable.
> 
> - There's no guarantee about when a GC is triggered (although a GC will be
> eventually triggered in practice). The resolve/reject may not be called.
> 
> For the above reasons, resolve/reject should be called at a deterministic
timing
> in a way that doesn't rely on GCs.
> 
> (In more general, it's not a good idea to do something complicated in a
> pre-finalizer.)

Ping on this?

[bajones, 2017-01-19 19:49:00]

https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRController.cpp:110: onGetDisplays();
On 2017/01/16 01:17:43, haraken wrote:
> 
> This doesn't really seem right to me. If we resolve/reject promises in a
> pre-finalizer or a destructor:
> 
> - It will expose a GC-behavior to the web; i.e., a web-exposed behavior will
> change depending on when a GC is triggered. This should be avoided; i.e., GC
> should not be observable.
> 
> - There's no guarantee about when a GC is triggered (although a GC will be
> eventually triggered in practice). The resolve/reject may not be called.
> 
> For the above reasons, resolve/reject should be called at a deterministic
timing
> in a way that doesn't rely on GCs.
> 
> (In more general, it's not a good idea to do something complicated in a
> pre-finalizer.)

Good points, thanks for bringing them up! I'm at a bit of a loss about how to
handle this otherwise, though. If we find that we have outstanding promises when
the object tracking them is GCed, are we supposed to just never resolve them?

Though, now that I think about it, if the page has a reference to the
outstanding promise then that would prevent this object from GCing anyway. In
which case the only occasion that we would hit this code is when the document
itself is being destroyed, at which point any outstanding promises are probably
moot?

[haraken, 2017-01-20 06:28:02]

https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2630003002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRController.cpp:110: onGetDisplays();
On 2017/01/19 19:49:00, bajones wrote:
> On 2017/01/16 01:17:43, haraken wrote:
> > 
> > This doesn't really seem right to me. If we resolve/reject promises in a
> > pre-finalizer or a destructor:
> > 
> > - It will expose a GC-behavior to the web; i.e., a web-exposed behavior will
> > change depending on when a GC is triggered. This should be avoided; i.e., GC
> > should not be observable.
> > 
> > - There's no guarantee about when a GC is triggered (although a GC will be
> > eventually triggered in practice). The resolve/reject may not be called.
> > 
> > For the above reasons, resolve/reject should be called at a deterministic
> timing
> > in a way that doesn't rely on GCs.
> > 
> > (In more general, it's not a good idea to do something complicated in a
> > pre-finalizer.)
> 
> Good points, thanks for bringing them up! I'm at a bit of a loss about how to
> handle this otherwise, though. If we find that we have outstanding promises
when
> the object tracking them is GCed, are we supposed to just never resolve them?
> 
> Though, now that I think about it, if the page has a reference to the
> outstanding promise then that would prevent this object from GCing anyway. In
> which case the only occasion that we would hit this code is when the document
> itself is being destroyed, at which point any outstanding promises are
probably
> moot?

If that is the case, you can move onGetDisplays() to contextDestroyed()? Then
you can assert that all promises are resolved when you reach dispose().