Bug 1713969 - Generate session id ahead of time when requesting new session. r=owlish

Hiro found this race condition.

The problem is in this code:

```
338:    this.eventDispatcher
339:      .sendRequestForResult(message)
340:      .then(sessionId => {
341:        return this.waitAndSetupWindow(sessionId, aOpenWindowInfo, aName);
342:      })
```

At line 339 we ask the Java front-end to create a session and return back the
id of the session just created.

We then, at line 341, wait until the `geckoview-window-created` event fires
which signals that the session has been opened. We then _synchronously_ set
some attributes on the window object that allows us to connect the Java session
to the JavaScript window object.

Synchronicity is important as right after the `geckoview-window-created` event
fires all actors expect the embedderElement to be set on the window.

This, however, races with the code that creates the session:

```
540: } else if ("GeckoView:OnNewSession".equals(event)) {
541:     final String uri = message.getString("uri");
542:     final GeckoResult<GeckoSession> result = delegate.onNewSession(GeckoSession.this, uri);
...
547:
548:     callback.resolveTo(result.map(session -> {
...
562:         session.open(GeckoSession.this.mWindow.runtime);
563:         return session.getId();
564:     }));
```

As you can see, at line 562 we open the session, which asynchronously builds
the Gecko window and eventually fires the `geckoview-window-create` event. In
most cases, the message with the sessionId, sent at line 563, reaches the
JavaScript layer before the window has been opened, but sometimes it doesn't.

When the window opens before the JavaScript layer receives the sessionId back,
the promise at line 341 will never complete, causing intermittent failures in
our testing harness (and causing missing windows in consumer devices).

To fix this problem we modify the timing of creating the GeckoSession id.

The GeckoSession id only makes sense when the session is associated to a
GeckoRuntime instance, as the id is only used by Gecko. We can thus generate
the id whenever the Session is opened.

For sessions that are opened by the embedder directly, we can generate the id
randomly like usual. However, when the session is opened as a result of a
onNewSession (or onNewTab) call, we will set the id of the session using the id
given by the JavaScript layer.

This will enable the JavaScript layer to wait for the `geckoview-window-create`
before the Java layer has opportunity to respond, fixing the race condition.

Co-Authored-By: Hiroyuki Ikezoe <hikezoe.birchill@mozilla.com>

Differential Revision: https://phabricator.services.mozilla.com/D124502

mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java

