1322235 - The extension policy service is too slow and used on hot code paths

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[u462496]

In attempting to convert my XUL addons to WebExtensions, I have been working with trying to emulate what I currently do with XUL popup using browserAction popup.  The browserAction popup is decidedly (and visibly) much slower.  I dare say that the delay in displaying the browserAction popup is practically unacceptable taking into account what users are accustomed to when opening popups.

I did some time comparisons between the XUL popup and the browserAction popup.  The menuitems in the XUL popup display similarly to the browserAction popup, except that the XUL items are actually a bit more complex (using XBL extensions which display more objects in the menuitem than the browserAction popup items).  I am not loading any remote resources at all.  Tests were performed on Mac OS X, FF 53.

Here are some timings:

Create popup which displays menuitems representing 301 tabs. Average time from user mousedown on the respective button to popupshown event:

XUL: 293ms
WE browserAction: 1366ms (doing fast click, so the built-in 200ms delay from mouseup event before allowing display should not be a factor here)

I wanted to see how much of this time entailed just accessing and iterating the tabs, only accessing the desired tab properties but not creating any elements or adding anything to the DOM:

XUL - gBrowser.tabs: 1.2ms
WE - browser.tabs.query({currentWindow: true}): 203ms (2/3 of the time it takes for XUL to do the whole thing)

For another perspective on what it takes to create HTML "menuitems", I ran a script in a browser web page to compare timing:

Average time from user mousedown on reload button (holding down shift key to force non-cache reload) to DOMContentLoaded event. (Timings are the difference between loading the host web page with and without running the script generating the "menuitems"):

HTML web page in browser window: 441ms (actually 38% of the time to render and display HTML in browserAction popup)

The HTML "menuitems" - both in the web page test and those generated in the browserAction popup are as follows:

<div class="menuitem">
  <div class="itemicon"><img class="imageicon" src="" width="12" height="12"></div>
  <div class="itemtext">tab title 0</div>
  <div class="itemclose" style="opacity: 0.5;">
    <img class="imageclose" src="file:///Users/allasso/Desktop/icons/button_close_12.png" width="10" height="10">
  </div>
</div>

Admittedly there is some css involved, but very little compared to that applied to XUL menuitem.

Conclusions:

Querying the tab data for WebExtension compared to XUL is much slower:

XUL: 1.2ms
WE: 203ms

Rendering the HTML in browserAction popup compared to XUL popup is much slower:

XUL: 292ms
browserAction: 1163ms

IIUC, a new document loads every time the browserAction popup is opened, so I don't see much of a way to optimize as a workaround since the user must see the contents as soon as they click to open it.  If I am not seeing this correctly, please advise.

Comment 1

[Kris Maglione [:kmag]]

Please attach a testcase extension that shows the problem.

Comment 2

[u462496]

Attachment: WE_popup_performance_1.zip

WebExtension 1

Comment 3

[u462496]

Attachment: WE_popup_performance_2.zip

WebExtension 2

Comment 4

[u462496]

Attachment: [e-mail]

UL extension 1

Comment 5

[u462496]

I have attached 2 WE extensions and 1 XUL extensions.  In essence using the combination of these 3 addons will demonstrate the following:

1) Timing comparison between XUL and WE extension of rendering and displaying static content (hardcoded in respective XUL and HTML) from the time the user mousedowns on respective button to the displaying of the popup and its contents.

2) Timing comparison between XUL and WE extension of iterating over all tabs and grabbing a few properties.

Use as follows:

1) Install all extensions concurrently on FF 53. (manually unzip WE extensions and follow instructions here for temporary installation: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Temporary_Installation_in_Firefox)

2) Install toolbar button for XUL extension 1.

3) Demonstrate timing of popup rendering and display and compare between WE and XUL:

XUL extension 1 -- toolbar button ("XUL1") popup -- Displays 500 iconic menuitems which are hardcoded in XUL document.  Then records et from the time of mousedown on the toolbar button to popupshown event.  The et for the test as well as the average time for the last 5 tests will be displayed in a separate popup.  This data will also be shown in the browser console log.

WE extension 1 -- toolbar button ("WE1") popup -- Displays 500 HTML iconic "menuitems" which are hard coded in HTML document.  Then records et from the time of mousedown on the toolbar button to popupshown event.  The et for the test as well as the average time for the last 5 tests will be displayed in a separate popup.  This data will also be shown in the browser console log.

NOTE 1: XUL extension 1 must be installed in order to demonstrate WE extension 1 results.

4) Demonstrate timing of iterating over tabs:

XUL extension 1 -- Tools > Test XUL Popup Performance > ... click option to set browser state for 100, 300 or 500 tabs.

XUL extension 1 -- Tools > Test XUL Popup Performance > Get Timing - "Iterate over gBrowser.tabs".  Runs code which iterates over `gBrowser.tabs`, and grabs `tab.label` and `tab.getAttribute("image")` for each tab.  The et as well as the average time for the last 5 tests will be displayed in a separate popup.  This data will also be shown in the browser console log. 

WE extension 2 -- Click toolbar button ("WE2").  Runs code which iterates over `browser.tabs.query({currentWindow: true})`, and grabs `tab.title` and `tab.favIconUrl` for each tab.  This data will be shown in the browser console log.  There will NOT be a popup which displays the results for this action.

NOTES:

All timing data for all tests will be displayed in the browser console log; most timing data is displayed in the info popup generated by the XUL addon for convenience.

The HTML "menuitem" displayed in the WE browserAction popup is as follows:

      <div class="menuitem">
        <div class="itemicon"><img class="imageicon" src="../icons/WE_1_16.png" width="12" height="12"></div>
        <div class="itemtext">tab title 0</div>
        <div class="itemacceltext">#C</div>
      </div>

500 items (used in the static popup content in #3) is not an unlikely amount if the extension is displaying items representing tabs.

The effects on timings for the WE extension is noticeably exacerbated by a busy user profile as compared to a clean one.  For example, timings for WE browserAction popup display of 780ms on a fresh profile compared to 1366ms on my working profile, which begins to push the effects to the point of being a bit untolerable.

Comment 6

[Kris Maglione [:kmag]]

OK, there are two issues here:

1) Popup content is still loaded in the main UI process, which means that rendering it blocks the main browser UI.

2) We call extensionURILoadableByAnyone about 8,000 times for the 500 times that the popup loads an image.

Comment 7

[u462496]

(In reply to Kris Maglione [:kmag] from comment #6) 
> 2) We call extensionURILoadableByAnyone about 8,000 times for the 500 times
> that the popup loads an image.

I removed the image loading, WE is still about 6x slower than XUL.

Comment 8

[u462496]

