Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level. r?mixedpuppy draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 10 Nov 2016 23:48:05 -0800
changeset 437853 7aa2141477b67f9de5582d7bb535a33d4c784da8
parent 437852 40cc05cd27cfba787f80f1fb53391e07c7ea8897
child 536738 d9974e9beef61401cd83162bab964b16b5efb045
push id35525
push usermaglione.k@gmail.com
push dateFri, 11 Nov 2016 18:31:02 +0000
reviewersmixedpuppy
bugs1316914
milestone52.0a1
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level. r?mixedpuppy MozReview-Commit-ID: JdaxEH22Sub
toolkit/modules/addons/WebRequest.jsm
--- a/toolkit/modules/addons/WebRequest.jsm
+++ b/toolkit/modules/addons/WebRequest.jsm
@@ -217,23 +217,27 @@ class ResponseHeaderChanger extends Head
       }
     } catch (e) {
       Cu.reportError(new Error(`Error setting response header ${name}: ${e}`));
     }
   }
 
   visitHeaders(visitor) {
     if (this.channel instanceof Ci.nsIHttpChannel) {
-      this.channel.visitResponseHeaders((name, value) => {
-        if (name.toLowerCase() === "content-type") {
-          value = getData(this.channel).contentType || value;
-        }
+      try {
+        this.channel.visitResponseHeaders((name, value) => {
+          if (name.toLowerCase() === "content-type") {
+            value = getData(this.channel).contentType || value;
+          }
 
-        visitor(name, value);
-      });
+          visitor(name, value);
+        });
+      } catch (e) {
+        // Throws if response headers aren't available yet.
+      }
     }
   }
 }
 
 var HttpObserverManager;
 
 var ContentPolicyManager = {
   policyData: new Map(),
@@ -595,16 +599,66 @@ HttpObserverManager = {
     let data = getData(channel);
     if (!data.suspended) {
       channel.suspend();
       data.suspended = true;
       return true;
     }
   },
 
+  getRequestData(channel, loadContext, policyType, extraData) {
+    let {loadInfo} = channel;
+
+    let data = {
+      requestId: RequestId.get(channel),
+      url: channel.URI.spec.spec,
+      method: channel.requestMethod,
+      browser: loadContext && loadContext.topFrameElement,
+      type: WebRequestCommon.typeForPolicyType(policyType),
+      fromCache: getData(channel).fromCache,
+      windowId: 0,
+      parentWindowId: 0,
+    };
+
+    if (loadInfo) {
+      let originPrincipal = loadInfo.triggeringPrincipal;
+      if (originPrincipal.URI) {
+        data.originUrl = originPrincipal.URI.spec;
+      }
+
+      let {isSystemPrincipal} = Services.scriptSecurityManager;
+
+      data.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
+                                isSystemPrincipal(loadInfo.loadingPrincipal));
+
+      if (loadInfo.frameOuterWindowID) {
+        Object.assign(data, {
+          windowId: loadInfo.frameOuterWindowID,
+          parentWindowId: loadInfo.outerWindowID,
+        });
+      } else {
+        Object.assign(data, {
+          windowId: loadInfo.outerWindowID,
+          parentWindowId: loadInfo.parentOuterWindowID,
+        });
+      }
+    }
+
+    if (channel instanceof Ci.nsIHttpChannelInternal) {
+      try {
+        data.ip = channel.remoteAddress;
+      } catch (e) {
+        // The remoteAddress getter throws if the address is unavailable,
+        // but ip is an optional property so just ignore the exception.
+      }
+    }
+
+    return Object.assign(data, extraData);
+  },
+
   runChannelListener(channel, loadContext = null, kind, extraData = null) {
     let handlerResults = [];
     let requestHeaders;
     let responseHeaders;
 
     try {
       if (this.activityInitialized) {
         let channelData = getData(channel);
@@ -612,127 +666,72 @@ HttpObserverManager = {
           if (channelData.errorNotified) {
             return;
           }
           channelData.errorNotified = true;
         } else if (this.errorCheck(channel, loadContext, channelData)) {
           return;
         }
       }
-      let listeners = this.listeners[kind];
-      let browser = loadContext && loadContext.topFrameElement;
-      let loadInfo = channel.loadInfo;
+
+      let {loadInfo} = channel;
       let policyType = (loadInfo ? loadInfo.externalContentPolicyType
                                  : Ci.nsIContentPolicy.TYPE_OTHER);
 
       let includeStatus = (["headersReceived", "onRedirect", "onStart", "onStop"].includes(kind) &&
                            channel instanceof Ci.nsIHttpChannel);
 
-      let blockable = ["headersReceived", "opening", "modify"].includes(kind);
-
-      if (channel instanceof Ci.nsIHttpChannel) {
-        requestHeaders = new RequestHeaderChanger(channel);
-        try {
-          responseHeaders = new ResponseHeaderChanger(channel);
-        } catch (e) {
-          // Just ignore this for the request phases where response headers
-          // aren't yet available.
-        }
-      }
-
       let commonData = null;
       let uri = channel.URI;
       let requestBody;
-      for (let [callback, opts] of listeners.entries()) {
+      for (let [callback, opts] of this.listeners[kind].entries()) {
         if (!this.shouldRunListener(policyType, uri, opts.filter)) {
           continue;
         }
 
         if (!commonData) {
-          commonData = {
-            requestId: RequestId.get(channel),
-            url: uri.spec,
-            method: channel.requestMethod,
-            browser: browser,
-            type: WebRequestCommon.typeForPolicyType(policyType),
-            fromCache: getData(channel).fromCache,
-            windowId: 0,
-            parentWindowId: 0,
-          };
-
-          if (loadInfo) {
-            let originPrincipal = loadInfo.triggeringPrincipal;
-            if (originPrincipal.URI) {
-              commonData.originUrl = originPrincipal.URI.spec;
-            }
-
-            let {isSystemPrincipal} = Services.scriptSecurityManager;
-
-            commonData.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
-                                            isSystemPrincipal(loadInfo.loadingPrincipal));
-
-            if (loadInfo.frameOuterWindowID) {
-              Object.assign(commonData, {
-                windowId: loadInfo.frameOuterWindowID,
-                parentWindowId: loadInfo.outerWindowID,
-              });
-            } else {
-              Object.assign(commonData, {
-                windowId: loadInfo.outerWindowID,
-                parentWindowId: loadInfo.parentOuterWindowID,
-              });
-            }
-          }
-
-          if (channel instanceof Ci.nsIHttpChannelInternal) {
-            try {
-              commonData.ip = channel.remoteAddress;
-            } catch (e) {
-              // The remoteAddress getter throws if the address is unavailable,
-              // but ip is an optional property so just ignore the exception.
-            }
-          }
-
-          Object.assign(commonData, extraData);
+          commonData = this.getRequestData(channel, loadContext, policyType, extraData);
         }
-
         let data = Object.assign({}, commonData);
 
         if (opts.requestHeaders) {
+          requestHeaders = requestHeaders || new RequestHeaderChanger(channel);
           data.requestHeaders = requestHeaders.toArray();
         }
 
         if (opts.responseHeaders && responseHeaders) {
+          responseHeaders = responseHeaders || new ResponseHeaderChanger(channel);
           data.responseHeaders = responseHeaders.toArray();
         }
 
         if (opts.requestBody) {
           requestBody = requestBody || WebRequestUpload.createRequestBody(channel);
           data.requestBody = requestBody;
         }
 
         if (includeStatus) {
           mergeStatus(data, channel, kind);
         }
 
         try {
           let result = callback(data);
 
-          if (result && typeof result === "object" && blockable && opts.blocking) {
+          if (result && typeof result === "object" && opts.blocking) {
             handlerResults.push({opts, result});
           }
         } catch (e) {
           Cu.reportError(e);
         }
       }
     } catch (e) {
       Cu.reportError(e);
     }
 
-    return this.applyChanges(kind, channel, loadContext, handlerResults, requestHeaders, responseHeaders);
+    return this.applyChanges(kind, channel, loadContext, handlerResults,
+                             requestHeaders, responseHeaders);
   },
 
   applyChanges: Task.async(function* (kind, channel, loadContext, handlerResults, requestHeaders, responseHeaders) {
     let asyncHandlers = handlerResults.filter(({result}) => isThenable(result));
     let isAsync = asyncHandlers.length > 0;
     let shouldResume = false;
 
     try {