Net internals: Use sendwithPromise

Remove use of CallJavascriptFunctionUnsafe from net_internals_ui.cc.

Update BrowserBridge to class based syntax and use sendWithPromise as
needed.

handler occurs like in production code, so cannot easily be stubbed
out with a TestBrowserProxy. Therefore, moved test observers to the
class being tested.

Note: Unlike many WebUI tests, these tests assume a round trip to the
Change-Id: Id2453205e1bdc912b5e20aeba12f1082ed09629d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2634129
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845033}

chrome/browser/resources/net_internals/browser_bridge.js

@@ -6,155 +6,59 @@
  * This class provides a "bridge" for communicating between the javascript and
  * the browser.
  */
-const BrowserBridge = (function() {
-  'use strict';
-
-  /**
-   * @constructor
-   */
-  function BrowserBridge() {
-    assertFirstConstructorCall(BrowserBridge);
-
-    // List of observers for various bits of browser state.
-    this.hstsObservers_ = [];
-    this.expectCTObservers_ = [];
-    this.setNetworkDebugModeObservers_ = [];
+class BrowserBridge {
+  constructor() {}
+
+  //--------------------------------------------------------------------------
+  // Messages sent to the browser
+  //--------------------------------------------------------------------------
+  sendReloadProxySettings() {
+    chrome.send('reloadProxySettings');
   }
 
-  cr.addSingletonGetter(BrowserBridge);
-
-  BrowserBridge.prototype = {
-
-    //--------------------------------------------------------------------------
-    // Messages sent to the browser
-    //--------------------------------------------------------------------------
-
-    /**
-     * Wraps |chrome.send|.
-     * TODO(mattm): remove this and switch things to use chrome.send directly.
-     */
-    send(value1, value2) {
-      if (arguments.length == 1) {
-        chrome.send(value1);
-      } else if (arguments.length == 2) {
-        chrome.send(value1, value2);
-      } else {
-        throw 'Unsupported number of arguments.';
-      }
-    },
-
-    sendReloadProxySettings() {
-      this.send('reloadProxySettings');
-    },
-
-    sendClearBadProxies() {
-      this.send('clearBadProxies');
-    },
-
-    sendClearHostResolverCache() {
-      this.send('clearHostResolverCache');
-    },
-
-    sendHSTSQuery(domain) {
-      this.send('hstsQuery', [domain]);
-    },
-
-    sendHSTSAdd(domain, sts_include_subdomains) {
-      this.send('hstsAdd', [domain, sts_include_subdomains]);
-    },
-
-    sendDomainSecurityPolicyDelete(domain) {
-      this.send('domainSecurityPolicyDelete', [domain]);
-    },
-
-    sendExpectCTQuery(domain) {
-      this.send('expectCTQuery', [domain]);
-    },
-
-    sendExpectCTAdd(domain, report_uri, enforce) {
-      this.send('expectCTAdd', [domain, report_uri, enforce]);
-    },
-
-    sendExpectCTTestReport(report_uri) {
-      this.send('expectCTTestReport', [report_uri]);
-    },
-
-    sendCloseIdleSockets() {
-      this.send('closeIdleSockets');
-    },
-
-    sendFlushSocketPools() {
-      this.send('flushSocketPools');
-    },
-
-    setNetworkDebugMode(subsystem) {
-      this.send('setNetworkDebugMode', [subsystem]);
-    },
+  sendClearBadProxies() {
+    chrome.send('clearBadProxies');
+  }
 
-    //--------------------------------------------------------------------------
-    // Messages received from the browser.
-    //--------------------------------------------------------------------------
+  sendClearHostResolverCache() {
+    chrome.send('clearHostResolverCache');
+  }
 
-    receive(command, params) {
-      this[command](params);
-    },
+  sendHSTSQuery(domain) {
+    return cr.sendWithPromise('hstsQuery', domain);
+  }
 
-    receivedHSTSResult(info) {
-      for (let i = 0; i < this.hstsObservers_.length; i++) {
-        this.hstsObservers_[i].onHSTSQueryResult(info);
-      }
-    },
+  sendHSTSAdd(domain, sts_include_subdomains) {
+    chrome.send('hstsAdd', [domain, sts_include_subdomains]);
+  }
 
-    receivedExpectCTResult(info) {
-      for (let i = 0; i < this.expectCTObservers_.length; i++) {
-        this.expectCTObservers_[i].onExpectCTQueryResult(info);
-      }
-    },
+  sendDomainSecurityPolicyDelete(domain) {
+    chrome.send('domainSecurityPolicyDelete', [domain]);
+  }
 
-    receivedExpectCTTestReportResult(result) {
-      for (let i = 0; i < this.expectCTObservers_.length; i++) {
-        this.expectCTObservers_[i].onExpectCTTestReportResult(result);
-      }
-    },
+  sendExpectCTQuery(domain) {
+    return cr.sendWithPromise('expectCTQuery', domain);
+  }
 
-    receivedSetNetworkDebugMode(status) {
-      for (let i = 0; i < this.setNetworkDebugModeObservers_.length; i++) {
-        this.setNetworkDebugModeObservers_[i].onSetNetworkDebugMode(status);
-      }
-    },
+  sendExpectCTAdd(domain, report_uri, enforce) {
+    chrome.send('expectCTAdd', [domain, report_uri, enforce]);
+  }
 
-    //--------------------------------------------------------------------------
+  sendExpectCTTestReport(report_uri) {
+    return cr.sendWithPromise('expectCTTestReport', report_uri);
+  }
 
-    /**
-     * Adds a listener for the results of HSTS (HTTPS Strict Transport Security)
-     * queries. The observer will be called back with:
-     *
-     *   observer.onHSTSQueryResult(result);
-     */
-    addHSTSObserver(observer) {
-      this.hstsObservers_.push(observer);
-    },
+  sendCloseIdleSockets() {
+    chrome.send('closeIdleSockets');
+  }
 
-    /**
-     * Adds a listener for the results of Expect-CT queries. The observer will
-     * be called back with:
-     *
-     *   observer.onExpectCTQueryResult(result);
-     */
-    addExpectCTObserver(observer) {
-      this.expectCTObservers_.push(observer);
-    },
+  sendFlushSocketPools() {
+    chrome.send('flushSocketPools');
+  }
 
-    /**
-     * Adds a listener for network debugging mode status. The observer
-     * will be called back with:
-     *
-     *   observer.onSetNetworkDebugMode(status);
-     */
-    addSetNetworkDebugModeObserver(observer) {
-      this.setNetworkDebugModeObservers_.push(observer);
-    },
-  };
+  setNetworkDebugMode(subsystem) {
+    chrome.send('setNetworkDebugMode', [subsystem]);
+  }
+}
 
-  return BrowserBridge;
-})();
+cr.addSingletonGetter(BrowserBridge);

chrome/browser/resources/net_internals/dns_view.js

@@ -22,8 +22,9 @@ const DnsView = (function() {
     // Call superclass's constructor.
     superClass.call(this, DnsView.MAIN_BOX_ID);
 
-    $(DnsView.CLEAR_CACHE_BUTTON_ID).onclick =
-        g_browser.sendClearHostResolverCache.bind(g_browser);
+    $(DnsView.CLEAR_CACHE_BUTTON_ID).onclick = () => {
+      BrowserBridge.getInstance().sendClearHostResolverCache();
+    };
   }
 
   DnsView.TAB_ID = 'tab-handle-dns';

chrome/browser/resources/net_internals/index.html

@@ -15,6 +15,7 @@ found in the LICENSE file.
     <link rel="stylesheet" href="chromeos_view.css">
     <script src="chrome://resources/js/assert.js"></script>
     <script src="chrome://resources/js/util.js"></script>
+    <script src="chrome://resources/js/promise_resolver.js"></script>
     <script src="chrome://resources/js/cr.js"></script>
     <script src="chrome://net-internals/index.js"></script>
   </head>

chrome/browser/resources/net_internals/main.js

@@ -2,12 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-/**
- * Object to communicate between the renderer and the browser.
- * @type {!BrowserBridge}
- */
-let g_browser = null;
-
 /**
  * This class is the root view object of the page.  It owns all the other
  * views, and manages switching between them.  It is also responsible for
@@ -30,10 +24,6 @@ const MainView = (function() {
       document.body.classList.add('touch');
     }
 
-    // This must be initialized before the tabs, so they can register as
-    // observers.
-    g_browser = BrowserBridge.getInstance();
-
     // Create the tab switcher.
     this.initTabs_();
     superClass.call(this, this.tabSwitcher_);

chrome/browser/resources/net_internals/proxy_view.js

@@ -24,10 +24,12 @@ const ProxyView = (function() {
     superClass.call(this, ProxyView.MAIN_BOX_ID);
 
     // Hook up the UI components.
-    $(ProxyView.RELOAD_SETTINGS_BUTTON_ID).onclick =
-        g_browser.sendReloadProxySettings.bind(g_browser);
-    $(ProxyView.CLEAR_BAD_PROXIES_BUTTON_ID).onclick =
-        g_browser.sendClearBadProxies.bind(g_browser);
+    $(ProxyView.RELOAD_SETTINGS_BUTTON_ID).onclick = () => {
+      BrowserBridge.getInstance().sendReloadProxySettings();
+    };
+    $(ProxyView.CLEAR_BAD_PROXIES_BUTTON_ID).onclick = () => {
+      BrowserBridge.getInstance().sendClearBadProxies();
+    };
   }
 
   ProxyView.TAB_ID = 'tab-handle-proxy';

chrome/browser/resources/net_internals/sockets_view.js

@@ -46,11 +46,11 @@ const SocketsView = (function() {
     __proto__: superClass.prototype,
 
     closeIdleSockets() {
-      g_browser.sendCloseIdleSockets();
+      BrowserBridge.getInstance().sendCloseIdleSockets();
     },
 
     flushSocketPools() {
-      g_browser.sendFlushSocketPools();
+      BrowserBridge.getInstance().sendFlushSocketPools();
     }
   };
 

chrome/browser/ui/webui/net_internals/net_internals_ui.cc

@@ -67,11 +67,13 @@ class NetInternalsMessageHandler
  private:
   network::mojom::NetworkContext* GetNetworkContext();
 
-  // Calls g_browser.receive in the renderer, passing in |command| and |arg|.
+  // Resolve JS |callback_id| with |result|.
   // If the renderer is displaying a log file, the message will be ignored.
-  void SendJavascriptCommand(const std::string& command, base::Value arg);
+  void ResolveCallbackWithResult(const std::string& callback_id,
+                                 base::Value result);
 
-  void OnExpectCTTestReportCallback(bool success);
+  void OnExpectCTTestReportCallback(const std::string& callback_id,
+                                    bool success);
 
   //--------------------------------
   // Javascript message handlers:
@@ -145,15 +147,6 @@ void NetInternalsMessageHandler::RegisterMessages() {
                           base::Unretained(this)));
 }
 
-void NetInternalsMessageHandler::SendJavascriptCommand(
-    const std::string& command,
-    base::Value arg) {
-  std::unique_ptr<base::Value> command_value(new base::Value(command));
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  web_ui()->CallJavascriptFunctionUnsafe("g_browser.receive",
-                                         *command_value.get(), arg);
-}
-
 void NetInternalsMessageHandler::OnReloadProxySettings(
     const base::ListValue* list) {
   GetNetworkContext()->ForceReloadProxyConfig(base::NullCallback());
@@ -184,14 +177,23 @@ void NetInternalsMessageHandler::OnDomainSecurityPolicyDelete(
 }
 
 void NetInternalsMessageHandler::OnHSTSQuery(const base::ListValue* list) {
-  // |list| should be: [<domain to query>].
+  std::string callback_id;
+  bool get_callback_id = list->GetString(0, &callback_id);
   std::string domain;
-  bool get_domain_result = list->GetString(0, &domain);
-  DCHECK(get_domain_result);
+  bool get_domain_result = list->GetString(1, &domain);
+  DCHECK(get_domain_result && get_callback_id);
 
+  AllowJavascript();
   GetNetworkContext()->GetHSTSState(
-      domain, base::BindOnce(&NetInternalsMessageHandler::SendJavascriptCommand,
-                             this->AsWeakPtr(), "receivedHSTSResult"));
+      domain,
+      base::BindOnce(&NetInternalsMessageHandler::ResolveCallbackWithResult,
+                     base::Unretained(this), callback_id));
+}
+
+void NetInternalsMessageHandler::ResolveCallbackWithResult(
+    const std::string& callback_id,
+    base::Value result) {
+  ResolveJavascriptCallback(base::Value(callback_id), result);
 }
 
 void NetInternalsMessageHandler::OnHSTSAdd(const base::ListValue* list) {
@@ -214,19 +216,22 @@ void NetInternalsMessageHandler::OnHSTSAdd(const base::ListValue* list) {
 }
 
 void NetInternalsMessageHandler::OnExpectCTQuery(const base::ListValue* list) {
-  // |list| should be: [<domain to query>].
+  std::string callback_id;
   std::string domain;
-  bool result = list->GetString(0, &domain);
-  DCHECK(result);
+  bool callback_result = list->GetString(0, &callback_id);
+  bool result = list->GetString(1, &domain);
+
+  DCHECK(result && callback_result);
 
   url::Origin origin = url::Origin::Create(GURL("https://" + domain));
+  AllowJavascript();
 
   GetNetworkContext()->GetExpectCTState(
       domain,
       net::NetworkIsolationKey(origin /* top_frame_site */,
                                origin /* frame_site */),
-      base::BindOnce(&NetInternalsMessageHandler::SendJavascriptCommand,
-                     this->AsWeakPtr(), "receivedExpectCTResult"));
+      base::BindOnce(&NetInternalsMessageHandler::ResolveCallbackWithResult,
+                     base::Unretained(this), callback_id));
 }
 
 void NetInternalsMessageHandler::OnExpectCTAdd(const base::ListValue* list) {
@@ -259,24 +264,29 @@ void NetInternalsMessageHandler::OnExpectCTAdd(const base::ListValue* list) {
 
 void NetInternalsMessageHandler::OnExpectCTTestReport(
     const base::ListValue* list) {
-  // |list| should be: [<report URI>].
+  std::string callback_id;
   std::string report_uri_str;
-  bool result = list->GetString(0, &report_uri_str);
-  DCHECK(result);
+  bool callback_result = list->GetString(0, &callback_id);
+  bool result = list->GetString(1, &report_uri_str);
+  DCHECK(result && callback_result);
   GURL report_uri(report_uri_str);
-  if (!report_uri.is_valid())
+  AllowJavascript();
+  if (!report_uri.is_valid()) {
+    ResolveCallbackWithResult(callback_id, base::Value("invalid"));
     return;
+  }
 
   GetNetworkContext()->SetExpectCTTestReport(
       report_uri,
       base::BindOnce(&NetInternalsMessageHandler::OnExpectCTTestReportCallback,
-                     this->AsWeakPtr()));
+                     base::Unretained(this), callback_id));
 }
 
