1409878 - add async proxy.onRequest API

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

The proxy api currently has a sort-of PAC script, which is fine for those who really need to stick to PAC scripts.  However, it is not ideal.  A background script should be able to register a proxy resolve listener.  The blocker on doing that is that the proxy service calls the filters synchronously.

Comment 1

[Eric Jung [:ericjung]]

https://docs.google.com/document/d/17DqDXWc3RMWH_LPUkjZcSmtqXHi1mpz6T-ocSey41Pg/edit

Comment 2

[Shane Caraveo (:mixedpuppy)]

The new api will use the channel filter and have more access to information from the channel.  Just how much I'm not certain, but we'll replicate as much as we can from webrequest channel handling.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1409878 implement async proxy api

Review commit: https://reviewboard.mozilla.org/r/222374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222374/

Comment 6

[Shane Caraveo (:mixedpuppy)]

Primary remaining issues are:

- limit proxy.onAuthRequired to proxy auth requests
  - will also deprecate the ability to handle auth for system requests in the regular webRequest.onAuthRequired
- figure out why proxyInfo is unavailable in webRequest when using the new api
  - The only difference [I can think would cause an issue] is using ProxyService.registerChannelFilter rather than ProxyService.registerFilter

Comment 7

[Shane Caraveo (:mixedpuppy)]

Honza, I'm wondering if you've any thoughts on the proxyInfo issue I mentioned in comment 6.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Honza, I'm wondering if you've any thoughts on the proxyInfo issue I
> mentioned in comment 6.

BTW, this only happens when the proxy server requests authentication, and only with the new api.  I don't believe it is due to the async work, since the the old api still works fine in that situation.  The only real difference in handling as I mentioned is that I am using nsIProtocolProxyChannelFilter for the new API.  Authentication works, but the channel is missing proxyInfo.

You can see the new tests in the patch at:

toolkit/components/extensions/test/xpcshell/test_proxy_listener.js (proxyInfo available)
toolkit/components/extensions/test/xpcshell/test_ext_proxy_onauthrequired.js (proxyInfo unavailable)

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

https://reviewboard.mozilla.org/r/222374/#review228296

::: toolkit/components/extensions/ProxyScriptContext.jsm:255
(Diff revision 1)
> +      this /* nsIProtocolProxyChannelFilter aFilter */,
> +      0 /* unsigned long aPosition */
> +    );
> +  }
> +
> +  // Copy from WebRequest.jsm with small

"Copy from WebRequest.jsm with small changes."

::: toolkit/components/extensions/WebRequestEventManager.jsm:1
(Diff revision 1)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

I should hg cp ext-WebRequest to this file for the final patch.  This code has a couple small changes that are not noticable otherwise.  Primary item is the addtion of the options argument, which I intend to use as a way to pass options for proxy authentication handling.

::: toolkit/components/extensions/ext-proxy.js:29
(Diff revision 1)
> -let proxyScriptContextMap = new WeakMap();
> +const proxyScriptContextMap = new WeakMap();
> +
> +// EventManager-like class specifically for Proxy filters. Inherits from
> +// EventManager. Takes care of converting |details| parameter
> +// when invoking listeners.
> +function ProxyFilterEventManager(context, eventName) {

Very similar to the webRequestEventManager except the use of ProxyChannelFilter.

::: toolkit/components/extensions/ext-proxy.js:106
(Diff revision 1)
>            this.register(url);
>          },
>  
> +        onRequest: new ProxyFilterEventManager(context, "onRequest").api(),
> +
> +        onAuthRequired: new WebRequestEventManager(context, "onAuthRequired", {apiName: "proxy", proxyAuthOnly: true}).api(),

Rather than copy or modularize large parts of WebRequest.jsm, I'm sharing the event manager from ext-webRequest here.