(In reply to Kevin Jones from comment #7)
> (In reply to Kris Maglione [:kmag] from comment #6) 
> > 2) We call extensionURILoadableByAnyone about 8,000 times for the 500 times
> > that the popup loads an image.
> 
> I removed the image loading, WE is still about 6x slower than XUL.

~ 7.4x slower with images
~ 5.9x slower without images

Comment 9

[u462496]

It appears by the summary update that this bug will no longer be addressing the discrepancy of querying the tab data between XUL and WE, which is about 22x slower with WE.  Should I file a separate bug for this?

Comment 10

[Kris Maglione [:kmag]]

Feel free to file another bug for tabs.query.

The actual delay in the popup showing will be fixed by out-of-process extensions, which already has its own bug.

Beyond that, the HTML that you're rendering is just expensive to render. We can probably improve things a bit by minimizing the number of reflows we force in the resizing code, but, again, that's already planned, and is a separate bug.

Comment 11

[Andy McKay]

Naveed, can I query why this is a qf:p1? It only affects add-ons that use the the tabs.query or specific uses cases mentioned in comment 10 and that should be a small part of the target audience compared to quantum flow. I'm not sure if that's required in the quantum flow release criteria.

Comment 12

[Kris Maglione [:kmag]]

We invoke this code every time we attempt to load or get flags for a moz-extension: URL, which means it has huge startup and runtime overhead when WebExtensions are running.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

Bill, can you please review the binding code, and the general sanity of the
platform code. Andrew and zombie, can you please matching algorithms and
tests.

Change summary:

The existing JavaScript matching code works well overall, but it needs to be
called a lot, particularly from hot code paths. In most cases, the overhead of
the matching code on its own adds up enough to cause a problem. When we have
to call out to JavaScript via XPConnect to make a policy decision, it adds up
even more.

These classes solve both of these problems by a) being very fast, and b) being
accessible directly from C++. They are particularly optimized for the common
cases where only literal or prefix matches are required, and they take special
steps to avoid virtual calls wherever possible, and caching computed URL
values so that they can be reused across many match operations without
additional overhead.

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

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 2 - Use MatchGlob for webAccessibleResources.

This is the first step toward migrating the web-accessible URL policy to
purely native code. It should have a noticeable performance improvement on its
own, but the main improvement comes from being able to pass the pattern
objects to the pure C++ policy service.

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

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

This is the second step to migrating the policy service to pure native code,
with similar impacts and reasoning to the previous patch.

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

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings.

Bill, can you please review the binding code and the general sanity of the
platform code? Andrew and zombie, can you please review the policy logic and
tests?

As in part 1, this aims to reduce the overhead of our extension policy logic
by making it directly available to native code with as little JS and XPConnect
overhead as possible.

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

Comment 17

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 5 - Add an ExtensionPolicyService singleton class to track active extension policies.

Bill, can you please review the binding changes? Shane, can you please review
the policy service?

This is the first step to making extension policy data directly available to
C++ code without any COM overhead. It tracks the set of currently active
extensions, and how they map to add-on IDs and URIs.

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

Comment 18

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

This replaces the JS policy service stubs with a pure C++ version which
directly makes policy decisions based on active WebExtensionPolicy objects.

This is the first step in a larger refactoring, which will remove the
ExtensionManagement module entirely, and replace the current add-on policy
service with direct, non-virtual access to native WebExtensionPolicy objects.

It will also be followed by related changes to migrate the content script and
extension page matching to native code, based on the existing MatchPattern and
WebExtensionPolicy bindings.

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

Comment 19

[Kris Maglione [:kmag]]

Attachment: Bug 1322235: Part 7 - DeCOMtaminate moz-extension protocol handler.

This removes unnecessary COM overhead from the extension protocol service,
particularly from the flag lookup code, which is called often, and from hot
paths. The devirtualized lookups should have virtually no overhead for
extensions without web-accessible resources, and very little overhead except
when resources are specified as non-prefix globs.

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

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871115 [details]
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

https://reviewboard.mozilla.org/r/142628/#review146564

::: toolkit/components/extensions/Extension.jsm:762
(Diff revision 1)
>        this.whiteListedHosts = new MatchPatternSet(
>          this.whiteListedHosts.patterns
>              .filter(host => !origins.includes(host.pattern)));
> +
> +      this.policy.permissions = Array.from(this.permissions);
> +      this.policy.allowedOrigins = this.whiteListedHosts;

Also for add-permissions?

::: toolkit/components/extensions/ExtensionChild.jsm:534
(Diff revision 1)
>        }
> +
> +      if (this.policy) {
> +        this.policy.permissions = Array.from(this.permissions);
> +        this.policy.allowedOrigins = this.whiteListedHosts;
> +      }

Shouldn't this be done in the add-permissions block above?

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871114 [details]
Bug 1322235: Part 5 - Add an ExtensionPolicyService singleton class to track active extension policies.

https://reviewboard.mozilla.org/r/142626/#review146582

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8871115 [details]
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

https://reviewboard.mozilla.org/r/142628/#review146564

> Also for add-permissions?

Yeah. I botched the rebase where I split these changes out of part 3. Good catch.

Comment 23

[Tomislav Jovanovic :zombie]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review146456

Skipping the WebIDL/JSAPI parts, the matching code looks fairly readable -- here's just a lot of it, so I'd like to go through once more.

I don't have strong opinions against throwing on empty/invalid patterns, but I don't think bounding that change in behavior with this big, multi-patch, many-KLOC performance-optimization bug is a good way to do it.  It can complicate all sorts of things down the line if people need to find the regression, back out, or do something else unusual.

I would strongly prefer if you maintained the existing behavior in this bug, and filed another bug to actually change this (and I volunteer to fix it ;).

::: dom/webidl/MatchPattern.webidl:102
(Diff revision 1)
> +
> +  /**
> +   * Returns true if any sub-pattern overlaps any sub-pattern the given
> +   * pattern set.
> +   */
> +  boolean overlaps(MatchPatternSet patternSet);

This isn't used anywhere.

::: dom/webidl/MatchPattern.webidl:105
(Diff revision 1)
> +   * pattern set.
> +   */
> +  boolean overlaps(MatchPatternSet patternSet);
> +
> +  /**
> +   * Returns true if any sub-pattern overlaps *every* sub-pattern the given

nit: sub-pattern *in* the given

::: toolkit/components/extensions/MatchGlob.h:75
(Diff revision 1)
> +  bool mIsPrefix = false;
> +
> +  JS::Heap<JSObject*> mRegExp;
> +};
> +
> +class MatchGlobSet final : public nsTArray<RefPtr<MatchGlob>>

Don't see this used either.

::: toolkit/components/extensions/MatchPattern.cpp:129
(Diff revision 1)
> +
> +const nsString&
> +URLInfo::Path() const
> +{
> +  if (mPath.IsEmpty()) {
> +    nsCString path;

Wouldn't this fail to cache when the path actually is empty?  It's not an issue for http/https, but could be for other protocols, and easy to miss when we add support for another.

::: toolkit/components/extensions/MatchPattern.cpp:257
(Diff revision 1)
> +  /***************************************************************************
> +   * Scheme
> +   ***************************************************************************/
> +  auto index = aPattern.FindChar(':');
> +  if (index <= 0) {
> +    return false;

It makes sense to throw on invalid patterns, but this *is* a change in behavior, especially for empty patterns.  Likely to break things when just one of the patterns in the set is invalid.

::: toolkit/components/extensions/MatchPattern.cpp:294
(Diff revision 1)
> +  }
> +
> +  if (host.EqualsLiteral("*")) {
> +    mMatchSubdomain = true;
> +  } else if (StringHead(host, 2).EqualsLiteral("*.")) {
> +    mDomain = NS_ConvertUTF16toUTF8(Substring(host, 2));

This accepts invalid domain patterns that the JS barks at, like "*.*.com".  Though it will never match any actual domains, we do wacky things for Subsumes() and Ovelaps() that may assume a valid /\w+/ only domain name.  (I don't think it's actually an issue, but it may be for some similar addition later).

In any case, if we throw on other kinds of invalid patterns, this should be consistent.

::: toolkit/components/extensions/MatchPattern.cpp:340
(Diff revision 1)
> +
> +    return false;
> +}
> +
> +bool
> +MatchPattern::Matches(const URLInfo& aURL, bool aExplicit) const

I can't find where the URI gets converted to `URLInfo` before being passed here.  If this is some WebIDL magic, I would find it convenient if it wasn't confusing.

::: toolkit/components/extensions/MatchPattern.cpp:350
(Diff revision 1)
> +
> +  if (!mSchemes->Contains(aURL.Scheme())) {
> +    return false;
> +  }
> +
> +  if (!DomainIsWildcard() && !MatchesDomain(aURL.Host())) {

Is the wildcard check here actually worth optimizing?

::: toolkit/components/extensions/MatchPattern.cpp:354
(Diff revision 1)
> +
> +  if (!DomainIsWildcard() && !MatchesDomain(aURL.Host())) {
> +    return false;
> +  }
> +
> +  if (mPath && !mPath->IsWildcard() && !mPath->Matches(aURL.Path())) {

nit: might read better if you return (the opposite of) this condition.

::: toolkit/components/extensions/MatchPattern.cpp:378
(Diff revision 1)
> +  if (!aCookie.IsDomain()) {
> +    return false;
> +  }
> +
> +  // Things get tricker for domain cookies. The extension needs to be able
> +  // to read any cookies that could be read any host it has permissions

nit: be read *by* any host

::: toolkit/components/extensions/MatchPattern.cpp:385
(Diff revision 1)
> +  // since the pattern "*://*.foo.example.com/" doesn't match ".example.com",
> +  // but it does match "bar.foo.example.com", which can read cookies
> +  // with the domain ".example.com".
> +  //
> +  // So, instead, we need to manually check our filters, and accept any
> +  // with hosts that end with our cookie's host.

This sentence is wrong, it was originally in the MatchPatternSet equivalent from the js version.

::: toolkit/components/extensions/MatchPattern.cpp:396
(Diff revision 1)
> +bool
> +MatchPattern::SubsumesDomain(const MatchPattern& aPattern) const
> +{
> +  nsAutoCString domain(aPattern.mDomain);
> +  if (aPattern.mMatchSubdomain) {
> +    domain.InsertLiteral("*.", 0);

This is a bit hackish, and it took me a while.  Either add a comment, or better yet, I think this could simply be:

    if (aPattern.mMatchSubdomain && !mMatchSubdomain && aPattern.mDomain == mDomain) {
      return false;
    }

::: toolkit/components/extensions/MatchPattern.cpp:463
(Diff revision 1)
> +    } else {
> +      RefPtr<MatchPattern> pattern = MatchPattern::Constructor(
> +        aGlobal, elem.GetAsString(), aOptions, aRv);
> +
> +      if (!pattern) {
> +        return nullptr;

Change of behavior for one invalid pattern in a set can easily be avoided here.

::: toolkit/components/extensions/MatchPattern.cpp:577
(Diff revision 1)
> +                       const nsAString& aGlob,
> +                       bool aAllowQuestion,
> +                       ErrorResult& aRv)
> +{
> +  RefPtr<MatchGlob> glob = new MatchGlob(aGlobal.GetAsSupports());
> +  if (!glob->Init(aGlobal.Context(), aGlob, aAllowQuestion)) {

When would this ever be falsy?  What argument value wouldn't produce a valid regex (that's still a string)?

::: toolkit/components/extensions/MatchPattern.cpp:640
(Diff revision 1)
> +    AutoJSAPI jsapi;
> +    jsapi.Init();
> +    JSContext* cx = jsapi.cx();
> +
> +    JSAutoCompartment ac(cx, mRegExp);
> +
> +    JS::RootedObject regexp(cx, mRegExp);
> +    JS::RootedValue result(cx);
> +
> +    nsString input(aString);
> +
> +    size_t index = 0;
> +    if (!JS_ExecuteRegExpNoStatics(cx, regexp, input.BeginWriting(), aString.Length(),

I'm presuming this is necessary, so it's surprising all this is needed each time you want to execute the same regex.

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:1
(Diff revision 1)
> -/* Any copyright is dedicated to the Public Domain.
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

nit: you are actually adding these on purpose, aren't you?  ;)

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:5
(Diff revision 1)
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> -
>  "use strict";
>  
> -Components.utils.import("resource://gre/modules/MatchPattern.jsm");
> +add_task(async function test_MatchPattern_matches() {

nit: not async

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review146456

We currently reject invalid patterns before we get to the point of creating MatchPattern objects. I'd rather not add new code to special case them just to remove it immediately after.

> This isn't used anywhere.

Correct, but it may be useful in the future, and I'd already written it before I realized we actually needed the overlapsAll behavior.

> Don't see this used either.

It's used in later patches in the series.

> Wouldn't this fail to cache when the path actually is empty?  It's not an issue for http/https, but could be for other protocols, and easy to miss when we add support for another.

It would, but we don't support any schemes which allow empty paths, or accept patterns without path components. In any case, the worst that would happen if we were to start accepting them is that do some extra lookups in the rare cases where we dealt with those URLs.

> It makes sense to throw on invalid patterns, but this *is* a change in behavior, especially for empty patterns.  Likely to break things when just one of the patterns in the set is invalid.

Possibly for consumers of WebRequest.jsm, but we validate patterns elsewhere before we try to create MatchPatterns from them.

> This accepts invalid domain patterns that the JS barks at, like "*.*.com".  Though it will never match any actual domains, we do wacky things for Subsumes() and Ovelaps() that may assume a valid /\w+/ only domain name.  (I don't think it's actually an issue, but it may be for some similar addition later).
> 
> In any case, if we throw on other kinds of invalid patterns, this should be consistent.

Yes, I thought about validating them, but we do regexp validation elsewhere, so this isn't likely to be an issue. I'm planning to add better error reporting in a follow-up.

> I can't find where the URI gets converted to `URLInfo` before being passed here.  If this is some WebIDL magic, I would find it convenient if it wasn't confusing.

URLInfo has an implicit conversion constructor from nsIURI.

> Is the wildcard check here actually worth optimizing?

Yes, because it means we don't have to do the hostname lookup when we're only dealing with wildcard patterns.

> This is a bit hackish, and it took me a while.  Either add a comment, or better yet, I think this could simply be:
> 
>     if (aPattern.mMatchSubdomain && !mMatchSubdomain && aPattern.mDomain == mDomain) {
>       return false;
>     }

It is a bit hackish, but I copied the logic from the JS implementation. But it's also not as hackish as it seems, because for the original pattern `*.domain`, `mDomain` is "domain", so except in the case of `*`, this gives us back the original domain pattern.

Anyway, no, that check wouldn't work, because `*.com` subsumes `*.foo.com`, but their `mDomain` members are not the same.

> When would this ever be falsy?  What argument value wouldn't produce a valid regex (that's still a string)?

At the moment, it can only be false for EOM.

> I'm presuming this is necessary, so it's surprising all this is needed each time you want to execute the same regex.

Most of it wouldn't be necessary if this was ordinarily called from JS. The need for the `nsString` instance is a bit unfortunate. Ideally it wouldn't be necessary, but for some reason, `JS_ExecuteRegExpNoStatics` takes a `char16_t` rather than a `const char16_t`.

> nit: you are actually adding these on purpose, aren't you?  ;)

Yes. They're techinically required by the code style guide, and the other tests in this directory have them.

> nit: not async

As a rule, all task functions are expected to be async. It may not actually await, but making it async from the start doesn't hurt anything, and prevents future headaches.

Comment 25

[Tomislav Jovanovic :zombie]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review146456

> Correct, but it may be useful in the future, and I'd already written it before I realized we actually needed the overlapsAll behavior.

I thought that's insufficient reason to add code?

> It is a bit hackish, but I copied the logic from the JS implementation. But it's also not as hackish as it seems, because for the original pattern `*.domain`, `mDomain` is "domain", so except in the case of `*`, this gives us back the original domain pattern.
> 
> Anyway, no, that check wouldn't work, because `*.com` subsumes `*.foo.com`, but their `mDomain` members are not the same.

I meant with the return below:

    if (aPattern.mMatchSubdomain && !mMatchSubdomain && aPattern.mDomain == mDomain) {
      return false;
    }

    return MatchesDomain(aPattern.mDomain);

AFAIU your reason for prefixing `domain` with `*.`, the only special case  where this function shouldn't simply `return MatchesDomain(domain)` is because `foo.com` doesn't subsume `*.foo.com`, which I think my test above covers.

> Yes. They're techinically required by the code style guide, and the other tests in this directory have them.

Well that seems outdated.

The only style guide I could find [1] mentions mode lines for all js files, it doesn't single out test files, and since we don't use them in (shipping) js files, this just feels wrong.

1) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 26

[Tomislav Jovanovic :zombie]

Comment on attachment 8871112 [details]
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

https://reviewboard.mozilla.org/r/142622/#review146966

I already thought that the way we were reaching into `MatchPattern` internals to store/add/remove permissions was icky, and with these changes, it got even uglier, especially because of normalization.  You'd likely rather avoid adding even more C++ to handle those, so I guess we could still encapsulate/abstract on the JS side later.

::: toolkit/components/extensions/Extension.jsm:548
(Diff revision 1)
>          this.apiNames.add(type.api);
>        }
> +
> +      this.permissions.add(perm);
>      }
> -    this.whiteListedHosts = new MatchPattern(whitelist);
> +    this.whiteListedHosts = new MatchPatternSet(whitelist, {ignorePath: true});

nit: `ignorePath` is ignored here

::: toolkit/components/extensions/ext-management.js:63
(Diff revision 1)
>      let m = extension.manifest;
> +
> +    let hostPerms = extension.whiteListedHosts.patterns.map(matcher => matcher.patttern);
> +
>      extInfo.permissions = Array.from(extension.permissions).filter(perm => {
> -      return !extension.whiteListedHosts.pat.includes(perm);
> +      return hostPerms.includes(perm);

Won't this fail to find with the normalization of pattern to end with `/*`?

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8871112 [details]
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

https://reviewboard.mozilla.org/r/142622/#review146966

Agree. I'm planning to cleanup the permission code as a follow-up.

> nit: `ignorePath` is ignored here

Yeah, I just figured it was better to be safe and include it.

> Won't this fail to find with the normalization of pattern to end with `/*`?

They shouldn't, because the patterns in both lists are normalized at extension startup. In any case, I'm planning to remove the remove this and just use the WebExtensionPolicy object directly in a follow-up.

Comment 28

[Tomislav Jovanovic :zombie]

Comment on attachment 8871112 [details]
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

https://reviewboard.mozilla.org/r/142622/#review146966

> Yeah, I just figured it was better to be safe and include it.

You should include it consistently in other places then. ;)

> They shouldn't, because the patterns in both lists are normalized at extension startup. In any case, I'm planning to remove the remove this and just use the WebExtensionPolicy object directly in a follow-up.

Yeah, I figured this out actually, minutes later when I added the "first" comment above, I just forgot to remove this, sorry.

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8871112 [details]
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

https://reviewboard.mozilla.org/r/142622/#review146966

> You should include it consistently in other places then. ;)

I think the only places where I didn't include it were the places we knew we were already dealing with normalized strings. I guess I may as well remove it here, though.

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review146456

> I thought that's insufficient reason to add code?

It depends. There's a lot more overhead to changing DOM bindings than there is to just adding a method to a JS module, so I'd rather have them as complete as possible from the start.

> I meant with the return below:
> 
>     if (aPattern.mMatchSubdomain && !mMatchSubdomain && aPattern.mDomain == mDomain) {
>       return false;
>     }
> 
>     return MatchesDomain(aPattern.mDomain);
> 
> AFAIU your reason for prefixing `domain` with `*.`, the only special case  where this function shouldn't simply `return MatchesDomain(domain)` is because `foo.com` doesn't subsume `*.foo.com`, which I think my test above covers.

You might be right. I'll have to think about it.

> Well that seems outdated.
> 
> The only style guide I could find [1] mentions mode lines for all js files, it doesn't single out test files, and since we don't use them in (shipping) js files, this just feels wrong.
> 
> 1) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Right, all JS files. Most of our other JS files also have modelines.

Comment 31

[Tomislav Jovanovic :zombie]

Comment on attachment 8871113 [details]
Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings.

https://reviewboard.mozilla.org/r/142624/#review146998

This is hard for me to review at this point as it doesn't actually do all that much by itself, so I'll give it another go in the next round.

(Also, there are tests for functionality that's missing, so I stopped reviewing right around my last comment)

::: dom/webidl/WebExtensionPolicy.webidl:51
(Diff revision 1)
> +   * The list of currently-active permissions for the extension, as specified
> +   * in its manifest.json file. May be updated to reflect changes in the
> +   * extension's optional permissions.
> +   */
> +  [Cached, Frozen, Pure]
> +  attribute sequence<DOMString> permissions;

This is only for API permissions, right?  If so, how about `apiPermissions`, especially since your comment mentions the manifest where `permissions` includes `allowedOrigins`.

::: dom/webidl/WebExtensionPolicy.webidl:85
(Diff revision 1)
> +   * Returns true if the extension currently has the given permission.
> +   */
> +  boolean hasPermission(DOMString permission);
> +
> +  /**
> +   * Returns true the given path relative to the extension's moz-extension:

true *if* the given path

::: dom/webidl/WebExtensionPolicy.webidl:92
(Diff revision 1)
> +   */
> +  boolean isPathWebAccessible(DOMString pathname);
> +
> +  /**
> +   * Replaces localization placeholders in the given string with localized
> +   * text from the extensin's currently active locale.

"extension's"

Also, is the active locale a property of the extension?

::: toolkit/components/extensions/WebExtensionPolicy.h:76
(Diff revision 1)
> +  bool HasPermission(const nsAString& aPermission) const
> +  {
> +    return mPermissions->Contains(aPermission);
> +  }
> +
> +  nsCString BackgroundPageHTML() const;

I see this is used in later patches, but is a Policy class the best place for this functionality?

::: toolkit/components/extensions/WebExtensionPolicy.cpp:22
(Diff revision 1)
> +
> +static inline Result<Ok, nsresult>
> +WrapNSResult(PRStatus aRv)
> +{
> +    if (aRv != PR_SUCCESS) {
> +        return Err(NS_ERROR_FAILURE);

nit: indentation

::: toolkit/components/extensions/WebExtensionPolicy.cpp:189
(Diff revision 1)
> +nsCString
> +WebExtensionPolicy::BackgroundPageHTML() const
> +{
> +  nsAutoCString result;
> +
> +  if (mBackgroundScripts.IsNull()) {

Can `sequence<DOMString>?` also be an empty sequence/array?

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js:72
(Diff revision 1)
> +
> +  // Web-accessible resources
> +
> +  ok(policy.isPathWebAccessible("/foo/bar"), "Web-accessible glob should be web-accessible");
> +  ok(policy.isPathWebAccessible("/bar.baz"), "Web-accessible path should be web-accessible");
> +  ok(!policy.isPathWebAccessible("/bar.baz/quux"), "Non-web-accessible path should not be web-accessible");

nit: not really great error messages.

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js:82
(Diff revision 1)
> +  equal(policy.localize("foo"), "<foo>", "Localization callback should work as expected");
> +
> +
> +  // Protocol and lookups.
> +
> +  let proto = Services.io.getProtocolHandler("moz-extension", uuid).QueryInterface(Ci.nsISubstitutingProtocolHandler);

Can we test the protocol substitutions here?  At least smoke-test for the common edge cases "//foo", "../../bar" and the like?
(I know this would basically be testing nsIURI, which is likely pointless, but things can change, and tests can catch unexpected stuff)

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js:84
(Diff revision 1)
> +
> +  // Protocol and lookups.
> +
> +  let proto = Services.io.getProtocolHandler("moz-extension", uuid).QueryInterface(Ci.nsISubstitutingProtocolHandler);
> +
> +  deepEqual(WebExtensionPolicy.getActiveExtensions(), [], "Should have no active extensions");

How does this work?  Where is it implemented?  WebIDL magic is more powerful than I thought! :P

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8871113 [details]
Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings.

https://reviewboard.mozilla.org/r/142624/#review146998

> This is only for API permissions, right?  If so, how about `apiPermissions`, especially since your comment mentions the manifest where `permissions` includes `allowedOrigins`.

No, this includes origin permissions.

> "extension's"
> 
> Also, is the active locale a property of the extension?

Sort of. I don't think it needs to be exposed on the policy object for the moment, though.

> I see this is used in later patches, but is a Policy class the best place for this functionality?

Probably not, but it's one of the things the old policy service got hijacked for, and I didn't want to get side-tracked bikeshedding over a better name.

> Can `sequence<DOMString>?` also be an empty sequence/array?

Yes, but there's a difference between an empty array (a generated background page which loads no scripts) and null (no generated background page at all).

> Can we test the protocol substitutions here?  At least smoke-test for the common edge cases "//foo", "../../bar" and the like?
> (I know this would basically be testing nsIURI, which is likely pointless, but things can change, and tests can catch unexpected stuff)

Those are already tested in necko tests. This just tests that that we correctly register and unregister the base URL.

> How does this work?  Where is it implemented?  WebIDL magic is more powerful than I thought! :P

Oh, I guess I rebased this into the wrong commit. It was supposed to be in part 5.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871115 [details]
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

https://reviewboard.mozilla.org/r/142628/#review147212

r+ with add-permissions fixed

Comment 34

[Tomislav Jovanovic :zombie]

Comment on attachment 8871115 [details]
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

https://reviewboard.mozilla.org/r/142628/#review147180

::: toolkit/components/extensions/ExtensionManagement.jsm:105
(Diff revision 1)
> +    policy.active = true;
> +  },
> +
> +  // Called when an extension is unloaded.
> +  shutdownExtension(extension) {
> +    extension.policy.active = false;

Shouldn't this also forget (null) the policy?

::: toolkit/components/extensions/ExtensionPolicyService.cpp:190
(Diff revision 1)
> +}
> +
> +nsresult
> +ExtensionPolicyService::ExtensionURILoadableByAnyone(nsIURI* aURI, bool* aResult)
> +{
> +  URLInfo url(aURI);

What is gained by converting to URLInfo here?

::: toolkit/components/utils/simpleServices.js:61
(Diff revision 1)
>  
>    // aContext must be a nsIURI object for a valid moz-extension: URL.
> -  getAddonId(aContext) {
> +  getAddon(aContext) {
>      // In this case, we want the add-on ID even if the URL is web accessible,
>      // so check the root rather than the exact path.
>      let uri = Services.io.newURI("/", null, aContext);

This step doesn't seem necessary anymore.

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8871115 [details]
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

https://reviewboard.mozilla.org/r/142628/#review147180

> Shouldn't this also forget (null) the policy?

It could, but there's no particular reason it should be necessary.

> What is gained by converting to URLInfo here?

GetByURL only knows how to handle URLInfo objects, so we wind up with one anyway. If we create it explicltly, though, we can use the Path() getter rather than fetch the path from the URI object again.

Comment 36

[Andrew Swan [:aswan]]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review147238

The jsapi bits are beyond me but the actual mechanics look okay.

::: toolkit/components/extensions/MatchPattern.h:150
(Diff revision 1)
> +  nsIAtom* Scheme() const;
> +  const nsCString& Host() const;
> +  const nsString& Path() const;
> +  const nsString& Spec() const;
> +
> +  bool InheritsPrincipal() const;

this doesn't appear to be used

::: toolkit/components/extensions/MatchPattern.h:168
(Diff revision 1)
> +
> +  mutable Maybe<bool> mInheritsPrincipal;
> +};
> +
> +
> +// Likewise for cookies.

This is far enough away from the comment you're referencing it, maybe just repeat it

::: toolkit/components/extensions/MatchPattern.h:205
(Diff revision 1)
> +  Constructor(dom::GlobalObject& aGlobal,
> +              const nsAString& aPattern,
> +              const MatchPatternOptions& aOptions,
> +              ErrorResult& aRv);
> +
> +  bool Matches(const URLInfo& aURL, bool aExplicit = false) const;

Just for my own curiosity, is there any point having the default value here?  I would assume wrappers generated for webidl would handle defaults and explicitly pass all arguments always.

::: toolkit/components/extensions/MatchPattern.h:246
(Diff revision 1)
> +  nsCString mDomain;
> +  bool mMatchSubdomain = false;

It isn't completely evident from the names, a comment explaining how these fields are meant to be set for various cases (i.e., `*` vs `*.domain.com` vs `literal.domain.com`) would be helpful.

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:40
(Diff revision 1)
> +
> +  function fail({url, pattern, normalized}) {
> +    ok(!test(url, pattern, normalized), `Expected no match: ${JSON.stringify(pattern)}, ${url}`);
> +  }
> +
> +  function invalid({url, pattern}) {

why does this take a url as a parameter

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:163
(Diff revision 1)
>    pass({hosts: "http://*/", filter: "http://ab.cd/"});
>    fail({hosts: "http://*/", filter: "https://ab.cd/"});
>  
>    // Wildcard wildcards.
>    pass({hosts: "<all_urls>", filter: "ftp://ab.cd/"});
> -  fail({hosts: "<all_urls>", filter: ""});
> +  // fail({hosts: "<all_urls>", filter: ""});

what's the deal here?  this now throws?  just remove it instead of commenting it out?

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:202
(Diff revision 1)
> +  let moz = "http://mozilla.org";
> +
> +  pass({url: moz, pattern: ["*"]});
> +  pass({url: moz, pattern: ["http://*"]});
> +  pass({url: moz, pattern: ["*mozilla*"]});
> +  // pass({url: moz, pattern: ["*example*", "*mozilla*"]});

whatever you were working on here, either finish it or remove this?

::: toolkit/components/extensions/test/xpcshell/test_MatchPattern.js:226
(Diff revision 1)
> +  // Trailing slash
> +  pass({url: moz, pattern: ["*.org/"]});
> +  fail({url: moz, pattern: ["*.org"]});
> +
> +  // Wrong TLD
> +  fail({url: moz, pattern: ["www*.m*.com/"]});

this is not just the wrong tld, `moz` does not begin with `www.`

Comment 37

[Andrew Swan [:aswan]]

Comment on attachment 8871111 [details]
Bug 1322235: Part 2 - Use MatchGlob for webAccessibleResources.

https://reviewboard.mozilla.org/r/142620/#review147248

::: toolkit/components/extensions/Extension.jsm:904
(Diff revision 1)
>  
>    runManifest(manifest) {
> -    // Strip leading slashes from web_accessible_resources.
> +    let resources = [];
> -    let strippedWebAccessibleResources = [];
>      if (manifest.web_accessible_resources) {
> -      strippedWebAccessibleResources = manifest.web_accessible_resources.map(path => path.replace(/^\/+/, ""));
> +      resources = manifest.web_accessible_resources.map(path => path.replace(/^\/*/, "/"));

can you add a comment, eg "normalize paths to all contain a single leading slash"

Comment 38

[Andrew Swan [:aswan]]

Comment on attachment 8871112 [details]
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

https://reviewboard.mozilla.org/r/142622/#review147252

I'm disappointed you didn't rename `whiteListedHosts` while you were here :-)

::: toolkit/components/extensions/Extension.jsm:541
(Diff revision 1)
>        let type = classifyPermission(perm);
>        if (type.origin) {
> -        whitelist.push(perm);
> +        let matcher = new MatchPattern(perm, {ignorePath: true});
> +
> +        whitelist.push(matcher);
> +        perm = matcher.pattern;

I'm not a fan of using the normalized permissions here.  In the permissions api we're finally rightfully calling these things "origins" but the wildcard paths are ugly.

::: toolkit/modules/addons/MatchPattern.jsm:115
(Diff revision 1)
>    overlapsIgnoringPath(other) {
>      return this.schemes.some(scheme => other.schemes.includes(scheme)) &&
>             (this.hostMatch(other) || other.hostMatch(this));
>    },
> +
> +  get pattern() { return this.pat; },

Is this (and the other change in this file) actually needed?  Or just something you used during development?

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review147238

> this doesn't appear to be used

It's used in the follow-ups for content script matching.

> Just for my own curiosity, is there any point having the default value here?  I would assume wrappers generated for webidl would handle defaults and explicitly pass all arguments always.

The default value isn't necessary for WebIDL consumers, but I'm planning to use these objects directly in CAPS code, where the default values will be useful.

> why does this take a url as a parameter

Not for any particular reason. I was just trying to minimize the changes I made to the original MatchPattern.jsm tests.

> what's the deal here?  this now throws?  just remove it instead of commenting it out?

Yeah, I meant to remove it.

> this is not just the wrong tld, `moz` does not begin with `www.`

Eh, I didn't write it...

Comment 40

[Kris Maglione [:kmag]]

Comment on attachment 8871112 [details]
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest.

https://reviewboard.mozilla.org/r/142622/#review147252

I considered it...

> I'm not a fan of using the normalized permissions here.  In the permissions api we're finally rightfully calling these things "origins" but the wildcard paths are ugly.

We have to normalize them one way or the other. People can specify patterns with any path glob, even though the path is ignored, and we shouldn't wind up treating `http://foo.com/*` differently from `http://foo.com/`. I'd rather keep the normalized variant with the `/*`, since it more closely matches the reality of its behavior.

> Is this (and the other change in this file) actually needed?  Or just something you used during development?

For the moment, it's still needed for WebRequest.jsm, which deals with MatchPattern.jsm as well as native match patterns. I'm planning to remove MatchPattern.jsm altogether as soon as I can, but I need to make sure there aren't any extensions that depend on it first.

Comment 41

[Andrew Swan [:aswan]]

Comment on attachment 8871113 [details]
Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings.

https://reviewboard.mozilla.org/r/142624/#review147256

> Andrew and zombie, can you please review the policy logic and tests?

WebExtensionPolicy is mostly a dumb container with not much "logic" to review.  The test contains a bunch of stuff that isn't actually implemented in this patch that I presume just comes in a later patch.  r+ assuming you'll just move that stuff to the right place.

::: toolkit/components/extensions/WebExtensionPolicy.h:57
(Diff revision 1)
> +
> +  void GetURL(const nsAString& aPath, nsAString& aURL, ErrorResult& aRv) const;
> +
> +  Result<nsString, nsresult> GetURL(const nsAString& aPath) const;
> +
> +  bool CanAccessURI(nsIURI* aURI, bool aExplicit = false) const

There's no `explicit` argument in the webidl...

Comment 42

[Andrew Swan [:aswan]]

Do you have any data on the improvement we get from these patches?  I guess that begs the question of what sort of workload should be measured, ugh.

Comment 43

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #42)
> Do you have any data on the improvement we get from these patches?  I guess
> that begs the question of what sort of workload should be measured, ugh.

Not particularly good data. It would be help if we had an add-on that was affected by these changes installed by default so it would show up in talos. But in the profiling I've done locally, the places where this used to show a huge amount of overhead don't show up in profiles at all. For Screenshots, for instance, we used to have 10ms at startup just in extensionURILoadableByAnyone callbacks, which have completely disappeared by profiles. Similar for content script matching. That used to reliably show up in talos profiles for tests like sessionrestore, and cause considerable overhead, but doesn't show up at all now.

I still have some more follow-ups that depend on this to avoid loading even more code before we have to, which should remove most of the remaining tabpaint/tpaint overhead.

Comment 44

[Kris Maglione [:kmag]]

Comment on attachment 8871113 [details]
Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings.

https://reviewboard.mozilla.org/r/142624/#review147256

> There's no `explicit` argument in the webidl...

Yeah, it's only used on the native side, but it should probably be in the WebIDL bindings too.

Comment 45

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #42)
> Do you have any data on the improvement we get from these patches?  I guess
> that begs the question of what sort of workload should be measured, ugh.

Unfortunately, the presence of Screenshots is making it difficult to get useful talos numbers at this point. They seem to have gotten very lucky with their landing, and changes which improve startup performance actually wind up making ts_paint and sessionrestore numbers worse with screenshots enabled, presumably because they give some tasks a chance to run earlier, and delay first paint and slow down sessionrestore.

That said, so far, the combination of this and the follow-up bugs seem to speed up ts_paint_webext e10s by about 12%, and responsiveness by about 10%.

On Windows, they seem to speed up ordinary ts_paint by about 2.5% and slow down sessionrestore by 8-10%. On Linux, they seem to slow down the ordinary ts_paint and sessionrestore tests by about 4-8%. The huge number of background scripts that Screenshots still loads at startup probably has a lot to do with this.

Comment 46

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review147152

::: toolkit/components/extensions/MatchGlob.h:12
(Diff revision 1)
> +#define mozilla_extensions_MatchGlob_h
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/MatchGlobBinding.h"
> +
> +#include "jsapi.h"

I think you might just need RootingAPI.h and jspubtd.h. Please try very hard to avoid including jsapi.h.

::: toolkit/components/extensions/MatchGlob.h:30
(Diff revision 1)
> +                      , public nsWrapperCache
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(MatchGlob)
> +

This file has a lot of blank lines that seem unecessary to me.

::: toolkit/components/extensions/MatchGlob.h:69
(Diff revision 1)
> +
> +  nsCOMPtr<nsISupports> mParent;
> +
> +  nsString mGlob;
> +
> +  nsString mPathLiteral;

Please explain what these fields mean. Things like: if mPathLiteral is non-empty, then...

::: toolkit/components/extensions/MatchPattern.h:13
(Diff revision 1)
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/MatchPatternBinding.h"
> +#include "mozilla/extensions/MatchGlob.h"
> +
> +#include "jsapi.h"

I think jspubtd.h should be enough here.

::: toolkit/components/extensions/MatchPattern.h:22
(Diff revision 1)
> +#include "nsIAtom.h"
> +#include "nsICookie2.h"
> +#include "nsISupports.h"
> +#include "nsIURI.h"

Is it possible to forward-declare some of these instead of including them?

::: toolkit/components/extensions/MatchPattern.h:96
(Diff revision 1)
> +    RefPtr<AtomSet> matcher = sMatcher;
> +    return matcher.forget();

return do_AddRef(sMatcher);

::: toolkit/components/extensions/MatchPattern.h:246
(Diff revision 1)
> +  nsCString mDomain;
> +  bool mMatchSubdomain = false;

Agreed. Also, the name |mMatchSubdomain| doesn't make much sense to me. What about |mStarInDomain|?

::: toolkit/components/extensions/MatchPattern.cpp:32
(Diff revision 1)
> +{
> +  mElems.SetCapacity(aElems.Length());
> +
> +  for (const auto& elem : aElems) {
> +    RefPtr<nsIAtom> atom = NS_AtomizeMainThread(elem);
> +    if (!mElems.Contains(atom)) {

It would be more efficient to remove duplicates after sorting, but maybe you don't care how fast this is.

::: toolkit/components/extensions/MatchPattern.cpp:42
(Diff revision 1)
> +  mElems.Sort();
> +}
> +
> +AtomSet::AtomSet(const char** aElems)
> +{
> +  for (; *aElems; aElems++) {

Ideally we would not modify aFoo parameters inside the function. Also, I think this would be nicer if you use a loop variable.

::: toolkit/components/extensions/MatchPattern.cpp:84
(Diff revision 1)
> +  if (!Contains(aAtom)) {
> +    mElems.AppendElement(aAtom);
> +    mElems.Sort();
> +  }

This would be more efficient if you used IndexOfFirstElementGt combined with InsertElementAt.

::: toolkit/components/extensions/MatchPattern.cpp:129
(Diff revision 1)
> +
> +const nsString&
> +URLInfo::Path() const
> +{
> +  if (mPath.IsEmpty()) {
> +    nsCString path;

Please add a comment to that effect then.

::: toolkit/components/extensions/MatchPattern.cpp:272
(Diff revision 1)
> +  }
> +
> +  /***************************************************************************
> +   * Host
> +   ***************************************************************************/
> +  auto offset = index + 1;

I didn't read through this code very carefully, but I think it would be clearer if you had integer variables that were specifically for the start and end of whatever segment you're currently examining. Right now, index and offset seem like interchangeable variables that mean differnet things at different times.

::: toolkit/components/extensions/MatchPattern.cpp:273
(Diff revision 1)
> +
> +  /***************************************************************************
> +   * Host
> +   ***************************************************************************/
> +  auto offset = index + 1;
> +  auto tail = Substring(aPattern, offset);

All of these auto variables are really confusing. Generally I think the convention is to avoid auto except in places where the type is obvious (like assigning from a static_cast) or long and awkward (like an iterator type).

::: toolkit/components/extensions/MatchPattern.cpp:324
(Diff revision 1)
> +
> +
> +bool
> +MatchPattern::MatchesDomain(const nsACString& aDomain) const
> +{
> +    if (DomainIsWildcard() || mDomain == aDomain) {

Indent is wrong in this function.

::: toolkit/components/extensions/MatchPattern.cpp:329
(Diff revision 1)
> +    if (DomainIsWildcard() || mDomain == aDomain) {
> +      return true;
> +    }
> +
> +    if (mMatchSubdomain) {
> +      int32_t offset = (int32_t)aDomain.Length() - mDomain.Length();

Maybe there's some reason it's okay to convert a uint32_t to an int32_t here, but can you just use an int64_t here to say everyone some brain cells?

::: toolkit/components/extensions/MatchPattern.cpp:629
(Diff revision 1)
> +  escaped.Append('$');
> +
> +  // TODO: Switch to the Rust regexp crate, when Rust integration is easier.
> +  // It uses a much more efficient, linear time matching algorithm, and
> +  // doesn't require special casing for the literal and prefix cases.
> +  mRegExp = JS_NewUCRegExpObject(aCx, escaped.get(), escaped.Length(), 0);

If this fails, I think you need to call NoteJSContextException on the ErrorResult, so I think you'll need to re-jigger the error handling here.

Comment 47

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8871113 [details]
Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings.

https://reviewboard.mozilla.org/r/142624/#review149422

::: toolkit/components/extensions/WebExtensionPolicy.h:13
(Diff revision 1)
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/WebExtensionPolicyBinding.h"
> +#include "mozilla/extensions/MatchPattern.h"
> +
> +#include "jsapi.h"

Again, please avoid jsapi.h if you can. In this case I don't see anything requiring it.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This should have a modeline.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:38
(Diff revision 1)
> +    return Ok();
> +}
> +
> +#define NS_TRY(expr) MOZ_TRY(WrapNSResult(expr))
> +
> +#define PROTO "moz-extension"

Could this just be a static const variable? (Same for the others below.)

Comment 48

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8871114 [details]
Bug 1322235: Part 5 - Add an ExtensionPolicyService singleton class to track active extension policies.

https://reviewboard.mozilla.org/r/142626/#review149430

::: toolkit/components/extensions/ExtensionPolicyService.cpp:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please add a modeline.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:142
(Diff revision 1)
> +}
> +
> +/* static */ already_AddRefed<WebExtensionPolicy>
> +WebExtensionPolicy::GetByID(dom::GlobalObject& aGlobal, const nsAString& aID)
> +{
> +  RefPtr<WebExtensionPolicy> result = EPS().GetByID(aID);

You can use do_AddRef for all of these.

Comment 49

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8871116 [details]
Bug 1322235: Part 7 - DeCOMtaminate moz-extension protocol handler.

https://reviewboard.mozilla.org/r/142630/#review149432

Comment 50

[Kris Maglione [:kmag]]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review147152

> Is it possible to forward-declare some of these instead of including them?

No, not without switching to nsCOMPtr to RefPtr for the members with those types, anyway. I'd be fine with that, but most people seem to frown on it.

> return do_AddRef(sMatcher);

Oh, that's new.

> It would be more efficient to remove duplicates after sorting, but maybe you don't care how fast this is.

Hm. Yeah, I guess this is O(n log n), which isn't great.

> Maybe there's some reason it's okay to convert a uint32_t to an int32_t here, but can you just use an int64_t here to say everyone some brain cells?

I don't think it's possible to have a domain name that's more than a GB long, and it would have to be closer to 4GB to overflow back into a positive integer, but, yeah, it should probably be int64_t.

Comment 51

[Kris Maglione [:kmag]]

Comment on attachment 8871110 [details]
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings.

https://reviewboard.mozilla.org/r/142618/#review147152

> Agreed. Also, the name |mMatchSubdomain| doesn't make much sense to me. What about |mStarInDomain|?

I think `mMatchSubdomain` is a better description of the actual behavior. `*.foo.com` matches foo.com and subdomains of foo.com. `foo.*` has a star in it, but only matches the literal domain `"foo.*"`. But I'll add comments to make the behavior clearer.

Comment 52

[Kris Maglione [:kmag]]

Comment on attachment 8871115 [details]
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService.

https://reviewboard.mozilla.org/r/142628/#review147180

> This step doesn't seem necessary anymore.

Yeah, it hasn't been necessary for a long time.

Comment 53

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94a3fb9eb7e4c933ae318ff9b0a5bc9a3ac28e75
Bug 1322235: Part 1 - Add native MatchPattern and MatchGlob bindings. r=billm,aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/50e2a20caa7a189c0ed5a8679f483a5b10100aaf
Bug 1322235: Part 2 - Use MatchGlob for webAccessibleResources. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/34316bd2fecfb2802b135ea4f8592468c5fc7618
Bug 1322235: Part 3 - Use MatchPatternSet for whiteListedHosts and webRequest. r=aswan,zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/19d92b19910f3ce9bfa2b2874d17635fe323080b
Bug 1322235: Part 4 - Add initial native WebExtensionPolicy bindings. r=billm,aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/02ce614daf1999405cd4e63d60ab2b6ebd4e4048
Bug 1322235: Part 5 - Add an ExtensionPolicyService singleton class to track active extension policies. r=billm,mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/35e67c38d3cd93cc43f4665849847959f7d97656
Bug 1322235: Part 6 - Replace AddonPolicyService with a stub implementation in ExtensionPolicyService. r=mixedpuppy,zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6001a0646bb46df0266896fe5bf2069b6cfadae
Bug 1322235: Part 7 - DeCOMtaminate moz-extension protocol handler. r=billm

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/94a3fb9eb7e4
https://hg.mozilla.org/mozilla-central/rev/50e2a20caa7a
https://hg.mozilla.org/mozilla-central/rev/34316bd2fecf
https://hg.mozilla.org/mozilla-central/rev/19d92b19910f
https://hg.mozilla.org/mozilla-central/rev/02ce614daf19
https://hg.mozilla.org/mozilla-central/rev/35e67c38d3cd
https://hg.mozilla.org/mozilla-central/rev/e6001a0646bb

Comment 55

[u462496]

Kris, thanks for all your time on this, that looks like it was hard work!

Comparing times per the method outlined in comment 5:

48% speed improvement on opening and displaying the browserAction menu (old: 679ms, new: 460ms).  Yay!

This leaves comparison with XUL popup as follows:

browserAction popup is now 5.28x slower than XUL popup  (87ms vs 460ms) compared to previously being 7.99x slower.

With my user profile which is relatively lightweight, browserAction popup opening time increases to 817ms.

Is this now the most we'll ever be able to expect out of browserAction popups?  Is this performance difference due to the difference in time to add XUL elements to the DOM as opposed to HTML?  Or is it due to creating and displaying the popup itself?

I know you're very busy and I don't expect a snappy answer on this.

Comment 56

[Kris Maglione [:kmag]]

(In reply to Kevin Jones from comment #55)
> Is this now the most we'll ever be able to expect out of browserAction
> popups?  Is this performance difference due to the difference in time to add
> XUL elements to the DOM as opposed to HTML?  Or is it due to creating and
> displaying the popup itself?

We now have telemetry on the amount of time it takes to open browserAction popups after the mouseup event:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-06-27&keys=__none__!__none__!__none__&max_channel_version=nightly%252F56&measure=WEBEXT_BROWSERACTION_POPUP_OPEN_MS&min_channel_version=null&processType=*&product=Firefox&sanitize=0&sort_keys=submissions&start_date=2017-06-20&table=0&trim=1&use_submission_date=0

and the vast majority of popups open in <200ms, which is generally recognized as the upper limit of what humans perceive as instantaneous. The median is about 30ms, and the 75th percentile is 70ms, so I think it's fair to say that it's up to the extension authors to make their popups faster at this point.

Comment 57

[u462496]

(In reply to Kris Maglione [:kmag] from comment #56)
> (In reply to Kevin Jones from comment #55)
> > Is this now the most we'll ever be able to expect out of browserAction
> > popups?  Is this performance difference due to the difference in time to add
> > XUL elements to the DOM as opposed to HTML?  Or is it due to creating and
> > displaying the popup itself?
> 
> We now have telemetry on the amount of time it takes to open browserAction
> popups after the mouseup event:
> 
> https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2017-06-27&keys=__none__!__none__!
> __none__&max_channel_version=nightly%252F56&measure=WEBEXT_BROWSERACTION_POPU
> P_OPEN_MS&min_channel_version=null&processType=*&product=Firefox&sanitize=0&s
> ort_keys=submissions&start_date=2017-06-
> 20&table=0&trim=1&use_submission_date=0
> 
> and the vast majority of popups open in <200ms, which is generally
> recognized as the upper limit of what humans perceive as instantaneous. The
> median is about 30ms, and the 75th percentile is 70ms, so I think it's fair
> to say that it's up to the extension authors to make their popups faster at
> this point.

Good enough.  Thanks.