Bug 1548102 - Coordinate Page.frameNavigated and Runtime.executionContextDestroyed/Created events. r=remote-protocol-reviewers,ato

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

--HG--
extra : moz-landing-system : lando

remote/domains/ContentProcessDomain.jsm

@@ -8,7 +8,18 @@ var EXPORTED_SYMBOLS = ["ContentProcessDomain"];
 
 const {Domain} = ChromeUtils.import("chrome://remote/content/domains/Domain.jsm");
 
+ChromeUtils.defineModuleGetter(this, "ContextObserver",
+                               "chrome://remote/content/domains/ContextObserver.jsm");
+
 class ContentProcessDomain extends Domain {
+  destructor() {
+    super.destructor();
+
+    if (this._contextObserver) {
+      this._contextObserver.destructor();
+    }
+  }
+
   // helpers
 
   get content() {
@@ -22,4 +33,11 @@ class ContentProcessDomain extends Domain {
   get chromeEventHandler() {
     return this.docShell.chromeEventHandler;
   }
+
+  get contextObserver() {
+    if (!this._contextObserver) {
+      this._contextObserver = new ContextObserver(this.chromeEventHandler);
+    }
+    return this._contextObserver;
+  }
 }

remote/domains/ContextObserver.jsm

@@ -0,0 +1,103 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+/**
+ * Helper class to coordinate Runtime and Page events.
+ * Events have to be sent in the following order:
+ *  - Runtime.executionContextDestroyed
+ *  - Page.frameNavigated
+ *  - Runtime.executionContextCreated
+ *
+ * This class also handles the special case of Pages going from/to the BF cache.
+ * When you navigate to a new URL, the previous document may be stored in the BF Cache.
+ * All its asynchronous operations are frozen (XHR, timeouts, ...) and a `pagehide` event
+ * is fired for this document. We then navigate to the new URL.
+ * If the user navigates back to the previous page, the page is resurected from the
+ * cache. A `pageshow` event is fired and its asynchronous operations are resumed.
+ *
+ * When a page is in the BF Cache, we should consider it as frozen and shouldn't try
+ * to execute any javascript. So that the ExecutionContext should be considered as
+ * being destroyed and the document navigated.
+ */
+
+var EXPORTED_SYMBOLS = ["ContextObserver"];
+
+const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+const {EventEmitter} = ChromeUtils.import("resource://gre/modules/EventEmitter.jsm");
+
+class ContextObserver {
+  constructor(chromeEventHandler) {
+    this.chromeEventHandler = chromeEventHandler;
+    EventEmitter.decorate(this);
+
+    this.chromeEventHandler.addEventListener("DOMWindowCreated", this,
+      {mozSystemGroup: true});
+
+    // Listen for pageshow and pagehide to track pages going in/out to/from the BF Cache
+    this.chromeEventHandler.addEventListener("pageshow", this,
+      {mozSystemGroup: true});
+    this.chromeEventHandler.addEventListener("pagehide", this,
+      {mozSystemGroup: true});
+
+    Services.obs.addObserver(this, "inner-window-destroyed");
+  }
+
+  destructor() {
+    this.chromeEventHandler.removeEventListener("DOMWindowCreated", this,
+      {mozSystemGroup: true});
+    this.chromeEventHandler.removeEventListener("pageshow", this,
+      {mozSystemGroup: true});
+    this.chromeEventHandler.removeEventListener("pagehide", this,
+      {mozSystemGroup: true});
+    Services.obs.removeObserver(this, "inner-window-destroyed");
+  }
+
+  handleEvent({type, target, persisted}) {
+    const window = target.defaultView;
+    if (window.top != this.chromeEventHandler.ownerGlobal) {
+      // Ignore iframes for now.
+      return;
+    }
+    const { windowUtils } = window;
+    const frameId = windowUtils.outerWindowID;
+    const id = windowUtils.currentInnerWindowID;
+    switch (type) {
+    case "DOMWindowCreated":
+      // Do not pass `id` here as that's the new document ID instead of the old one
+      // that is destroyed. Instead, pass the frameId and let the listener figure out
+      // what ExecutionContext to destroy.
+      this.emit("context-destroyed", { frameId });
+      this.emit("frame-navigated", { frameId, window });
+      this.emit("context-created", { id, window });
+      break;
+    case "pageshow":
+      // `persisted` is true when this is about a page being resurected from BF Cache
+      if (!persisted) {
+        return;
+      }
+      // XXX(ochameau) we might have to emit FrameNavigate here to properly handle BF Cache
+      // scenario in Page domain events
+      this.emit("context-created", { id, window });
+      break;
+
+    case "pagehide":
+      // `persisted` is true when this is about a page being frozen into BF Cache
+      if (!persisted) {
+        return;
+      }
+      this.emit("context-destroyed", { id });
+      break;
+    }
+  }
+
+  // "inner-window-destroyed" observer service listener
+  observe(subject, topic, data) {
+    const innerWindowID = subject.QueryInterface(Ci.nsISupportsPRUint64).data;
+    this.emit("context-destroyed", { id: innerWindowID });
+  }
+}
+
+

remote/domains/content/Page.jsm

@@ -14,28 +14,21 @@ class Page extends ContentProcessDomain {
   constructor(session) {
     super(session);
     this.enabled = false;
+
+    this.onFrameNavigated = this.onFrameNavigated.bind(this);
   }
 
   destructor() {
     this.disable();
   }
 
-  QueryInterface(iid) {
-    if (iid.equals(Ci.nsIWebProgressListener) ||
-      iid.equals(Ci.nsISupportsWeakReference) ||
-      iid.equals(Ci.nsIObserver)) {
-      return this;
-    }
-    throw Cr.NS_ERROR_NO_INTERFACE;
-  }
-
   // commands
 
   async enable() {
     if (!this.enabled) {
       this.enabled = true;
-      this.chromeEventHandler.addEventListener("DOMWindowCreated", this,
-        {mozSystemGroup: true});
+      this.contextObserver.on("frame-navigated", this.onFrameNavigated);
+
       this.chromeEventHandler.addEventListener("DOMContentLoaded", this,
         {mozSystemGroup: true});
       this.chromeEventHandler.addEventListener("pageshow", this,
@@ -45,8 +38,8 @@ class Page extends ContentProcessDomain {
 
   disable() {
     if (this.enabled) {
-      this.chromeEventHandler.removeEventListener("DOMWindowCreated", this,
-        {mozSystemGroup: true});
+      this.contextObserver.off("frame-navigated", this.onFrameNavigated);
+
       this.chromeEventHandler.removeEventListener("DOMContentLoaded", this,
         {mozSystemGroup: true});
       this.chromeEventHandler.removeEventListener("pageshow", this,
@@ -100,6 +93,19 @@ class Page extends ContentProcessDomain {
     return this.content.location.href;
   }
 
+  onFrameNavigated(name, { frameId, window }) {
+    const url = window.location.href;
+    this.emit("Page.frameNavigated", {
+      frame: {
+        id: frameId,
+        // frameNavigated is only emitted for the top level document
+        // so that it never has a parent.
+        parentId: null,
+        url,
+      },
+    });
+  }
+
   handleEvent({type, target}) {
     if (target.defaultView != this.content) {
       // Ignore iframes for now
@@ -111,17 +117,6 @@ class Page extends ContentProcessDomain {
     const url = target.location.href;
 
     switch (type) {
-    case "DOMWindowCreated":
-      this.emit("Page.frameNavigated", {
-        frame: {
-          id: frameId,
-          // frameNavigated is only emitted for the top level document
-          // so that it never has a parent.
-          parentId: null,
-          url,
-        },
-      });
-      break;
     case "DOMContentLoaded":
       this.emit("Page.domContentEventFired", {timestamp});
       break;

remote/domains/content/Runtime.jsm

@@ -22,6 +22,9 @@ class Runtime extends ContentProcessDomain {
     // Map of all the ExecutionContext instances:
     // [Execution context id (Number) => ExecutionContext instance]
     this.contexts = new Map();
+
+    this.onContextCreated = this.onContextCreated.bind(this);
+    this.onContextDestroyed = this.onContextDestroyed.bind(this);
   }
 
   destructor() {
@@ -33,21 +36,16 @@ class Runtime extends ContentProcessDomain {
   async enable() {
     if (!this.enabled) {
       this.enabled = true;
-      this.chromeEventHandler.addEventListener("DOMWindowCreated", this,
-        {mozSystemGroup: true});
-
-      // Listen for pageshow and pagehide to track pages going in/out to/from the BF Cache
-      this.chromeEventHandler.addEventListener("pageshow", this,
-        {mozSystemGroup: true});
-      this.chromeEventHandler.addEventListener("pagehide", this,
-        {mozSystemGroup: true});
-
-      Services.obs.addObserver(this, "inner-window-destroyed");
+      this.contextObserver.on("context-created", this.onContextCreated);
+      this.contextObserver.on("context-destroyed", this.onContextDestroyed);
 
       // Spin the event loop in order to send the `executionContextCreated` event right
       // after we replied to `enable` request.
       Services.tm.dispatchToMainThread(() => {
-        this._createContext(this.content);
+        this.onContextCreated("context-created", {
+          id: this.content.windowUtils.currentInnerWindowID,
+          window: this.content,
+        });
       });
     }
   }
@@ -55,13 +53,8 @@ class Runtime extends ContentProcessDomain {
   disable() {
     if (this.enabled) {
       this.enabled = false;
-      this.chromeEventHandler.removeEventListener("DOMWindowCreated", this,
-        {mozSystemGroup: true});
-      this.chromeEventHandler.removeEventListener("pageshow", this,
-        {mozSystemGroup: true});
-      this.chromeEventHandler.removeEventListener("pagehide", this,
-        {mozSystemGroup: true});
-      Services.obs.removeObserver(this, "inner-window-destroyed");
+      this.contextObserver.off("context-created", this.onContextCreated);
+      this.contextObserver.off("context-destroyed", this.onContextDestroyed);
     }
   }
 
@@ -120,38 +113,13 @@ class Runtime extends ContentProcessDomain {
     return this.__debugger;
   }
 
-  handleEvent({type, target, persisted}) {
-    if (target.defaultView != this.content) {
-      // Ignore iframes for now.
-      return;
-    }
-    switch (type) {
-    case "DOMWindowCreated":
-      this._createContext(target.defaultView);
-      break;
-
-    case "pageshow":
-      // `persisted` is true when this is about a page being resurected from BF Cache
-      if (!persisted) {
-        return;
+  getContextByFrameId(frameId) {
+    for (const ctx of this.contexts.values()) {
+      if (ctx.frameId === frameId) {
+        return ctx;
       }
-      this._createContext(target.defaultView);
-      break;
-
-    case "pagehide":
-      // `persisted` is true when this is about a page being frozen into BF Cache
-      if (!persisted) {
-        return;
-      }
-      const id = target.defaultView.windowUtils.currentInnerWindowID;
-      this._destroyContext(id);
-      break;
     }
-  }
-
-  observe(subject, topic, data) {
-    const innerWindowID = subject.QueryInterface(Ci.nsISupportsPRUint64).data;
-    this._destroyContext(innerWindowID);
+    return null;
   }
 
   /**
@@ -162,9 +130,7 @@ class Runtime extends ContentProcessDomain {
    * @param {Window} window
    *     The window object of the newly instantiated document.
    */
-  _createContext(window) {
-    const { windowUtils } = window;
-    const id = windowUtils.currentInnerWindowID;
+  onContextCreated(name, { id, window }) {
     if (this.contexts.has(id)) {
       return;
     }
@@ -172,13 +138,12 @@ class Runtime extends ContentProcessDomain {
     const context = new ExecutionContext(this._debugger, window);
     this.contexts.set(id, context);
 
-    const frameId = windowUtils.outerWindowID;
     this.emit("Runtime.executionContextCreated", {
       context: {
         id,
         auxData: {
           isDefault: window == this.content,
-          frameId,
+          frameId: context.frameId,
         },
       },
     });
@@ -187,18 +152,32 @@ class Runtime extends ContentProcessDomain {
   /**
    * Helper method to destroy the ExecutionContext of the given id. Also emit
    * the related `Runtime.executionContextDestroyed` event.
+   * ContextObserver will call this method with either `id` or `frameId` argument
+   * being set.
    *
    * @param {Number} id
    *     The execution context id to destroy.
+   * @param {Number} frameId
+   *     The frame id of execution context to destroy.
+   * Eiter `id` or `frameId` is passed.
    */
-  _destroyContext(id) {
-    const context = this.contexts.get(id);
+  onContextDestroyed(name, { id, frameId }) {
+    let context;
+    if (id && frameId) {
+      throw new Error("Expects only id *or* frameId argument to be passed");
+    }
+
+    if (id) {
+      context = this.contexts.get(id);
+    } else {
+      context = this.getContextByFrameId(frameId);
+    }
 
     if (context) {
       context.destructor();
-      this.contexts.delete(id);
+      this.contexts.delete(context.id);
       this.emit("Runtime.executionContextDestroyed", {
-        executionContextId: id,
+        executionContextId: context.id,
       });
     }
   }

remote/domains/content/runtime/ExecutionContext.jsm

@@ -31,6 +31,12 @@ class ExecutionContext {
     this._debugger = dbg;
     this._debuggee = this._debugger.addDebuggee(debuggee);
 
+    // Here, we assume that debuggee is a window object and we will propably have
+    // to adapt that once we cover workers or contexts that aren't a document.
+    const { windowUtils } = debuggee;
+    this.id = windowUtils.currentInnerWindowID;
+    this.frameId = windowUtils.outerWindowID;
+
     this._remoteObjects = new Map();
   }
 

remote/jar.mn

@@ -31,6 +31,7 @@ remote.jar:
   # domains
   content/domains/ContentProcessDomain.jsm (domains/ContentProcessDomain.jsm)
   content/domains/ContentProcessDomains.jsm (domains/ContentProcessDomains.jsm)
+  content/domains/ContextObserver.jsm (domains/ContextObserver.jsm)
   content/domains/Domain.jsm (domains/Domain.jsm)
   content/domains/Domains.jsm (domains/Domains.jsm)
   content/domains/ParentProcessDomains.jsm (domains/ParentProcessDomains.jsm)

remote/test/browser/browser.ini

@@ -9,6 +9,7 @@ skip-if = debug || asan # bug 1546945
 [browser_cdp.js]
 [browser_main_target.js]
 [browser_page_frameNavigated.js]
+[browser_page_runtime_events.js]
 [browser_runtime_evaluate.js]
 [browser_runtime_callFunctionOn.js]
 [browser_runtime_executionContext.js]

remote/test/browser/browser_page_runtime_events.js

@@ -0,0 +1,115 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/* global getCDP */
+
+// Assert the order of Runtime.executionContextDestroyed, Page.frameNavigated and Runtime.executionContextCreated
+
+const TEST_URI = "data:text/html;charset=utf-8,default-test-page";
+
+add_task(async function testCDP() {
+  // Open a test page, to prevent debugging the random default page
+  await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URI);
+
+  // Start the CDP server
+  RemoteAgent.listen(Services.io.newURI("http://localhost:9222"));
+
+  // Retrieve the chrome-remote-interface library object
+  const CDP = await getCDP();
+
+  // Connect to the server
+  const client = await CDP({
+    target(list) {
+      // Ensure debugging the right target, i.e. the one for our test tab.
+      return list.find(target => {
+        return target.url == TEST_URI;
+      });
+    },
+  });
+  ok(true, "CDP client has been instantiated");
+
+  const {Page, Runtime} = client;
+
+  const events = [];
+  function assertReceivedEvents(expected, message) {
+    Assert.deepEqual(events, expected, message);
+    // Empty the list of received events
+    events.splice(0);
+  }
+  Page.frameNavigated(() => {
+    events.push("frameNavigated");
+  });
+  Runtime.executionContextCreated(() => {
+    events.push("executionContextCreated");
+  });
+  Runtime.executionContextDestroyed(() => {
+    events.push("executionContextDestroyed");
+  });
+
+  // turn on navigation related events, such as DOMContentLoaded et al.
+  await Page.enable();
+  ok(true, "Page domain has been enabled");
+
+  const onExecutionContextCreated = Runtime.executionContextCreated();
+  await Runtime.enable();
+  ok(true, "Runtime domain has been enabled");
+
+  // Runtime.enable will dispatch `executionContextCreated` for the existing document
+  let { context } = await onExecutionContextCreated;
+  ok(!!context.id, "The execution context has an id");
+  ok(context.auxData.isDefault, "The execution context is the default one");
+  ok(!!context.auxData.frameId, "The execution context has a frame id set");
+
+  assertReceivedEvents(["executionContextCreated"], "Received only executionContextCreated event after Runtime.enable call");
+
+  const { frameTree } = await Page.getFrameTree();
+  ok(!!frameTree.frame, "getFrameTree exposes one frame");
+  is(frameTree.childFrames.length, 0, "getFrameTree reports no child frame");
+  ok(!!frameTree.frame.id, "getFrameTree's frame has an id");
+  is(frameTree.frame.url, TEST_URI, "getFrameTree's frame has the right url");
+  is(frameTree.frame.id, context.auxData.frameId, "getFrameTree and executionContextCreated refers about the same frame Id");
+
+  const onFrameNavigated = Page.frameNavigated();
+  const onExecutionContextDestroyed = Runtime.executionContextDestroyed();
+  const onExecutionContextCreated2 = Runtime.executionContextCreated();
+  const url = "data:text/html;charset=utf-8,test-page";
+  const { frameId } = await Page.navigate({ url  });
+  ok(true, "A new page has been loaded");
+  ok(frameId, "Page.navigate returned a frameId");
+  is(frameId, frameTree.frame.id, "The Page.navigate's frameId is the same than " +
+    "getFrameTree's one");
+
+  const frameNavigated = await onFrameNavigated;
+  ok(!frameNavigated.frame.parentId, "frameNavigated is for the top level document and" +
+    " has a null parentId");
+  is(frameNavigated.frame.id, frameId, "frameNavigated id is the same than the one " +
+    "returned by Page.navigate");
+  is(frameNavigated.frame.name, undefined, "frameNavigated name isn't implemented yet");
+  is(frameNavigated.frame.url, url, "frameNavigated url is the same being given to " +
+    "Page.navigate");
+
+  const { executionContextId } = await onExecutionContextDestroyed;
+  ok(executionContextId, "The destroyed event reports an id");
+  is(executionContextId, context.id, "The destroyed event is for the first reported execution context");
+
+  ({ context } = await onExecutionContextCreated2);
+  ok(!!context.id, "The execution context has an id");
+  ok(context.auxData.isDefault, "The execution context is the default one");
+  is(context.auxData.frameId, frameId, "The execution context frame id is the same " +
+    "the one returned by Page.navigate");
+
+  isnot(executionContextId, context.id, "The destroyed id is different from the " +
+    "created one");
+
+  assertReceivedEvents(["executionContextDestroyed", "frameNavigated", "executionContextCreated"],
+    "Received frameNavigated between the two execution context events during navigation to another URL");
+
+  await client.close();
+  ok(true, "The client is closed");
+
+  BrowserTestUtils.removeTab(gBrowser.selectedTab);
+
+  await RemoteAgent.close();
+});