::: toolkit/components/extensions/ext-proxy.js:119
(Diff revision 1)
> +            extension.off("proxy-error", listener);
> +          };
> +        }).api(),
> +
> +        // TODO deprecate
>          onProxyError: new EventManager(context, "proxy.onProxyError", fire => {

not sure if there is a way to make onProxyError = this.onError when it comes to the events.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222374/diff/1-2/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

https://reviewboard.mozilla.org/r/222372/#review228346

Hi Shane,
I want to take a more deeper look to the test case, but in the meantime here an initial round of feedback comments
(and some questions).

::: toolkit/components/extensions/ProxyScriptContext.jsm:19
(Diff revision 2)
> +const {
> +  apiManager,
> +} = ExtensionParent;
> +
> +// global is not avialable in some xpcshell tests (without browser window)
> +const tabTracker = apiManager.global && apiManager.global.tabTracker;

How about passing the `tabTracker` instance as an additional parameter of the `ProxyChannelFilter` constructor instead on retrieving it from `ExtensionParent.apiManager.global`?

So that we pass it from the "ext-proxy.js" and "ext-webRequest.js" API modules where tabTracker is already expected to be available as a global.

::: toolkit/components/extensions/ProxyScriptContext.jsm:23
(Diff revision 2)
> +
> +const {
> +  apiManager,
> +} = ExtensionParent;
> +
> +// global is not avialable in some xpcshell tests (without browser window)

s/avialable/available/

::: toolkit/components/extensions/ProxyScriptContext.jsm:236
(Diff revision 2)
> +        throw new ExtensionError("ProxyInfoData: proxyData must be a string or array of objects");
> +    }
> +  },
>  };
>  
> +function parseFilter(filter) {

Nit, this function seems to "normalize" filter more than "parsing" it, maybe it could be renamed as `normalizeFilter`.

::: toolkit/components/extensions/ProxyScriptContext.jsm:258
(Diff revision 2)
> +    let data = {
> +      requestId: String(channel.id),
> +      url: channel.finalURL,
> +      method: channel.method,
> +      type: channel.type,
> +      fromCache: !!channel.fromCache,
> +
> +      originUrl: channel.originURL || undefined,
> +      documentUrl: channel.documentURL || undefined,
> +
> +      frameId: channel.windowId,
> +      parentFrameId: channel.parentWindowId,
> +
> +      frameAncestors: channel.frameAncestors || undefined,
> +
> +      ip: channel.remoteAddress,
> +
> +      proxyInfo: channel.proxyInfo,
> +
> +      timeStamp: Date.now(),
> +    };
> +
> +    return Object.assign(data, extraData);

Nit, this could be probably be also rewritten as

```
return {
  requestId: String(channel.id),
  // and the other properties defined on data
  // and then:
  
  ...extraData,
};
```

::: toolkit/components/extensions/ProxyScriptContext.jsm:289
(Diff revision 2)
> +   * This method (which is required by the nsIProtocolProxyService interface)
> +   * is called to apply proxy filter rules for the given URI and proxy object
> +   * (or list of proxy objects).
> +   *
> +   * @param {Object} service A reference to the Protocol Proxy Service.
> +   * @param {Object} channel The channel for which these proxy settings apply.

Nit, isn't this parameter of type `{nsIChannel|nsIHttpChannel}`?

::: toolkit/components/extensions/ProxyScriptContext.jsm:319
(Diff revision 2)
> +        // We only accept proxyInfo objects
> +        if (typeof ret == "object") {
> +          if (!Array.isArray(ret)) {
> +            ret = [ret];
> +          }
> +          proxyInfo = ProxyInfoData.createProxyInfoFromData(ret, defaultProxyInfo);

Nit, it is not immediately visible that besides the typeof and Array.isArray checks that we do here, `ProxyInfoData.createProxyInfoFromData` is also validate the returned proxyInfo object properties and raised additional ExtensionError is they are not valid, it would be nice to add a small inline comment here to mention that.

::: toolkit/components/extensions/ProxyScriptContext.jsm:333
(Diff revision 2)
> +        fileName: error.fileName,
> +        lineNumber: error.lineNumber,
> +        stack: error.stack,
> +      });
> +    }
> +    callback.onProxyFilterResult(proxyInfo || defaultProxyInfo);

Nit, it is a bit confusing that the `nsIProtocolProxyChannelFilter` parameter is actually called `callback`, I would prefer it to be named in a way that makes it clear that it is not a function, maybe `proxyChannelFilter` or `proxyFilter`.

