Fix a crash after exiting WebXR presentation

XRSession has a collection of V8 callbacks that it will call for
animation frame requests.

These callbacks were detected as unreachable from V8's perspective,
even though XRSession was alive from blink's perspective.

The fix is to make XRSession an ActiveScriptWrappable, so it is a
root object from V8's garbage collection while there are active
callbacks.

In discussion about this bug, it was pointed out that a couple places
we keep objects on the stack might also cause V8's garbage collector
to think wrappers are unreachable, so I now keep those objects alive
with explcit traceable references.

BUG=870403

Cq-Include-Trybots: luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I54cfda8597439c0a988acd918a1fcc6904cca340
Reviewed-on: https://chromium-review.googlesource.com/1166187
Commit-Queue: Bill Orr <billorr@chromium.org>
Reviewed-by: Brandon Jones <bajones@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581515}

third_party/blink/renderer/modules/xr/xr_frame_provider.cc

@@ -399,12 +399,12 @@ void XRFrameProvider::ProcessScheduledFrame(
     // In the process of fulfilling the frame requests for each session they are
     // extremely likely to request another frame. Work off of a separate list
     // from the requests to prevent infinite loops.
-    HeapVector<Member<XRSession>> processing_sessions;
-    swap(requesting_sessions_, processing_sessions);
+    DCHECK(processing_sessions_.IsEmpty());
+    swap(requesting_sessions_, processing_sessions_);
 
     // Inform sessions with a pending request of the new frame
-    for (unsigned i = 0; i < processing_sessions.size(); ++i) {
-      XRSession* session = processing_sessions.at(i).Get();
+    for (unsigned i = 0; i < processing_sessions_.size(); ++i) {
+      XRSession* session = processing_sessions_.at(i).Get();
 
       if (frame_pose_ && frame_pose_->input_state.has_value()) {
         session->OnInputStateChange(frame_id_,
@@ -432,6 +432,8 @@ void XRFrameProvider::ProcessScheduledFrame(
                          base::nullopt, base::nullopt);
       }
     }
+
+    processing_sessions_.clear();
   }
 }
 
@@ -544,6 +546,7 @@ void XRFrameProvider::Trace(blink::Visitor* visitor) {
   visitor->Trace(frame_transport_);
   visitor->Trace(immersive_session_);
   visitor->Trace(requesting_sessions_);
+  visitor->Trace(processing_sessions_);
 }
 
 }  // namespace blink

third_party/blink/renderer/modules/xr/xr_frame_provider.h

@@ -64,6 +64,7 @@ class XRFrameProvider final
 
   // Non-immersive Sessions which have requested a frame update.
   HeapVector<Member<XRSession>> requesting_sessions_;
+  HeapVector<Member<XRSession>> processing_sessions_;
 
   device::mojom::blink::XRPresentationProviderPtr presentation_provider_;
   device::mojom::blink::XRFrameDataProviderPtr immersive_data_provider_;

third_party/blink/renderer/modules/xr/xr_frame_request_callback_collection.cc

@@ -30,6 +30,7 @@ XRFrameRequestCallbackCollection::RegisterCallback(
 void XRFrameRequestCallbackCollection::CancelCallback(CallbackId id) {
   if (IsValidCallbackId(id)) {
     callbacks_.erase(id);
+    current_callbacks_.erase(id);
   }
 }
 
@@ -38,26 +39,36 @@ void XRFrameRequestCallbackCollection::ExecuteCallbacks(XRSession* session,
                                                         XRFrame* frame) {
   // First, generate a list of callbacks to consider.  Callbacks registered from
   // this point on are considered only for the "next" frame, not this one.
-  DCHECK(callbacks_to_invoke_.IsEmpty());
-  callbacks_to_invoke_.swap(pending_callbacks_);
 
-  for (const auto& id : callbacks_to_invoke_) {
-    V8XRFrameRequestCallback* callback = callbacks_.Take(id);
+  // Conceptually we are just going to iterate through current_callbacks_, and
+  // call each callback.  However, if we had multiple callbacks, subsequent ones
+  // could be removed while we are iterating.  HeapHashMap iterators aren't
+  // valid after collection modifications, so we also store a corresponding set
+  // of ids for iteration purposes.  current_callback_ids is the set of ids for
+  // callbacks we will call, and is kept in sync with current_callbacks_ but
+  // safe to iterate over.
+  DCHECK(current_callbacks_.IsEmpty());
+  current_callbacks_.swap(callbacks_);
 
-    // Callback won't be found if it was cancelled.
-    if (!callback)
+  Vector<CallbackId> current_callback_ids;
+  current_callback_ids.swap(pending_callbacks_);
+
+  for (const auto& id : current_callback_ids) {
+    auto it = current_callbacks_.find(id);
+    if (it == current_callbacks_.end())
       continue;
 
-    probe::AsyncTask async_task(context_, callback);
+    probe::AsyncTask async_task(context_, it->value);
     probe::UserCallback probe(context_, "XRRequestFrame", AtomicString(), true);
-    callback->InvokeAndReportException(session, timestamp, frame);
+    it->value->InvokeAndReportException(session, timestamp, frame);
   }
 
-  callbacks_to_invoke_.clear();
+  current_callbacks_.clear();
 }
 
 void XRFrameRequestCallbackCollection::Trace(blink::Visitor* visitor) {
   visitor->Trace(callbacks_);
+  visitor->Trace(current_callbacks_);
   visitor->Trace(context_);
 }
 

third_party/blink/renderer/modules/xr/xr_frame_request_callback_collection.h

@@ -45,8 +45,9 @@ class XRFrameRequestCallbackCollection final
       HeapHashMap<CallbackId, TraceWrapperMember<V8XRFrameRequestCallback>>;
   CallbackMap callbacks_;
   Vector<CallbackId> pending_callbacks_;
+
   // Only non-empty while inside executeCallbacks.
-  Vector<CallbackId> callbacks_to_invoke_;
+  CallbackMap current_callbacks_;
 
   CallbackId next_callback_id_ = 0;
 

third_party/blink/renderer/modules/xr/xr_session.cc

@@ -782,6 +782,10 @@ const HeapVector<Member<XRView>>& XRSession::views() {
   return views_;
 }
 
+bool XRSession::HasPendingActivity() const {
+  return !callback_collection_->IsEmpty() && !ended_;
+}
+
 void XRSession::Trace(blink::Visitor* visitor) {
   visitor->Trace(device_);
   visitor->Trace(output_context_);

third_party/blink/renderer/modules/xr/xr_session.h

@@ -19,6 +19,8 @@
 #include "third_party/blink/renderer/platform/transforms/transformation_matrix.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 
+#include "third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h"
+
 namespace blink {
 
 class Element;
@@ -34,8 +36,10 @@ class XRLayer;
 class XRPresentationContext;
 class XRView;
 
-class XRSession final : public EventTargetWithInlineData {
+class XRSession final : public EventTargetWithInlineData,
+                        public ActiveScriptWrappable<XRSession> {
   DEFINE_WRAPPERTYPEINFO();
+  USING_GARBAGE_COLLECTED_MIXIN(XRSession);
 
  public:
   enum EnvironmentBlendMode {
@@ -143,6 +147,9 @@ class XRSession final : public EventTargetWithInlineData {
 
   void Trace(blink::Visitor*) override;
 
+  // ScriptWrappable
+  bool HasPendingActivity() const override;
+
  private:
   class XRSessionResizeObserverDelegate;
 

third_party/blink/renderer/modules/xr/xr_session.idl

@@ -10,6 +10,7 @@ enum XREnvironmentBlendMode {
 };
 
 [
+    ActiveScriptWrappable,
     SecureContext,
     Exposed=Window,
     OriginTrialEnabled=WebXR