Bug 1404929 - Security info should be loaded lazily;r=honza draft
authorFred Lin <gasolin@gmail.com>
Wed, 15 Nov 2017 12:50:47 +0800
changeset 702366 2ce4b19a1d31e6efe54f5cdb93d51c2419254a07
parent 702247 960f50c2e0a991ab2ab313132e69fb2c96cb7866
child 741437 411952c6aa9d7e401f6dfdda78ff881be8b4eee4
push id90451
push userbmo:gasolin@mozilla.com
push dateThu, 23 Nov 2017 02:13:39 +0000
reviewershonza
bugs1404929
milestone59.0a1
Bug 1404929 - Security info should be loaded lazily;r=honza MozReview-Commit-ID: JIwepd7qdOB
devtools/client/netmonitor/src/components/SecurityPanel.js
devtools/client/netmonitor/src/components/TabboxPanel.js
devtools/client/netmonitor/src/connector/firefox-data-provider.js
devtools/client/netmonitor/test/browser_net_security-details.js
devtools/client/netmonitor/test/browser_net_security-error.js
devtools/client/netmonitor/test/browser_net_security-icon-click.js
devtools/client/netmonitor/test/browser_net_security-tab-visibility.js
devtools/client/netmonitor/test/shared-head.js
--- a/devtools/client/netmonitor/src/components/SecurityPanel.js
+++ b/devtools/client/netmonitor/src/components/SecurityPanel.js
@@ -1,15 +1,18 @@
 /* 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";
 
-const { createFactory } = require("devtools/client/shared/vendor/react");
+const {
+  Component,
+  createFactory,
+} = require("devtools/client/shared/vendor/react");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const { L10N } = require("../utils/l10n");
 const { getUrlHost } = require("../utils/request-utils");
 
 // Components
 const TreeViewClass = require("devtools/client/shared/components/tree/TreeView");
 const PropertiesView = createFactory(require("./PropertiesView"));
@@ -42,133 +45,162 @@ const SHA256_FINGERPRINT_LABEL =
 const SHA1_FINGERPRINT_LABEL = L10N.getStr("certmgr.certdetail.sha1fingerprint");
 
 /*
  * Security panel component
  * If the site is being served over HTTPS, you get an extra tab labeled "Security".
  * This contains details about the secure connection used including the protocol,
  * the cipher suite, and certificate details
  */
