Ensure navigator.getVRDisplays always resolves.

third_party/WebKit/Source/modules/vr/VRController.cpp

Index: third_party/WebKit/Source/modules/vr/VRController.cpp
diff --git a/third_party/WebKit/Source/modules/vr/VRController.cpp b/third_party/WebKit/Source/modules/vr/VRController.cpp
index 12dac02739349eb1dd4820eb0d1904cfb9ef42b1..0df41d123929b8776bdb9a749274202c3f2677af 100644
--- a/third_party/WebKit/Source/modules/vr/VRController.cpp
+++ b/third_party/WebKit/Source/modules/vr/VRController.cpp
@@ -23,6 +23,8 @@ VRController::VRController(NavigatorVR* navigatorVR)
       m_binding(this) {
   navigatorVR->document()->frame()->interfaceProvider()->getInterface(
       mojo::MakeRequest(&m_service));
+  m_service.set_connection_error_handler(convertToBaseCallback(
+      WTF::bind(&VRController::dispose, wrapWeakPersistent(this))));
   m_service->SetClient(
       m_binding.CreateInterfacePtrAndBind(),
       convertToBaseCallback(
@@ -32,15 +34,10 @@ VRController::VRController(NavigatorVR* navigatorVR)
 VRController::~VRController() {}
 
 void VRController::getDisplays(ScriptPromiseResolver* resolver) {
-  if (!m_service) {
-    DOMException* exception = DOMException::create(
-        InvalidStateError, "The service is no longer active.");
-    resolver->reject(exception);
-    return;
-  }
-
-  // If we've previously synced the VRDisplays just return the current list.
-  if (m_displaySynced) {
+  // If we've previously synced the VRDisplays or no longer have a valid service
+  // connection just return the current list. In the case of the service being
+  // disconnected this will be an empty array.
+  if (!m_service || m_displaySynced) {
     resolver->resolve(m_displays);
     return;
   }
@@ -106,6 +103,11 @@ void VRController::dispose() {
   // Shutdown all displays' message pipe
   for (size_t i = 0; i < m_displays.size(); ++i)
     m_displays[i]->dispose();
+
+  m_displays.clear();
+
+  // Ensure that any outstanding getDisplays promises are resolved.
+  onGetDisplays();

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

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

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

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]

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

 }
 
 DEFINE_TRACE(VRController) {