Closed Bug 1368102 Opened 7 years ago Closed 7 years ago

Move content script and extension page matching into C++

Categories

(WebExtensions :: General, enhancement, P1)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(Performance Impact:high, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [triaged])

Attachments

(8 files)

Bug 1368102: Part 1 - Make AddonManagerWebAPI::IsValidSite public.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
billm
: review+
Details
Bug 1368102: Part 3 - Use MatchPattern and MatchGlob bindings for content script matching.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1368102: Part 4 - Use WebExtensionContentScript to match content scripts.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
zombie
: review+
Details
Bug 1368102: Part 5 - Move static content script matching into C++.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
zombie
: review+
Details
Bug 1368102: Part 6 - Remove StubExtension and use WebExtensionPolicy directly.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1368102: Part 7 - Remove ScriptMatcher and use WebExtensionConentScript directly.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1368102: Part 8 - Move extension page matching into C++.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
billm
: review+
Details 
The content script and page matching code needs to be run against every new document, in every process, and the overhead begins to add up quickly.
Blocks: 1368152
Comment on attachment 8871848 [details]
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.

https://reviewboard.mozilla.org/r/143346/#review147236

Unless I missed something, this won't work with a non-zero frameID.  Or if it does, add a test that confirms it.

::: dom/webidl/WebExtensionContentScript.webidl:68
(Diff revision 1)
> +   */
> +  [Constant]
> +  readonly attribute boolean allFrames;
> +
> +  /**
> +   * If true, this (misleadingly-names, but inherited from Chrome) attribute

nit: *named

::: dom/webidl/WebExtensionContentScript.webidl:138
(Diff revision 1)
> +  /**
> +   * A set of paths, relative to the extension root, of JavaScript scripts to
> +   * execute in matching pages.
> +   */
> +  [Cached, Constant, Frozen]
> +  readonly attribute sequence<DOMString> jsPaths;

What about jsCode?  I guess I'll figure it out in a later patch...

::: toolkit/components/extensions/WebExtensionContentScript.h:61
(Diff revision 1)
> +  using Window = nsPIDOMWindowOuter*;
> +  using LoadInfo = nsILoadInfo*;

nit: can these be defined (and used) in the public section above?

::: toolkit/components/extensions/WebExtensionPolicy.cpp:347
(Diff revision 1)
> +{
> +  if (!mFrameID.IsNull() && aDoc.FrameID() != mFrameID.Value()) {
> +    return false;
> +  }
> +
> +  if (!mAllFrames && !aDoc.IsTopLevel()) {

Wont this return prematurely for a non-zero `FrameID` and false `AllFrames`?

::: toolkit/components/extensions/WebExtensionPolicy.cpp:351
(Diff revision 1)
> +  if (!mMatchAboutBlank && aDoc.URL().InheritsPrincipal()) {
> +    return false;
> +  }
> +
> +  if (!MatchesURI(aDoc.PrincipalURL())) {
> +    return false;
> +  }

I think you need a special case for top-level about:blank tab.

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionContentScript.js:35
(Diff revision 1)
> +  ok(contentScript.matchesURI(newURI("http://foo.com/bar")),
> +     "Simple matches include should match");
> +
> +  ok(contentScript.matchesURI(newURI("https://bar.com/baz/xflergx")),
> +     "Simple matches include should match");
> +

Missing negative test for `includeGlobs`.

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionContentScript.js:133
(Diff revision 1)
> +      topLevel: false,
> +      iframe: false,
> +      aboutBlank: false,
> +      srcdoc: false,
> +    },
> +  ];

Missing a test for frameID, which I believe you should be able to do here (from a xpcshell test).

::: toolkit/components/extensions/test/xpcshell/test_WebExtensionContentScript.js:156
(Diff revision 1)
> +      }
> +    }
> +  }
> +
> +  webNav.close();
> +  void requests;

What's this?
Attachment #8871848 - Flags: review?(tomica)
Comment on attachment 8871849 [details]
Bug 1368102: Part 3 - Use MatchPattern and MatchGlob bindings for content script matching.

https://reviewboard.mozilla.org/r/143348/#review147312

This seems to be rebased on top of a patch that never got submitted to bug 1322235.