::: toolkit/components/extensions/ext-proxy.js:29
(Diff revision 2)
> -let proxyScriptContextMap = new WeakMap();
> +const proxyScriptContextMap = new WeakMap();
> +
> +// EventManager-like class specifically for Proxy filters. Inherits from
> +// EventManager. Takes care of converting |details| parameter
> +// when invoking listeners.
> +function ProxyFilterEventManager(context, eventName) {

Nit, can't this be written as a class ProxyFilterEventManager that extends EventManager?

::: toolkit/components/extensions/ext-proxy.js:31
(Diff revision 2)
> +// EventManager-like class specifically for Proxy filters. Inherits from
> +// EventManager. Takes care of converting |details| parameter
> +// when invoking listeners.
> +function ProxyFilterEventManager(context, eventName) {
> +  let name = `proxy.${eventName}`;
> +  let register = (fire, filter, info) => {

`info` seems to be unused.

::: toolkit/components/extensions/ext-proxy.js:36
(Diff revision 2)
> +  let register = (fire, filter, info) => {
> +    let listener = (data) => {
> +      return fire.sync(data);
> +    };
> +
> +    let filter2 = {};

it is not a big deal, but it would be nice to find a name that is not `filter2` :-)

(maybe by renaming `filter` into `filterProps` or something similar and `filter2` into `filter`?)

::: toolkit/components/extensions/ext-proxy.js:108
(Diff revision 2)
>  
> +        onRequest: new ProxyFilterEventManager(context, "onRequest").api(),
> +
> +        onAuthRequired: new WebRequestEventManager(context, "onAuthRequired", {apiName: "proxy", proxyAuthOnly: true}).api(),
> +
> +        onError: new EventManager(context, "proxy.onError", fire => {

Instead of having two different event manager (one for the new onError and one for the "to be deprecated" onProxyError) could we create the event manager once (e.g. near line 80-81 of this file) and then assign it to both onError and onProxyError?

both these events are going to be fired when the "proxy-error" event is emitted on the extension and so they looks like two alias of the same events.

::: toolkit/components/extensions/WebRequestEventManager.jsm:18
(Diff revision 2)
> +const {
> +  EventManager,
> +} = ExtensionCommon;
> +
> +const {
> +  apiManager,
> +} = ExtensionParent;
> +
> +// global is not avialable in some xpcshell tests (without browser window)
> +const tabTracker = apiManager.global && apiManager.global.tabTracker;

As for "ProxyScriptContext.jsm":

How about passing the `tabTracker` instance as an additional parameter of the `WebRequestEventManager` constructor instead on retrieving it from `ExtensionParent.apiManager.global`?

::: toolkit/components/extensions/WebRequestEventManager.jsm:26
(Diff revision 2)
> +
> +const {
> +  apiManager,
> +} = ExtensionParent;
> +
> +// global is not avialable in some xpcshell tests (without browser window)

s/avialable/available/

::: toolkit/components/extensions/WebRequestEventManager.jsm:75
(Diff revision 2)
>      let blockingAllowed = context.extension.hasPermission("webRequestBlocking");
>  
>      let info2 = [];
>      if (info) {
>        for (let desc of info) {
>          if (desc == "blocking" && !blockingAllowed) {
>            Cu.reportError("Using webRequest.addListener with the blocking option " +
>                           "requires the 'webRequestBlocking' permission.");
>          } else {
>            info2.push(desc);
>          }
>        }
>      }

Looking at the test it seems that is expected that an addon that uses this API could use the additional "blocking" option on the proxy.onAuthRequired request as supported by the webRequest API events, but I'm wondering: does a non-blocking proxy.onAuthRequired listener have any use for the proxy API? 

if not, we could probably make "blocking" the default for a proxy.onAuthRequired event listener and remove the addition webRequest options parameter (which may also support other webRequest options that are not needed or not supposed to be supported by the proxy API).

::: toolkit/components/extensions/WebRequestEventManager.jsm:89
(Diff revision 2)
> -    let listenerDetails = {
> +    let listenerDetails = Object.assign({
>        addonId: context.extension.id,
>        extension: context.extension.policy,
>        blockingAllowed,
>        tabParent: context.xulBrowser.frameLoader.tabParent,
> -    };
> +    }, options);

Nit, This could also be written as:

```
let listenerDetails = {
  addonId:  context.extension.id,
  // and all the other properties and then:
  ...options
}
```

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_onauthrequired.js:41
(Diff revision 2)
> -      browser.test.assertEq("localhost", details.proxyInfo.host, "proxy host");
> -      browser.test.assertEq(port, details.proxyInfo.port, "proxy port");
> -      browser.test.assertEq("http", details.proxyInfo.type, "proxy type");
> -      browser.test.assertEq("", details.proxyInfo.username, "proxy username not set");
> +      // browser.test.assertEq("localhost", details.proxyInfo.host, "proxy host");
> +      // browser.test.assertEq(port, details.proxyInfo.port, "proxy port");
> +      // browser.test.assertEq("http", details.proxyInfo.type, "proxy type");
> +      // browser.test.assertEq("", details.proxyInfo.username, "proxy username not set");

I guess that these lines are commented temporarily because you are still working on this patch, am I right? (same for the ones in the other listeners)

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> - figure out why proxyInfo is unavailable in webRequest when using the new
> api
>   - The only difference [I can think would cause an issue] is using
> ProxyService.registerChannelFilter rather than ProxyService.registerFilter

There is no different between the two.  The internal function for calling a filter is only one, but for a channel filter we pass the channel, for a "normal" filter we pass URI of the channel, instead of the channel, so it's definitely not the thing.

if you give me a test case I can take a look, maybe there is a bug in my patch.

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #12)

> if you give me a test case I can take a look, maybe there is a bug in my
> patch.

If you apply the patch and use 

toolkit/components/extensions/test/xpcshell/test_ext_proxy_onauthrequired.js

uncomment all the assertEq lines that test proxyInfo.

The other test (using the webextension proxy/pac style api) will pass, it is here:

toolkit/components/extensions/test/xpcshell/test_ext_proxy_auth.js

The two tests are essentially the same.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

https://reviewboard.mozilla.org/r/222374/#review228710

::: toolkit/components/extensions/ProxyScriptContext.jsm:305
(Diff revision 2)
> +      if (wrapper.browserElement && tabTracker) {
> +        browserData = tabTracker.getBrowserData(wrapper.browserElement);
> +      }
> +      let filter = Object.assign({}, this.filter);
> +      if (filter.tabId != null && browserData.tabId != filter.tabId) {
> +        return;

you must throw here.  the contract is you do either of:

- return OK and call the callback (before you leave the method, or asynchronously)
- throw and don't call the callback

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

https://reviewboard.mozilla.org/r/222374/#review228712

::: toolkit/components/extensions/ProxyScriptContext.jsm:321
(Diff revision 2)
> +          if (!Array.isArray(ret)) {
> +            ret = [ret];
> +          }
> +          proxyInfo = ProxyInfoData.createProxyInfoFromData(ret, defaultProxyInfo);
> +        } else {
> +          throw new ExtensionError("ProxyInfoData: proxyData must be an array of objects");

and this is actually wrong as well.  as this is async and past the |await| you must call the callback with the default info, this exception will go just nowhere...

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

https://reviewboard.mozilla.org/r/222374/#review228718

::: toolkit/components/extensions/ProxyScriptContext.jsm:305
(Diff revision 2)
> +      if (wrapper.browserElement && tabTracker) {
> +        browserData = tabTracker.getBrowserData(wrapper.browserElement);
> +      }
> +      let filter = Object.assign({}, this.filter);
> +      if (filter.tabId != null && browserData.tabId != filter.tabId) {
> +        return;

Yeah, I'll fix those.

::: toolkit/components/extensions/ProxyScriptContext.jsm:321
(Diff revision 2)
> +          if (!Array.isArray(ret)) {
> +            ret = [ret];
> +          }
> +          proxyInfo = ProxyInfoData.createProxyInfoFromData(ret, defaultProxyInfo);
> +        } else {
> +          throw new ExtensionError("ProxyInfoData: proxyData must be an array of objects");

This is being caught just a few lines below and we send the error to the extension, which in turn calls the browser.proxy.onError listener.  After that, onProxyFilterResult is called.

Comment 17

[Honza Bambas (:mayhemer)]

I am not sure where the problem is.  I can clearly see that the nsHttpChannel on the parent process is assigned a proxy info object, all the time, before we call on-modify-request.  Hence, if it's missing in the object the browser.webRequest events are getting, then the problem is likely somewhere inside the web ext code.  I don't know that code, and it would be good to first look there (add some dumps, logs, whatever)

Not sure how regular this is, but 

./mach test toolkit/components/extensions/test/xpcshell/test_ext_proxy_onauthrequired.js --verbose --sequential

passes the checks in browser.webRequest.onBeforeRequest, but later fails in browser.proxy.onAuthRequired - pi is missing there (tho, we never remove it from the channel!)  - this is the remote version of the test.  I honestly don't know how to run just the non-remote test itself...

Comment 18

[Honza Bambas (:mayhemer)]

Putting a breakpoint to ChannelWrapper::GetProxyInfo shows it's called only once:

0 getRequestData(channel = [object ChannelWrapper]) ["resource://gre/modules/ProxyScriptContext.jsm":275]
    this = [object Object]
1 applyFilter() ["resource://gre/modules/ProxyScriptContext.jsm":312]
2 InterpretGeneratorResume(gen = [object Generator], val = undefined, kind = "next") ["self-hosted":1256]
3 next(val = undefined) ["self-hosted":1211]
    this = [object Generator]
4 _do_main() ["c:\Mozilla\src\mozilla-central1\testing\xpcshell\head.js":221]
    this = [object BackstagePass @ 0x2ebcdb27fa0 (native @ 0x2ebcdb33ab0)]
5 _execute_test() ["c:\Mozilla\src\mozilla-central1\testing\xpcshell\head.js":533]
    this = [object BackstagePass @ 0x2ebcdb27fa0 (native @ 0x2ebcdb33ab0)]
6 <TOP LEVEL> ["-e":1]


and then never again...

Comment 19

[Shane Caraveo (:mixedpuppy)]

I figured out what made it happen, not certain why though.  Removing access to channel.proxyInfo during the applyfilter call fixed the problem.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222374/diff/2-3/

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222374/diff/3-4/

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Luca Greco [:rpl] from comment #11)

> ::: toolkit/components/extensions/WebRequestEventManager.jsm:18
> (Diff revision 2)
> > +const {
> > +  EventManager,
> > +} = ExtensionCommon;
> > +
> > +const {
> > +  apiManager,
> > +} = ExtensionParent;
> > +
> > +// global is not avialable in some xpcshell tests (without browser window)
> > +const tabTracker = apiManager.global && apiManager.global.tabTracker;
> 
> As for "ProxyScriptContext.jsm":
> 
> How about passing the `tabTracker` instance as an additional parameter of
> the `WebRequestEventManager` constructor instead on retrieving it from
> `ExtensionParent.apiManager.global`?

I think it's fine to get it from apiManager.

I addressed the other feedback issues.

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> I figured out what made it happen, not certain why though.  Removing access
> to channel.proxyInfo during the applyfilter call fixed the problem.

The ChannelWrapper binding caches items, so that was the cause.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

Adding Kris to review as well, especially my use of ChannelWrapper.

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> I figured out what made it happen, not certain why though.  Removing access
> to channel.proxyInfo during the applyfilter call fixed the problem.

Shane, I believe you are caching the info somewhere (in the wrapper, presumably) and provide that cache even though it has actually changed on the channel.

Comment 25

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #24)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> > I figured out what made it happen, not certain why though.  Removing access
> > to channel.proxyInfo during the applyfilter call fixed the problem.
> 
> Shane, I believe you are caching the info somewhere (in the wrapper,
> presumably) and provide that cache even though it has actually changed on
> the channel.

Yeah, I commented on that in comment 22

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8953093 [details]
Bug 1409878 implement async proxy api

https://reviewboard.mozilla.org/r/222374/#review228312

::: toolkit/components/extensions/ProxyScriptContext.jsm:17
(Diff revisions 2 - 4)
>  ChromeUtils.import("resource://gre/modules/ExtensionParent.jsm");
>  
>  const {
>    apiManager,
>  } = ExtensionParent;
>  
> -// global is not avialable in some xpcshell tests (without browser window)
> +// global is not available in some xpcshell tests (without browser window)
>  const tabTracker = apiManager.global && apiManager.global.tabTracker;

If we are going to get the tabTracker directly from the `ExtensionParent.apiManager.global`, we could at least making it lazy, e.g. something like:

```
XPCOMUtils.defineLazyGetter(this, "tabTracker", () => {
  const {ExtensionParent} = ChromeUtils.import("...");
  
  return ExtensionParent.apiManager.global.tabTracker;
});
```

Beside that, this comment worries me a bit ("// global is not available in some xpcshell tests..."),
I'm wondering if it would still be needed to check if apiManager.global is defined once the tabTracker is lazy retrieved only when needed.

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1409878 implement async proxy api for webextensions,

Review commit: https://reviewboard.mozilla.org/r/224002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224002/

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1409878 implement async proxy api for webextensions,

Review commit: https://reviewboard.mozilla.org/r/224062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224062/

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8954855 [details]
Bug 1409878 implement async proxy api for webextensions,

https://reviewboard.mozilla.org/r/224002/#review230038

Hi Shane,
Follows some additional review comments (mostly nits, some proposed tweak to apply).

The changes on the ext-proxy API module and the tests looks good to me, but I don't have enough direct experience related to the proxy API (and the related internals) to sign off all the changes applied to ProxyScriptContext.jsm (nevertheless I looked into them and commented where I felt that it was confusing or missing something), but this patch is also going to be reviewed by Kris and he can definitely cover those parts.

Anyway, none of the nits and proposed tweaks that are part of this round of review comments seems to require any big change (and some of them are definitely optional, or subjective), and so I'm setting r+ for the parts that I can already sign off.

::: toolkit/components/extensions/ProxyScriptContext.jsm:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> -var EXPORTED_SYMBOLS = ["ProxyScriptContext"];
> +var EXPORTED_SYMBOLS = ["ProxyScriptContext", "ProxyChannelFilter"];
>  
>  /* exported ProxyScriptContext */

Nit, it seems that there should also be "ProxyChannelFilter" in this eslint "exported" inline comment (anyway, eslint doesn't seem to complain)

::: toolkit/components/extensions/ProxyScriptContext.jsm:254
(Diff revision 1)
> +  // Copy from WebRequest.jsm with small changes.
> +  getRequestData(channel, extraData) {
> +    let data = {
> +      requestId: String(channel.id),

I briefly looked at the version from WebRequest.jsm, and I was wondering about the reasons behind the following changes (and took a brief look at them):

- the `fromCache` is set to `channel.fromCache` in ProxyScriptContext.jsm and to `!!channel.fromCache` in WebRequest.jsm,  it doesn't seem to be a big difference, but it would be probably reasonable to do the same thing in both

- the properties named `windowId` and `parentWindowId` in WebRequest.jsm, and named `frameId` and `parentFrameId` in ProxyScriptContext.jsm, the reason seems to be that for the proxy API we are sending this data like it it, while for the webRequest API that data is converted to use the property names expected by the extension code (which are actually `frameId` and `parentFrameId`)

It could be helpful to add some details about the reasons of the small changes applied between the two versions of `getRequestData` in the inline comment (so that we don't forget the reasons for the differences between this two similar helpers).

It would be nice to actually share an helper function used in both WebRequest.jsm and ProxyScriptContext.jsm (e.g. we could move the shared part into a shared helper defined in ExtensionUtils.jsm, which is already imported in both the jsm), but if we are going to duplicate it for now (and extract an helper to use in both in a follow up), it could be helpful if we could also have a similar inline comment in WebRequest.jsm (which mention that we should keep an eye to changes that could be needed/useful to make on both the implementations to keep them in sync, especially when we are applying a fix).

::: toolkit/components/extensions/ProxyScriptContext.jsm:315
(Diff revision 1)
> +
> +      if (wrapper.matches(filter, this.context.extension.policy, {isProxy: true})) {
> +        let data = this.getRequestData(wrapper, {tabId: browserData.tabId});
> +
> +        let ret = await this.listener(data);
> +        if (ret === null) {

should we also do the same if ret is `undefined`?

::: toolkit/components/extensions/ProxyScriptContext.jsm:316
(Diff revision 1)
> +      if (wrapper.matches(filter, this.context.extension.policy, {isProxy: true})) {
> +        let data = this.getRequestData(wrapper, {tabId: browserData.tabId});
> +
> +        let ret = await this.listener(data);
> +        if (ret === null) {
> +          // Fall through to finally block.

Nit, we could tweak this inline comment a bit, e.g.
something like

"Fall through to the `finally` block to apply the proxy result"

(I didn't got initially that "finally" was actually related to the try...catch...finally from this function, and I was: "finally blocking what?" :-))

::: toolkit/components/extensions/ext-proxy.js:17
(Diff revision 1)
> +XPCOMUtils.defineLazyServiceGetter(this, "ProxyService",
> +                                   "@mozilla.org/network/protocol-proxy-service;1",
> +                                   "nsIProtocolProxyService");

ProxyService doesn't seem to be used in this file.

::: toolkit/components/extensions/ext-proxy.js:27
(Diff revision 1)
> -let proxyScriptContextMap = new WeakMap();
> +const proxyScriptContextMap = new WeakMap();
> +
> +// EventManager-like class specifically for Proxy filters. Inherits from
> +// EventManager. Takes care of converting |details| parameter
> +// when invoking listeners.
> +function ProxyFilterEventManager(context, eventName) {

Let's rewrite this as a class, it would make it a bit more readable and it doesn't need a lot of changes:

```
class ProxyFilterEventManager extends EventManager {
  constructor(context, eventName) {
    let name = `proxy.${eventName}`;
    let register = (fire, filterProps, extraInfoSpec = []) => {
      ...
    };

    super(context, name, register);
  }
}
```

::: toolkit/components/extensions/ext-proxy.js:108
(Diff revision 1)
> -          extension.on("proxy-error", listener);
> -          return () => {
> +        // TODO deprecate
> +        onProxyError: onError,

Nit, we should file a bugzilla issue for the onProxyError deprecation and mention its bugzilla id here.

::: toolkit/components/extensions/test/xpcshell/test_proxy_listener.js:3
(Diff revision 1)
>  "use strict";
>  
>  /* eslint no-unused-vars: ["error", {"args": "none", "varsIgnorePattern": "^(FindProxyForURL)$"}] */

This eslint inline comment is still needed? it seems to be not needed in this test file (I guess it was in the original test_proxy_script.js that it is based on).

::: toolkit/components/extensions/test/xpcshell/test_proxy_listener.js:154
(Diff revision 1)
> -      failoverProxy: null,
> -    },
> -  });
>  });
>  
>  async function getExtension(proxyResult) {

Nit, how about renaming proxyResult into proxyInfo here and at line 166? just to make it immediately clear that they are exactly the same object.

::: toolkit/components/extensions/test/xpcshell/test_proxy_listener.js:156
(Diff revision 1)
> -  });
>  });
>  
>  async function getExtension(proxyResult) {
> +  function background(proxyInfo) {
> +    browser.test.log(`=== using proxyInfo ${JSON.stringify(proxyInfo)}`);

Nit, it would be nice to tweak this logged message to be a bit more descriptive, e.g.:
how about `testing proxy.onRequest with proxyInfo = ${JSON.stringify(proxyInfo)`?

Comment 30

[Shane Caraveo (:mixedpuppy)]

(In reply to Luca Greco [:rpl] from comment #29)
> Comment on attachment 8954855 [details]

> ::: toolkit/components/extensions/ProxyScriptContext.jsm:254
> (Diff revision 1)
> > +  // Copy from WebRequest.jsm with small changes.
> > +  getRequestData(channel, extraData) {
> > +    let data = {
> > +      requestId: String(channel.id),
> 
> I briefly looked at the version from WebRequest.jsm, and I was wondering
> about the reasons behind the following changes (and took a brief look at
> them):

The differences here are due to extra steps WebRequest does that we do not need here.  See serializeRequestData in WebRequest.  We also cannot really share a helper here as I discovered, some things we do not want to retrieve at this stage in the request. There are a couple items I'm thinking might need to be removed even, though they don't seem to present a problem right now.  See the chatter between myself and Honza up in the comment 20s area.

> ::: toolkit/components/extensions/ProxyScriptContext.jsm:315
> (Diff revision 1)
> > +
> > +      if (wrapper.matches(filter, this.context.extension.policy, {isProxy: true})) {
> > +        let data = this.getRequestData(wrapper, {tabId: browserData.tabId});
> > +
> > +        let ret = await this.listener(data);
> > +        if (ret === null) {
> 
> should we also do the same if ret is `undefined`?

Yes.  That also pointed out a bug for me to fix.

> ::: toolkit/components/extensions/test/xpcshell/test_proxy_listener.js:154
> (Diff revision 1)
> > -      failoverProxy: null,
> > -    },
> > -  });
> >  });
> >  
> >  async function getExtension(proxyResult) {
> 
> Nit, how about renaming proxyResult into proxyInfo here and at line 166?
> just to make it immediately clear that they are exactly the same object.

eslint complained about that.

Comment 31

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8954904 [details]
Bug 1409878 implement async proxy api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224062/diff/1-2/

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8954904 [details]
Bug 1409878 implement async proxy api for webextensions,

https://reviewboard.mozilla.org/r/224062/#review230296

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8954904 [details]
Bug 1409878 implement async proxy api for webextensions,

https://reviewboard.mozilla.org/r/224062/#review230404

::: toolkit/components/extensions/ProxyScriptContext.jsm:25
(Diff revision 2)
> +ChromeUtils.defineModuleGetter(this, "ExtensionParent",
> +                               "resource://gre/modules/ExtensionParent.jsm");

Nit: Please keep module getter definitions together and sorted.

::: toolkit/components/extensions/ProxyScriptContext.jsm:305
(Diff revision 2)
> +
> +      let browserData = {tabId: -1, windowId: -1};
> +      if (wrapper.browserElement) {
> +        browserData = tabTracker.getBrowserData(wrapper.browserElement);
> +      }
> +      let filter = {...this.filter};

Why do we need to clone this object?

::: toolkit/components/extensions/ProxyScriptContext.jsm:306
(Diff revision 2)
> +      if (filter.tabId != null && browserData.tabId != filter.tabId) {
> +        // Fall through to finally block.
> +        return;
> +      }
> +      if (filter.windowId != null && browserData.windowId != filter.windowId) {

Nit: `Id !== filter.`

::: toolkit/components/extensions/ProxyScriptContext.jsm:307
(Diff revision 2)
> +      if (wrapper.browserElement) {
> +        browserData = tabTracker.getBrowserData(wrapper.browserElement);
> +      }
> +      let filter = {...this.filter};
> +      if (filter.tabId != null && browserData.tabId != filter.tabId) {
> +        // Fall through to finally block.

This comment seems kind of unnecessary, especially when repeated.

::: toolkit/components/extensions/ProxyScriptContext.jsm:319
(Diff revision 2)
> +
> +      if (wrapper.matches(filter, this.context.extension.policy, {isProxy: true})) {
> +        let data = this.getRequestData(wrapper, {tabId: browserData.tabId});
> +
> +        let ret = await this.listener(data);
> +        if (ret === undefined || ret === null) {

Can just be `ret == null`

::: toolkit/components/extensions/ext-proxy.js:40
(Diff revision 2)
> +                                         ...context.extension.optionalOrigins.patterns]);
> +
> +        filter.urls = new MatchPatternSet(filter.urls);
> +
> +        if (!perms.overlapsAll(filter.urls)) {
> +          Cu.reportError("The proxy.addListener filter doesn't overlap with host permissions.");

We should really do this on the content side, with `context.cloneScope.Error()`... Otherwise there won't be any useful location information, and this error will be pretty inscrutable.

Comment 34

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8954904 [details]
Bug 1409878 implement async proxy api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224062/diff/2-3/

Comment 35

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8954904 [details]
Bug 1409878 implement async proxy api for webextensions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224062/diff/3-4/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f0ee2f582a2
implement async proxy api for webextensions, r=kmag,rpl

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9f0ee2f582a2

Comment 38

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 39

[Will Bamberg [:wbamberg]]

I've written some docs for this:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy/onRequest
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy/ProxyInfo
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy/RequestDetails (this one is a little weird as it doesn't explicitly exist, but it seemed cleaner to document the listener argument as a separate type)

I've also updated the main page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy to talk a little about the 2 approaches (PAC versus onRequest) and have moved most of the PAC stuff into https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy/register.

There's a compat data PR open for the two types: https://github.com/mdn/browser-compat-data/pull/1832

Please let me know if we need anything else here.

Comment 40

[Shane Caraveo (:mixedpuppy)]

(In reply to Will Bamberg [:wbamberg] from comment #39)

> RequestDetails (this one is a little weird as it doesn't explicitly exist,
> but it seemed cleaner to document the listener argument as a separate type)

I think that layout works well.
 
> I've also updated the main page:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy 

You might also mention the availability of the DNS api that can be used with onRequest, and also the proxy settings in browserSettings where one can configure firefox to use a real PAC script.

Comment 41

[Will Bamberg [:wbamberg]]

Thanks Shane. I've mentioned those 2 APIs in the overview page. I'm marking this dev-doc-complete but let me know if we need anything else here.

Comment 42

[Julien Cristau [:jcristau]]

This doesn't seem to belong in the main user-facing release notes.