-function SecurityPanel({
-  openLink,
-  request,
-}) {
-  const { securityInfo, url } = request;
-
-  if (!securityInfo || !url) {
-    return null;
-  }
-
-  let object;
-
-  if (securityInfo.state === "secure" || securityInfo.state === "weak") {
-    const { subject, issuer, validity, fingerprint } = securityInfo.cert;
-    const HOST_HEADER_LABEL = L10N.getFormatStr("netmonitor.security.hostHeader",
-      getUrlHost(url));
-
-    object = {
-      [CONNECTION_LABEL]: {
-        [PROTOCOL_VERSION_LABEL]:
-          securityInfo.protocolVersion || NOT_AVAILABLE,
-        [CIPHER_SUITE_LABEL]:
-          securityInfo.cipherSuite || NOT_AVAILABLE,
-        [KEA_GROUP_LABEL]:
-          securityInfo.keaGroupName || NOT_AVAILABLE,
-        [SIGNATURE_SCHEME_LABEL]:
-          securityInfo.signatureSchemeName || NOT_AVAILABLE,
-      },
-      [HOST_HEADER_LABEL]: {
-        [HSTS_LABEL]:
-          securityInfo.hsts ? ENABLED_LABEL : DISABLED_LABEL,
-        [HPKP_LABEL]:
-          securityInfo.hpkp ? ENABLED_LABEL : DISABLED_LABEL,
-      },
-      [CERTIFICATE_LABEL]: {
-        [SUBJECT_INFO_LABEL]: {
-          [CERT_DETAIL_COMMON_NAME_LABEL]:
-            subject.commonName || NOT_AVAILABLE,
-          [CERT_DETAIL_ORG_LABEL]:
-            subject.organization || NOT_AVAILABLE,
-          [CERT_DETAIL_ORG_UNIT_LABEL]:
-            subject.organizationUnit || NOT_AVAILABLE,
-        },
-        [ISSUER_INFO_LABEL]: {
-          [CERT_DETAIL_COMMON_NAME_LABEL]:
-            issuer.commonName || NOT_AVAILABLE,
-          [CERT_DETAIL_ORG_LABEL]:
-            issuer.organization || NOT_AVAILABLE,
-          [CERT_DETAIL_ORG_UNIT_LABEL]:
-            issuer.organizationUnit || NOT_AVAILABLE,
-        },
-        [PERIOD_OF_VALIDITY_LABEL]: {
-          [BEGINS_LABEL]:
-            validity.start || NOT_AVAILABLE,
-          [EXPIRES_LABEL]:
-            validity.end || NOT_AVAILABLE,
-        },
-        [FINGERPRINTS_LABEL]: {
-          [SHA256_FINGERPRINT_LABEL]:
-            fingerprint.sha256 || NOT_AVAILABLE,
-          [SHA1_FINGERPRINT_LABEL]:
-            fingerprint.sha1 || NOT_AVAILABLE,
-        },
-      },
-    };
-  } else {
-    object = {
-      [ERROR_LABEL]:
-        new DOMParser().parseFromString(securityInfo.errorMessage, "text/html")
-          .body.textContent || NOT_AVAILABLE
+class SecurityPanel extends Component {
+  static get propTypes() {
+    return {
+      connector: PropTypes.object.isRequired,
+      openLink: PropTypes.func,
+      request: PropTypes.object.isRequired,
     };
   }
 
-  return div({ className: "panel-container security-panel" },
-    PropertiesView({
-      object,
-      renderValue: (props) => renderValue(props, securityInfo.weaknessReasons),
-      enableFilter: false,
-      expandedNodes: TreeViewClass.getExpandedNodes(object),
-      openLink,
-    })
-  );
-}
+  /**
+   * `componentDidMount` is called when opening the SecurityPanel for the first time
+   */
+  componentDidMount() {
+    this.maybeFetchSecurityInfo(this.props);
+  }
+
+  /**
+   * `componentWillReceiveProps` is the only method called when switching between two
+   * requests while the security panel is displayed.
+   */
+  componentWillReceiveProps(nextProps) {
+    this.maybeFetchSecurityInfo(nextProps);
+  }
+
+  /**
+   * When switching to another request, lazily fetch securityInfo
+   * from the backend. The Security Panel will first be empty and then
+   * display the content.
+   */
+  maybeFetchSecurityInfo(props) {
+    if (!props.request.securityInfo) {
+      // This method will set `props.request.securityInfo`
+      // asynchronously and force another render.
+      props.connector.requestData(props.request.id, "securityInfo");
+    }
+  }
 
-SecurityPanel.displayName = "SecurityPanel";
+  renderValue(props, weaknessReasons = []) {
+    const { member, value } = props;
 
-SecurityPanel.propTypes = {
-  request: PropTypes.object.isRequired,
-  openLink: PropTypes.func,
-};
+    // Hide object summary
+    if (typeof member.value === "object") {
+      return null;
+    }
 
-function renderValue(props, weaknessReasons = []) {
-  const { member, value } = props;
-
-  // Hide object summary
-  if (typeof member.value === "object") {
-    return null;
+    return span({ className: "security-info-value" },
+      member.name === ERROR_LABEL ?
+        // Display multiline text for security error
+        value
+        :
+        // Display one line selectable text for security details
+        input({
+          className: "textbox-input",
+          readOnly: "true",
+          value,
+        })
+      ,
+      weaknessReasons.indexOf("cipher") !== -1 &&
+      member.name === CIPHER_SUITE_LABEL ?
+        // Display an extra warning icon after the cipher suite
+        div({
+          id: "security-warning-cipher",
+          className: "security-warning-icon",
+          title: WARNING_CIPHER_LABEL,
+        })
+        :
+        null
+    );
   }
 
-  return span({ className: "security-info-value" },
-    member.name === ERROR_LABEL ?
-      // Display multiline text for security error
-      value
-      :
-      // Display one line selectable text for security details
-      input({
-        className: "textbox-input",
-        readOnly: "true",
-        value,
+  render() {
+    let { openLink, request } = this.props;
+    const { securityInfo, url } = request;
+
+    if (!securityInfo || !url) {
+      return null;
+    }
+
+    let object;
+
+    if (securityInfo.state === "secure" || securityInfo.state === "weak") {
+      const { subject, issuer, validity, fingerprint } = securityInfo.cert;
+      const HOST_HEADER_LABEL = L10N.getFormatStr("netmonitor.security.hostHeader",
+        getUrlHost(url));
+
+      object = {
+        [CONNECTION_LABEL]: {
+          [PROTOCOL_VERSION_LABEL]:
+            securityInfo.protocolVersion || NOT_AVAILABLE,
+          [CIPHER_SUITE_LABEL]:
+            securityInfo.cipherSuite || NOT_AVAILABLE,
+          [KEA_GROUP_LABEL]:
+            securityInfo.keaGroupName || NOT_AVAILABLE,
+          [SIGNATURE_SCHEME_LABEL]:
+            securityInfo.signatureSchemeName || NOT_AVAILABLE,
+        },
+        [HOST_HEADER_LABEL]: {
+          [HSTS_LABEL]:
+            securityInfo.hsts ? ENABLED_LABEL : DISABLED_LABEL,
+          [HPKP_LABEL]:
+            securityInfo.hpkp ? ENABLED_LABEL : DISABLED_LABEL,
+        },
+        [CERTIFICATE_LABEL]: {
+          [SUBJECT_INFO_LABEL]: {
+            [CERT_DETAIL_COMMON_NAME_LABEL]:
+              subject.commonName || NOT_AVAILABLE,
+            [CERT_DETAIL_ORG_LABEL]:
+              subject.organization || NOT_AVAILABLE,
+            [CERT_DETAIL_ORG_UNIT_LABEL]:
+              subject.organizationUnit || NOT_AVAILABLE,
+          },
+          [ISSUER_INFO_LABEL]: {
+            [CERT_DETAIL_COMMON_NAME_LABEL]:
+              issuer.commonName || NOT_AVAILABLE,
+            [CERT_DETAIL_ORG_LABEL]:
+              issuer.organization || NOT_AVAILABLE,
+            [CERT_DETAIL_ORG_UNIT_LABEL]:
+              issuer.organizationUnit || NOT_AVAILABLE,
+          },
+          [PERIOD_OF_VALIDITY_LABEL]: {
+            [BEGINS_LABEL]:
+              validity.start || NOT_AVAILABLE,
+            [EXPIRES_LABEL]:
+              validity.end || NOT_AVAILABLE,
+          },
+          [FINGERPRINTS_LABEL]: {
+            [SHA256_FINGERPRINT_LABEL]:
+              fingerprint.sha256 || NOT_AVAILABLE,
+            [SHA1_FINGERPRINT_LABEL]:
+              fingerprint.sha1 || NOT_AVAILABLE,
+          },
+        },
+      };
+    } else {
+      object = {
+        [ERROR_LABEL]:
+          new DOMParser().parseFromString(securityInfo.errorMessage, "text/html")
+            .body.textContent || NOT_AVAILABLE
+      };
+    }
+
+    return div({ className: "panel-container security-panel" },
+      PropertiesView({
+        object,
+        renderValue: (props) => this.renderValue(props, securityInfo.weaknessReasons),
+        enableFilter: false,
+        expandedNodes: TreeViewClass.getExpandedNodes(object),
+        openLink,
       })
-    ,
-    weaknessReasons.indexOf("cipher") !== -1 &&
-    member.name === CIPHER_SUITE_LABEL ?
-      // Display an extra warning icon after the cipher suite
-      div({
-        id: "security-warning-cipher",
-        className: "security-warning-icon",
-        title: WARNING_CIPHER_LABEL,
-      })
-      :
-      null
-  );
+    );
+  }
 }
 
 module.exports = SecurityPanel;
--- a/devtools/client/netmonitor/src/components/TabboxPanel.js
+++ b/devtools/client/netmonitor/src/components/TabboxPanel.js
@@ -91,17 +91,21 @@ function TabboxPanel({
       },
         StackTracePanel({ connector, openLink, request, sourceMapService }),
       ),
       request.securityState && request.securityState !== "insecure" &&
       TabPanel({
         id: PANELS.SECURITY,
         title: SECURITY_TITLE,
       },
-        SecurityPanel({ request, openLink }),
+        SecurityPanel({
+          connector,
+          openLink,
+          request,
+        }),
       ),
     )
   );
 }
 
 TabboxPanel.displayName = "TabboxPanel";
 
 TabboxPanel.propTypes = {
--- a/devtools/client/netmonitor/src/connector/firefox-data-provider.js
+++ b/devtools/client/netmonitor/src/connector/firefox-data-provider.js
@@ -252,24 +252,21 @@ class FirefoxDataProvider {
       return false;
     }
 
     let { payload } = this.getRequestFromQueue(id);
 
     // The payload is ready when all values in the record are true. (i.e. all data
     // received, but the lazy one. responseContent is the only one for now).
     // Note that we never fetch response header/cookies for request with security issues.
-    // (Be careful, securityState can be undefined, for example for WebSocket requests)
-    // Also note that service worker don't have security info set.
     // Bug 1404917 should simplify this heuristic by making all these field be lazily
     // fetched, only on-demand.
     return record.requestHeaders &&
       record.requestCookies &&
       record.eventTimings &&
-      (record.securityInfo || record.fromServiceWorker) &&
       (
        (record.responseHeaders && record.responseCookies) ||
        payload.securityState == "broken" ||
        (payload.responseContentAvailable && !payload.status)
       );
   }
 
   /**
@@ -339,22 +336,17 @@ class FirefoxDataProvider {
     } = networkInfo;
 
     // Create tracking record for this request.
     this.rdpRequestMap.set(actor, {
       requestHeaders: false,
       requestCookies: false,
       responseHeaders: false,
       responseCookies: false,
-      securityInfo: false,
       eventTimings: false,
-
-      // This isn't a request data, but we need to know about request being served from
-      // service worker later, from isRequestPayloadReady.
-      fromServiceWorker,
     });
 
     this.addRequest(actor, {
       cause,
       fromCache,
       fromServiceWorker,
       isXHR,
       method,
@@ -386,22 +378,20 @@ class FirefoxDataProvider {
     switch (updateType) {
       case "requestHeaders":
       case "requestCookies":
       case "requestPostData":
       case "responseHeaders":
       case "responseCookies":
         this.requestPayloadData(actor, updateType);
         break;
+      // (Be careful, securityState can be undefined, for example for WebSocket requests)
+      // Also note that service worker don't have security info set.
       case "securityInfo":
-        this.updateRequest(actor, {
-          securityState: networkInfo.securityInfo,
-        }).then(() => {
-          this.requestPayloadData(actor, updateType);
-        });
+        this.updateRequest(actor, { securityState: networkInfo.securityInfo });
         break;
       case "responseStart":
         this.updateRequest(actor, {
           httpVersion: networkInfo.response.httpVersion,
           remoteAddress: networkInfo.response.remoteAddress,
           remotePort: networkInfo.response.remotePort,
           status: networkInfo.response.status,
           statusText: networkInfo.response.statusText,
@@ -417,20 +407,18 @@ class FirefoxDataProvider {
           mimeType: networkInfo.response.content.mimeType,
 
           // This field helps knowing when/if responseContent property is available
           // and can be requested via `requestData`
           responseContentAvailable: true,
         });
         break;
       case "eventTimings":
-        this.updateRequest(actor, { totalTime: networkInfo.totalTime })
-          .then(() => {
-            this.requestPayloadData(actor, updateType);
-          });
+        this.pushRequestToQueue(actor, { totalTime: networkInfo.totalTime });
+        this.requestPayloadData(actor, updateType);
         break;
     }
 
     emit(EVENTS.NETWORK_EVENT_UPDATED, actor);
   }
 
   /**
    * Wrapper method for requesting HTTP details data for the payload.
@@ -486,17 +474,17 @@ class FirefoxDataProvider {
       // from `onNetworkEventUpdate`. There should be no more RDP requests after this.
       emit(EVENTS.PAYLOAD_READY, actor);
     }
   }
 
   /**
    * Public connector API to lazily request HTTP details from the backend.
    *
-   * This is internal method that focus on:
+   * The method focus on:
    * - calling the right actor method,
    * - emitting an event to tell we start fetching some request data,
    * - call data processing method.
    *
    * @param {string} actor actor id (used as request id)
    * @param {string} method identifier of the data we want to fetch
    *
    * @return {Promise} return a promise resolved when data is received.
--- a/devtools/client/netmonitor/test/browser_net_security-details.js
+++ b/devtools/client/netmonitor/test/browser_net_security-details.js
@@ -17,22 +17,22 @@ add_task(function* () {
   info("Performing a secure request.");
   const REQUESTS_URL = "https://example.com" + CORS_SJS_PATH;
   let wait = waitForNetworkEvents(monitor, 1);
   yield ContentTask.spawn(tab.linkedBrowser, REQUESTS_URL, function* (url) {
     content.wrappedJSObject.performRequests(1, url);
   });
   yield wait;
 
-  wait = waitForDOM(document, "#security-panel");
   EventUtils.sendMouseEvent({ type: "click" },
     document.querySelector(".network-details-panel-toggle"));
   EventUtils.sendMouseEvent({ type: "click" },
     document.querySelector("#security-tab"));
-  yield wait;
+  yield waitUntil(() => document.querySelector(
+    "#security-panel .security-info-value"));
 
   let tabpanel = document.querySelector("#security-panel");
   let textboxes = tabpanel.querySelectorAll(".textbox-input");
 
   // Connection
   // The protocol will be TLS but the exact version depends on which protocol
   // the test server example.com supports.
   let protocol = textboxes[0].value;
--- a/devtools/client/netmonitor/test/browser_net_security-error.js
+++ b/devtools/client/netmonitor/test/browser_net_security-error.js
@@ -44,18 +44,16 @@ add_task(function* () {
   function waitForSecurityBrokenNetworkEvent() {
     let awaitedEvents = [
       "UPDATING_REQUEST_HEADERS",
       "RECEIVED_REQUEST_HEADERS",
       "UPDATING_REQUEST_COOKIES",
       "RECEIVED_REQUEST_COOKIES",
       "UPDATING_EVENT_TIMINGS",
       "RECEIVED_EVENT_TIMINGS",
-      "UPDATING_SECURITY_INFO",
-      "RECEIVED_SECURITY_INFO",
     ];
 
     let promises = awaitedEvents.map((event) => {
       return monitor.panelWin.once(EVENTS[event]);
     });
 
     return Promise.all(promises);
   }
--- a/devtools/client/netmonitor/test/browser_net_security-icon-click.js
+++ b/devtools/client/netmonitor/test/browser_net_security-icon-click.js
@@ -41,16 +41,16 @@ add_task(function* () {
     yield ContentTask.spawn(tab.linkedBrowser, { url }, function* (args) {
       content.wrappedJSObject.performRequests(1, args.url);
     });
     return wait;
   }
 
   function* clickAndTestSecurityIcon() {
     let icon = document.querySelector(".requests-security-state-icon");
-
     info("Clicking security icon of the first request and waiting for panel update.");
     EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
+    yield waitUntil(() => document.querySelector("#security-panel .security-info-value"));
 
     ok(document.querySelector("#security-tab[aria-selected=true]"),
        "Security tab is selected.");
   }
 });
--- a/devtools/client/netmonitor/test/browser_net_security-tab-visibility.js
+++ b/devtools/client/netmonitor/test/browser_net_security-tab-visibility.js
@@ -37,17 +37,16 @@ add_task(function* () {
   let { getSelectedRequest } =
     windowRequire("devtools/client/netmonitor/src/selectors/index");
 
   store.dispatch(Actions.batchEnable(false));
 
   for (let testcase of TEST_DATA) {
     info("Testing Security tab visibility for " + testcase.desc);
     let onNewItem = monitor.panelWin.once(EVENTS.NETWORK_EVENT);
-    let onSecurityInfo = monitor.panelWin.once(EVENTS.RECEIVED_SECURITY_INFO);
     let onComplete = testcase.isBroken ?
                        waitForSecurityBrokenNetworkEvent() :
                        waitForNetworkEvents(monitor, 1);
 
     info("Performing a request to " + testcase.uri);
     yield ContentTask.spawn(tab.linkedBrowser, testcase.uri, function* (url) {
       content.wrappedJSObject.performRequests(1, url);
     });
@@ -60,22 +59,30 @@ add_task(function* () {
       document.querySelectorAll(".request-list-item")[0]);
 
     is(getSelectedRequest(store.getState()).securityState, undefined,
        "Security state has not yet arrived.");
     is(!!document.querySelector("#security-tab"), testcase.visibleOnNewEvent,
       "Security tab is " + (testcase.visibleOnNewEvent ? "visible" : "hidden") +
       " after new request was added to the menu.");
 
-    info("Waiting for security information to arrive.");
-    yield onSecurityInfo;
+    if (testcase.visibleOnSecurityInfo) {
+      // click security panel to lazy load the securityState
+      yield waitUntil(() => document.querySelector("#security-tab"));
+      EventUtils.sendMouseEvent({ type: "click" },
+        document.querySelector("#security-tab"));
+      yield waitUntil(() => document.querySelector(
+        "#security-panel .security-info-value"));
+      info("Waiting for security information to arrive.");
 
-    yield waitUntil(() => !!getSelectedRequest(store.getState()).securityState);
-    ok(getSelectedRequest(store.getState()).securityState,
-       "Security state arrived.");
+      yield waitUntil(() => !!getSelectedRequest(store.getState()).securityState);
+      ok(getSelectedRequest(store.getState()).securityState,
+         "Security state arrived.");
+    }
+
     is(!!document.querySelector("#security-tab"), testcase.visibleOnSecurityInfo,
        "Security tab is " + (testcase.visibleOnSecurityInfo ? "visible" : "hidden") +
        " after security information arrived.");
 
     info("Waiting for request to complete.");
     yield onComplete;
 
     is(!!document.querySelector("#security-tab"), testcase.visibleOnceComplete,
--- a/devtools/client/netmonitor/test/shared-head.js
+++ b/devtools/client/netmonitor/test/shared-head.js
@@ -19,17 +19,16 @@ async function waitForExistingRequests(m
       // and have arbitrary number of field set.
       if (request.id.includes("-clone")) {
         continue;
       }
       // Do same check than FirefoxDataProvider.isRequestPayloadReady,
       // in order to ensure there is no more pending payload requests to be done.
       if (!request.requestHeaders || !request.requestCookies ||
           !request.eventTimings ||
-          (!request.securityInfo && !request.fromServiceWorker) ||
           ((!request.responseHeaders || !request.responseCookies) &&
             request.securityState != "broken" &&
             (!request.responseContentAvailable || request.status))) {
         return false;
       }
     }
     return true;
   }