::: toolkit/components/extensions/ExtensionTabs.jsm:553
(Diff revision 1)
>      if (this.hasActiveTabPermission) {
>        // If we have the "activeTab" permission for this tab, ignore
>        // the host whitelist.
> -      options.matchesHost = ["<all_urls>"];
> +      options.matches = ["<all_urls>"];
>      } else {
> -      options.matchesHost = [this.extension.whiteListedHosts.patterns.map(host => host.pattern)];
> +      options.matches = [this.extension.whiteListedHosts.patterns.map(host => host.pattern)];

Too many brackets?

::: toolkit/components/extensions/extension-process-script.js:93
(Diff revision 1)
>  
>      return true;
>    }
>  
>    matchesURI(uri) {
> -    if (!(this.matches.matches(uri) || this.matchesHost.matchesIgnoringPath(uri))) {
> +    if (!(this.matches.matches(uri))) {

nit: parentheses
Attachment #8871849 - Flags: review?(tomica)
Comment on attachment 8871848 [details]
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.

https://reviewboard.mozilla.org/r/143346/#review147236

> What about jsCode?  I guess I'll figure it out in a later patch...

That's only used for dynamically injected scripts, so the binding doesn't need to know anything about it, and it's added as an expando when it's needed.

> nit: can these be defined (and used) in the public section above?

No, they're implementation details that make it easier to access the variant types without worrying about the exact details of how they're stored.

> Wont this return prematurely for a non-zero `FrameID` and false `AllFrames`?

Yes.

> I think you need a special case for top-level about:blank tab.

I don't think so. All of the tests for about:blank matching pass without any additional special cases.

> Missing a test for frameID, which I believe you should be able to do here (from a xpcshell test).

frameID matching is tested elsewhere. I'm not entirely opposed to adding it here to, but I was trying to stay away from the details of dynamic injection as much as possible.

> What's this?

It was there to shut ESLint up while I was developing. I didn't notice it was still there.
Comment on attachment 8871849 [details]
Bug 1368102: Part 3 - Use MatchPattern and MatchGlob bindings for content script matching.

https://reviewboard.mozilla.org/r/143348/#review147312

No, I just accidentally rebased the one-line options.matchesHost change in ExtensionTabs.jsm onto an earlier patch after I pushed.

> Too many brackets?

Hm. Yeah, I must have only run the static content script tests after I changed this.
Blocks: 1368289
Comment on attachment 8871850 [details]
Bug 1368102: Part 4 - Use WebExtensionContentScript to match content scripts.

https://reviewboard.mozilla.org/r/143350/#review147322

::: toolkit/components/extensions/extension-process-script.js:141
(Diff revision 1)
>          let extension = ExtensionManager.get(recipient.extensionId);
> -        let script = new ScriptMatcher(extension, data.options);
>  
> +        let matcher = new WebExtensionContentScript(extension.policy, parseScriptOptions(data.options));
> +
> +        let options = Object.assign(matcher, {

nit: why `options` here?  `matcher` is already a more descriptive name for the argument you pass to `ScriptMatcher()`.

::: toolkit/components/extensions/extension-process-script.js:491
(Diff revision 1)
> +
> +    this.scripts = this.policy.contentScripts.map(matcher => new ScriptMatcher(this, matcher));
>    }
>  
>    startup() {
>      // Extension.jsm takes care of this in the parent.

Move or update the comment please, it's not clear what it refers to now that there's an `else` block below.

::: toolkit/components/extensions/extension-process-script.js:496
(Diff revision 1)
>      // Extension.jsm takes care of this in the parent.
>      if (isContentProcess) {
>        let uri = Services.io.newURI(this.data.resourceURL);
>        ExtensionManagement.startupExtension(this.uuid, uri, this);
> +    } else {
> +      this.policy = WebExtensionPolicy.getByID(this.id);

This doesn't seem like it shouldn't run in the content process.
Attachment #8871850 - Flags: review?(tomica) → review+
Comment on attachment 8871850 [details]
Bug 1368102: Part 4 - Use WebExtensionContentScript to match content scripts.

https://reviewboard.mozilla.org/r/143350/#review147322

> nit: why `options` here?  `matcher` is already a more descriptive name for the argument you pass to `ScriptMatcher()`.

The options variable goes away in a follow-up that removes the ScriptMatcher class.

> This doesn't seem like it shouldn't run in the content process.

In the content process, the policy object is always created in the process script, so it never exists at this point. In the parent process, it's created in Extension.jsm, and always exists at this point. In any case, this `if` goes away in bug 1368152.
Comment on attachment 8871847 [details]
Bug 1368102: Part 1 - Make AddonManagerWebAPI::IsValidSite public.

https://reviewboard.mozilla.org/r/143344/#review147346
Attachment #8871847 - Flags: review?(aswan) → review+
Comment on attachment 8871851 [details]
Bug 1368102: Part 5 - Move static content script matching into C++.

https://reviewboard.mozilla.org/r/143352/#review147344

::: toolkit/components/extensions/ExtensionPolicyService.cpp:160
(Diff revision 1)
> +ExtensionPolicyService::RegisterObservers()
> +{
> +  mObs->AddObserver(this, "content-document-global-created", false);
> +  mObs->AddObserver(this, "document-element-inserted", false);
> +  if (XRE_IsContentProcess()) {
> +    mObs->AddObserver(this, "http-on-opening-request", false);

Why are we only preloading in the content process?  Aren't there still cases where we load some content into the parent process, like when POSTing from a parent-process page?  Also, what happens with e10s off?

::: toolkit/components/extensions/extension-process-script.js:157
(Diff revision 1)
>      }
>    }
>  }
>  
> +let stubExtensions = new WeakMap();
> +let scriptMatchers = new DefaultWeakMap(matcher => new ScriptMatcher(stubExtensions.get(matcher.extension),

Wow this is dense, if you haven't yet internalized all 5 concepts used in this one line. ;)

::: toolkit/mozapps/extensions/AddonManager.jsm:104
(Diff revision 1)
>  });
>  
>  XPCOMUtils.defineLazyPreferenceGetter(this, "WEBEXT_PERMISSION_PROMPTS",
>                                        PREF_WEBEXT_PERM_PROMPTS, false);
>  
> -Services.ppmm.loadProcessScript("chrome://extensions/content/extension-process-script.js", true);
> +Services.ppmm.loadProcessScript("data:,Components.classes['@mozilla.org/webextensions/extension-process-script;1'].getService()", true);

Why is this loaded from this file anyway, and not one of the Extensions modules?
Attachment #8871851 - Flags: review?(tomica) → review+
Comment on attachment 8871851 [details]
Bug 1368102: Part 5 - Move static content script matching into C++.

https://reviewboard.mozilla.org/r/143352/#review147344

> Why are we only preloading in the content process?  Aren't there still cases where we load some content into the parent process, like when POSTing from a parent-process page?  Also, what happens with e10s off?

Long story, but essentially:

1) It's not actually possible to tell, at least from JS, whether the requests we're observing here belong to this process or are being proxied from a child process. It's probably possible from C++, but that's not a logic change I want to make in this bug.
2) Event loop idle time is more important in the parent process than it is in the child, and since we only load content scripts in the parent under unusual circumstances, we're better off avoiding the overhead as much as possible.

So scripts that get loaded in the parent process still get cached, and have more or less the same semantics as the ones loaded in the child, they don't get speculatively preloaded based on network activity.

> Wow this is dense, if you haven't yet internalized all 5 concepts used in this one line. ;)

Heh. You have a point. On the upside, this gets much simplified in a follow-up.

> Why is this loaded from this file anyway, and not one of the Extensions modules?

Because at the moment, we need to ensure that it's loaded early, into every process. That used to happen as a side-effect of us accidentally eagerly loading Extension.jsm from the add-on manager, but when I changed that, the lack of the eager process script load caused failures.
Priority: -- → P2
Whiteboard: [qf] → [qf][triaged]
This is critical for parent and content process performance.
Priority: P2 → P1
Comment on attachment 8871848 [details]
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.

https://reviewboard.mozilla.org/r/143346/#review148048

::: toolkit/components/extensions/WebExtensionPolicy.cpp:467
(Diff revision 1)
> +      nsIPrincipal* match(Window aWin)
> +      {
> +        nsCOMPtr<nsIDocument> doc = aWin->GetDoc();
> +        return doc->NodePrincipal();
> +      }
> +      nsIPrincipal* match(LoadInfo aLoadInfo) { return aLoadInfo->PrincipalToInherit(); }

Per loadinfo idl docs, "If the principalToInherit is null but the inherit flag is set, then the triggeringPrincipal is the principal that is inherited."  Do we need to handle that here, if not why not?
Comment on attachment 8871849 [details]
Bug 1368102: Part 3 - Use MatchPattern and MatchGlob bindings for content script matching.

https://reviewboard.mozilla.org/r/143348/#review148052

r+ given fixes for zombie's comments
Attachment #8871849 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8871850 [details]
Bug 1368102: Part 4 - Use WebExtensionContentScript to match content scripts.

https://reviewboard.mozilla.org/r/143350/#review148054
Attachment #8871850 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8871851 [details]
Bug 1368102: Part 5 - Move static content script matching into C++.

https://reviewboard.mozilla.org/r/143352/#review148066

::: toolkit/mozapps/extensions/AddonManager.jsm:104
(Diff revision 1)
>  });
>  
>  XPCOMUtils.defineLazyPreferenceGetter(this, "WEBEXT_PERMISSION_PROMPTS",
>                                        PREF_WEBEXT_PERM_PROMPTS, false);
>  
> -Services.ppmm.loadProcessScript("chrome://extensions/content/extension-process-script.js", true);
> +Services.ppmm.loadProcessScript("data:,Components.classes['@mozilla.org/webextensions/extension-process-script;1'].getService()", true);

Can you add a comment about why this is loaded here?
Attachment #8871851 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8871852 [details]
Bug 1368102: Part 6 - Remove StubExtension and use WebExtensionPolicy directly.

https://reviewboard.mozilla.org/r/143354/#review148082
Attachment #8871852 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8871853 [details]
Bug 1368102: Part 7 - Remove ScriptMatcher and use WebExtensionConentScript directly.

https://reviewboard.mozilla.org/r/143356/#review148084
Attachment #8871853 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8871854 [details]
Bug 1368102: Part 8 - Move extension page matching into C++.

https://reviewboard.mozilla.org/r/143358/#review148114
Attachment #8871854 - Flags: review?(mixedpuppy) → review+
Whiteboard: [qf][triaged] → [qf:p1][triaged]
Comment on attachment 8871848 [details]
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.

https://reviewboard.mozilla.org/r/143346/#review149434

::: toolkit/components/extensions/WebExtensionContentScript.h:12
(Diff revision 1)
> +#define mozilla_extensions_WebExtensionContentScript_h
> +
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/WebExtensionContentScriptBinding.h"
> +
> +#include "jsapi.h"

Same thing about jsapi.h.

::: toolkit/components/extensions/WebExtensionContentScript.h:16
(Diff revision 1)
> +#include "mozilla/extensions/MatchGlob.h"
> +#include "mozilla/extensions/MatchPattern.h"

Can these just be forward-declared?

::: toolkit/components/extensions/WebExtensionContentScript.h:30
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace extensions {
> +
> +using dom::Nullable;
> +using ContentScriptInit = dom::WebExtensionContentScriptInit;;

Two semicolons.
Attachment #8871848 - Flags: review?(wmccloskey) → review+
Comment on attachment 8871854 [details]
Bug 1368102: Part 8 - Move extension page matching into C++.

https://reviewboard.mozilla.org/r/143358/#review149436
Attachment #8871854 - Flags: review?(wmccloskey) → review+
Comment on attachment 8871848 [details]
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.

https://reviewboard.mozilla.org/r/143346/#review149434

> Can these just be forward-declared?

I don't think so. MatchGlob.h is necessary to declare `Nullable<MatchGlobSet>`. MatchPattern.h is necessary for anything that calls the methods with URLInfo& or DocInfo& arguments, which is pretty much everything that imports this.
Comment on attachment 8871848 [details]
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings.

https://reviewboard.mozilla.org/r/143346/#review149508
Attachment #8871848 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/863506a00d8362e8fe180001d0d4183c1b1f9d3c
Bug 1368102: Part 1 - Make AddonManagerWebAPI::IsValidSite public. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/056e3d934eb26523b35fcdd4b1cdadca2c8bb190
Bug 1368102: Part 2 - Add WebExtensionContentScript bindings. r=billm,mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9439245aa6bc591c4e138ea2f6978d7e3bab831
Bug 1368102: Part 3 - Use MatchPattern and MatchGlob bindings for content script matching. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/417fd0cf65a8bddc78c59156cb317cc4156f9392
Bug 1368102: Part 4 - Use WebExtensionContentScript to match content scripts. r=mixedpuppy,zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/32a3b7c392070cb1f9eb7b1ec1e177c6303d9d3f
Bug 1368102: Part 5 - Move static content script matching into C++. r=mixedpuppy,zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/350bf7ed1ad3ebe26e75360430cc0ee2d7477077
Bug 1368102: Part 6 - Remove StubExtension and use WebExtensionPolicy directly. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1eecd5c9c64f3d865ab8f3d529f61cd71f2a3b5
Bug 1368102: Part 7 - Remove ScriptMatcher and use WebExtensionConentScript directly. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5b869561f445909e2c5861d2f486c2432c65d5
Bug 1368102: Part 8 - Move extension page matching into C++. r=billm,mixedpuppy
Product: Toolkit → WebExtensions
Depends on: 1485282
No longer depends on: 1485282
Depends on: 1485282
No longer depends on: 1485282
Performance Impact: --- → P1
Whiteboard: [qf:p1][triaged] → [triaged]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: