Move content script and extension page matching into C++

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P1
normal
RESOLVED FIXED
a month ago
20 days ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1][triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

59 bytes, text/x-review-board-request
aswan
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
billm
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
zombie
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
zombie
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details | Review
59 bytes, text/x-review-board-request
mixedpuppy
: review+
billm
: review+
Details | Review
(Assignee)

Description

a month ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Blocks: 1368152

Comment 9

28 days ago
mozreview-review
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 10

28 days ago
mozreview-review
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)
(Assignee)

Comment 11

28 days ago
mozreview-review-reply
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.
(Assignee)

Comment 12

28 days ago
mozreview-review-reply
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.
(Assignee)

Updated

28 days ago
Blocks: 1368289

Comment 13

28 days ago
mozreview-review
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+
(Assignee)

Comment 14

28 days ago
mozreview-review-reply
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 15

28 days ago
mozreview-review
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 16

28 days ago
mozreview-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+
(Assignee)

Comment 17

28 days ago
mozreview-review-reply
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.

Updated

27 days ago
Priority: -- → P2
Whiteboard: [qf] → [qf][triaged]
(Assignee)

Comment 18

27 days ago
This is critical for parent and content process performance.
Priority: P2 → P1
Blocks: 1367478

Comment 19

26 days ago
mozreview-review
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 20

26 days ago
mozreview-review
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 21

26 days ago
mozreview-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 22

26 days ago
mozreview-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 23

26 days ago
mozreview-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 24

26 days ago
mozreview-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 25

25 days ago
mozreview-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+

Updated

24 days ago
Whiteboard: [qf][triaged] → [qf:p1][triaged]

Comment 26

22 days ago
mozreview-review
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 27

22 days ago
mozreview-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+
(Assignee)

Comment 28

22 days ago
mozreview-review-reply
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 29

21 days ago
mozreview-review
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+
(Assignee)

Comment 30

20 days ago
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
https://hg.mozilla.org/mozilla-central/rev/863506a00d83
https://hg.mozilla.org/mozilla-central/rev/056e3d934eb2
https://hg.mozilla.org/mozilla-central/rev/b9439245aa6b
https://hg.mozilla.org/mozilla-central/rev/417fd0cf65a8
https://hg.mozilla.org/mozilla-central/rev/32a3b7c39207
https://hg.mozilla.org/mozilla-central/rev/350bf7ed1ad3
https://hg.mozilla.org/mozilla-central/rev/a1eecd5c9c64
https://hg.mozilla.org/mozilla-central/rev/ef5b869561f4
Status: NEW → RESOLVED
Last Resolved: 20 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.