-void NetInternalsMessageHandler::OnExpectCTTestReportCallback(bool success) {
-  SendJavascriptCommand(
-      "receivedExpectCTTestReportResult",
-      success ? base::Value("success") : base::Value("failure"));
+void NetInternalsMessageHandler::OnExpectCTTestReportCallback(
+    const std::string& callback_id,
+    bool success) {
+  ResolveCallbackWithResult(
+      callback_id, success ? base::Value("success") : base::Value("failure"));
 }
 
 void NetInternalsMessageHandler::OnFlushSocketPools(

chrome/test/data/webui/net_internals/domain_security_policy_view.js

@@ -121,7 +121,7 @@ CheckHSTSQueryResultTask.prototype = {
    * Starts watching for the query results.
    */
   start: function() {
-    g_browser.addHSTSObserver(this);
+    DomainSecurityPolicyView.getInstance().addHSTSObserverForTest(this);
   },
 
   /**
@@ -254,7 +254,7 @@ CheckExpectCTQueryResultTask.prototype = {
    * Starts watching for the query results.
    */
   start: function() {
-    g_browser.addExpectCTObserver(this);
+    DomainSecurityPolicyView.getInstance().addExpectCTObserverForTest(this);
   },
 
   /**
@@ -392,7 +392,7 @@ SendTestReportTask.prototype = {
    * Sends the test report and starts watching for the result.
    */
   start: function() {
-    g_browser.addExpectCTObserver(this);
+    DomainSecurityPolicyView.getInstance().addExpectCTObserverForTest(this);
     $(DomainSecurityPolicyView.TEST_REPORT_EXPECT_CT_INPUT_ID).value =
         this.reportURITask_.reportURI();
     $(DomainSecurityPolicyView.TEST_REPORT_EXPECT_CT_SUBMIT_ID).click();

chrome/test/data/webui/net_internals/net_internals_test.js

@@ -50,11 +50,6 @@ var NetInternalsTest = (function() {
     setUp: function() {
       testing.Test.prototype.setUp.call(this);
 
-      // Wrap g_browser.receive around a test function so that assert and expect
-      // functions can be called from observers.
-      g_browser.receive = this.continueTest(
-          WhenTestDone.EXPECT, BrowserBridge.prototype.receive.bind(g_browser));
-
       var runTest = this.deferRunTest(WhenTestDone.EXPECT);
       window.setTimeout(runTest, 0);
     }