From 454157dd660f7ed70684a459fff0482bee1d87d1 Mon Sep 17 00:00:00 2001
From: Brian Vaughn <bvaughn@fb.com>
Date: Fri, 16 Aug 2019 11:25:44 -0700
Subject: [PATCH] Multi-renderer profiling improvements

Add support for mixed v15/v16 renderers that previously caused profiling to fail with 'profiling not supported by this renderer' type errors
---
 src/__tests__/profilerContext-test.js |  2 +-
 src/backend/legacy/renderer.js        |  4 ++--
 src/backend/renderer.js               | 12 +++++++++---
 src/devtools/ProfilerStore.js         | 11 ++++++++++-
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/src/__tests__/profilerContext-test.js b/src/__tests__/profilerContext-test.js
index c06f70d039..97eef46476 100644
--- a/src/__tests__/profilerContext-test.js
+++ b/src/__tests__/profilerContext-test.js
@@ -135,7 +135,7 @@ describe('ProfilerContext', () => {
     expect(context.didRecordCommits).toBe(false);
     expect(context.isProcessingData).toBe(false);
     expect(context.isProfiling).toBe(false);
-    expect(context.profilingData).not.toBe(null);
+    expect(context.profilingData).toBe(null);
 
     done();
   });
diff --git a/src/backend/legacy/renderer.js b/src/backend/legacy/renderer.js
index 51ca6a626a..036083af1e 100644
--- a/src/backend/legacy/renderer.js
+++ b/src/backend/legacy/renderer.js
@@ -875,10 +875,10 @@ export function attach(
     throw new Error('setInHook not supported by this renderer');
   };
   const startProfiling = () => {
-    throw new Error('startProfiling not supported by this renderer');
+    // Do not throw, since this would break a multi-root scenario where v15 and v16 were both present.
   };
   const stopProfiling = () => {
-    throw new Error('stopProfiling not supported by this renderer');
+    // Do not throw, since this would break a multi-root scenario where v15 and v16 were both present.
   };
 
   function getBestMatchForTrackedPath(): PathMatch | null {
diff --git a/src/backend/renderer.js b/src/backend/renderer.js
index 63aae20c87..1de2b36e7e 100644
--- a/src/backend/renderer.js
+++ b/src/backend/renderer.js
@@ -1614,7 +1614,9 @@ export function attach(
         currentRootID = getFiberID(getPrimaryFiber(root.current));
         setRootPseudoKey(currentRootID, root.current);
 
-        if (isProfiling) {
+        // Checking root.memoizedInteractions handles multi-renderer edge-case-
+        // where some v16 renderers support profiling and others don't.
+        if (isProfiling && root.memoizedInteractions != null) {
           // If profiling is active, store commit time and duration, and the current interactions.
           // The frontend may request this information after profiling has stopped.
           currentCommitProfilingMetadata = {
@@ -1658,7 +1660,11 @@ export function attach(
       mightBeOnTrackedPath = true;
     }
 
-    if (isProfiling) {
+    // Checking root.memoizedInteractions handles multi-renderer edge-case-
+    // where some v16 renderers support profiling and others don't.
+    const isProfilingSupported = root.memoizedInteractions != null;
+
+    if (isProfiling && isProfilingSupported) {
       // If profiling is active, store commit time and duration, and the current interactions.
       // The frontend may request this information after profiling has stopped.
       currentCommitProfilingMetadata = {
@@ -1702,7 +1708,7 @@ export function attach(
       mountFiberRecursively(current, null);
     }
 
-    if (isProfiling) {
+    if (isProfiling && isProfilingSupported) {
       const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get(
         currentRootID
       );
diff --git a/src/devtools/ProfilerStore.js b/src/devtools/ProfilerStore.js
index 103947ac8c..cf04b65939 100644
--- a/src/devtools/ProfilerStore.js
+++ b/src/devtools/ProfilerStore.js
@@ -62,6 +62,9 @@ export default class ProfilerStore extends EventEmitter<{|
   // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees.
   _isProfiling: boolean = false;
 
+  // Tracks whether a specific renderer logged any profiling data during the most recent session.
+  _rendererIDsThatReportedProfilingData: Set<number> = new Set();
+
   // After profiling, data is requested from each attached renderer using this queue.
   // So long as this queue is not empty, the store is retrieving and processing profiling data from the backend.
   _rendererQueue: Set<number> = new Set();
@@ -233,6 +236,8 @@ export default class ProfilerStore extends EventEmitter<{|
       if (!this._initialSnapshotsByRootID.has(rootID)) {
         this._initialSnapshotsByRootID.set(rootID, new Map());
       }
+
+      this._rendererIDsThatReportedProfilingData.add(rendererID);
     }
   };
 
@@ -280,6 +285,7 @@ export default class ProfilerStore extends EventEmitter<{|
       this._initialRendererIDs.clear();
       this._initialSnapshotsByRootID.clear();
       this._inProgressOperationsByRootID.clear();
+      this._rendererIDsThatReportedProfilingData.clear();
       this._rendererQueue.clear();
 
       // Record all renderer IDs initially too (in case of unmount)
@@ -315,7 +321,10 @@ export default class ProfilerStore extends EventEmitter<{|
         this._dataBackends.splice(0);
         this._rendererQueue.clear();
 
-        this._initialRendererIDs.forEach(rendererID => {
+        // Only request data from renderers that actually logged it.
+        // This avoids unnecessary bridge requests and also avoids edge case mixed renderer bugs.
+        // (e.g. when v15 and v16 are both present)
+        this._rendererIDsThatReportedProfilingData.forEach(rendererID => {
           if (!this._rendererQueue.has(rendererID)) {
             this._rendererQueue.add(rendererID);