@@ -121,7 +121,7 @@ public class GeckoSession {
     private SessionAccessibility mAccessibility;
     private SessionFinder mFinder;
 
-    private String mId = UUID.randomUUID().toString().replace("-", "");
+    private String mId;
     /* package */ String getId() {
         return mId;
     }
@@ -541,14 +541,15 @@ public class GeckoSession {
                     final String uri = message.getString("uri");
                     final GeckoResult<GeckoSession> result = delegate.onNewSession(GeckoSession.this, uri);
                     if (result == null) {
-                        callback.sendSuccess(null);
+                        callback.sendSuccess(false);
                         return;
                     }
 
+                    final String newSessionId = message.getString("newSessionId");
                     callback.resolveTo(result.map(session -> {
                         ThreadUtils.assertOnUiThread();
                         if (session == null) {
-                            return null;
+                            return false;
                         }
 
                         if (session.isOpen()) {
@@ -559,8 +560,8 @@ public class GeckoSession {
                             throw new IllegalArgumentException("Session is not attached to a window");
                         }
 
-                        session.open(GeckoSession.this.mWindow.runtime);
-                        return session.getId();
+                        session.open(GeckoSession.this.mWindow.runtime, newSessionId);
+                        return true;
                     }));
                 }
             }
@@ -1182,14 +1183,6 @@ public class GeckoSession {
         onWindowChanged(WINDOW_TRANSFER_OUT, /* inProgress */ false);
     }
 
-    /* package */ boolean equalsId(final GeckoSession other) {
-        if (other == null) {
-            return false;
-        }
-
-        return mId.equals(other.mId);
-    }
-
     /**
      * Return whether this session is open.
      *
@@ -1234,6 +1227,10 @@ public class GeckoSession {
      */
     @UiThread
     public void open(final @NonNull GeckoRuntime runtime) {
+        open(runtime, UUID.randomUUID().toString().replace("-", ""));
+    }
+
+    /* package */ void open(final @NonNull GeckoRuntime runtime, final String id) {
         ThreadUtils.assertOnUiThread();
 
         if (isOpen()) {
@@ -1245,6 +1242,7 @@ public class GeckoSession {
         final int screenId = mSettings.getScreenId();
         final boolean isPrivate = mSettings.getUsePrivateMode();
 
+        mId = id;
         mWindow = new Window(runtime, this, mNativeQueue);
         mWebExtensionController.setRuntime(runtime);
 

mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java

@@ -975,22 +975,22 @@ public class WebExtensionController {
         }
 
         if (result == null) {
-            message.callback.sendSuccess(null);
+            message.callback.sendSuccess(false);
             return;
         }
 
+        final String newSessionId = message.bundle.getString("newSessionId");
         message.callback.resolveTo(result.map(session -> {
             if (session == null) {
-                return null;
+                return false;
             }
 
             if (session.isOpen()) {
                 throw new IllegalArgumentException("Must use an unopened GeckoSession instance");
             }
 
-            session.open(mListener.runtime);
-
-            return session.getId();
+            session.open(mListener.runtime, newSessionId);
+            return true;
         }));
     }
 

mobile/android/modules/geckoview/GeckoViewNavigation.jsm

@@ -329,20 +329,38 @@ class GeckoViewNavigation extends GeckoViewModule {
       return null;
     }
 
+    const newSessionId = Services.uuid
+      .generateUUID()
+      .toString()
+      .slice(1, -1)
+      .replace(/-/g, "");
+
     const message = {
       type: "GeckoView:OnNewSession",
       uri: aUri ? aUri.displaySpec : "",
+      newSessionId,
     };
 
+    // The window might be already open by the time we get the response from
+    // the Java layer, so we need to start waiting before sending the message.
+    const setupPromise = this.waitAndSetupWindow(
+      newSessionId,
+      aOpenWindowInfo,
+      aName
+    );
+
     let browser = undefined;
     this.eventDispatcher
       .sendRequestForResult(message)
-      .then(sessionId => {
-        return this.waitAndSetupWindow(sessionId, aOpenWindowInfo, aName);
+      .then(didOpenSession => {
+        if (!didOpenSession) {
+          return Promise.reject();
+        }
+        return setupPromise;
       })
       .then(
         window => {
-          browser = (window && window.browser) || null;
+          browser = window.browser;
         },
         () => {
           browser = null;

mobile/android/modules/geckoview/GeckoViewTab.jsm

@@ -115,29 +115,20 @@ const GeckoViewTabBridge = {
   async createNewTab({ extensionId, createProperties } = {}) {
     debug`createNewTab`;
 
-    let sessionId;
-    try {
-      sessionId = await EventDispatcher.instance.sendRequestForResult({
-        type: "GeckoView:WebExtension:NewTab",
-        extensionId,
-        createProperties,
-      });
-    } catch (errorMessage) {
-      // The error message coming from GeckoView is about :NewTab not being
-      // registered so we need to have one that's extension friendly here.
-      throw new ExtensionError("tabs.create is not supported");
-    }
+    const newSessionId = Services.uuid
+      .generateUUID()
+      .toString()
+      .slice(1, -1)
+      .replace(/-/g, "");
 
-    if (!sessionId) {
-      throw new ExtensionError("Cannot create new tab");
-    }
-
-    const window = await new Promise(resolve => {
+    // The window might already be open by the time we get the response, so we
+    // need to start waiting before we send the message.
+    const windowPromise = new Promise(resolve => {
       const handler = {
         observe(aSubject, aTopic, aData) {
           if (
             aTopic === "geckoview-window-created" &&
-            aSubject.name === sessionId
+            aSubject.name === newSessionId
           ) {
             Services.obs.removeObserver(handler, "geckoview-window-created");
             resolve(aSubject);
@@ -147,6 +138,25 @@ const GeckoViewTabBridge = {
       Services.obs.addObserver(handler, "geckoview-window-created");
     });
 
+    let didOpenSession = false;
+    try {
+      didOpenSession = await EventDispatcher.instance.sendRequestForResult({
+        type: "GeckoView:WebExtension:NewTab",
+        extensionId,
+        createProperties,
+        newSessionId,
+      });
+    } catch (errorMessage) {
+      // The error message coming from GeckoView is about :NewTab not being
+      // registered so we need to have one that's extension friendly here.
+      throw new ExtensionError("tabs.create is not supported");
+    }
+
+    if (!didOpenSession) {
+      throw new ExtensionError("Cannot create new tab");
+    }
+
+    const window = await windowPromise;
     if (!window.tab) {
       window.tab = new Tab(window);
     }