Kris Maglione [:kmag] Assignee
	Description • 7 years ago

This is one more module for us to load at startup, which causes an unnecessary perf hit, and unnecessary complexity in the wake of recent refactorings.

	Andrew Swan [:aswan]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8871884 [details]
Bug 1368152: Part 1 - Move API extension registration to ExtensionAPI.jsm.

https://reviewboard.mozilla.org/r/143406/#review147348

	Andrew Swan [:aswan]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8871885 [details]
Bug 1368152: Part 2 - Move extension policy registration out of ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143408/#review147352

::: toolkit/components/extensions/Extension.jsm:80
(Diff revision 1)
> -Cu.import("resource://gre/modules/ExtensionManagement.jsm");
> +XPCOMUtils.defineLazyGetter(
> +  this, "processScript",
> +  () => Cc["@mozilla.org/webextensions/extension-process-script;1"]
> +          .getService().wrappedJSObject);

What's this?  I'm guessing its in some other pending but not-yet-landed patch?

	Kris Maglione [:kmag] Assignee
	Comment 7 • 7 years ago
mozreview-review-reply

Comment on attachment 8871885 [details]
Bug 1368152: Part 2 - Move extension policy registration out of ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143408/#review147352

> What's this?  I'm guessing its in some other pending but not-yet-landed patch?

Yeah, bug 1322235.

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8871885 [details]
Bug 1368152: Part 2 - Move extension policy registration out of ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143408/#review148116

	Andrew Swan [:aswan]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8871885 [details]
Bug 1368152: Part 2 - Move extension policy registration out of ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143408/#review148450

::: toolkit/components/extensions/extension-process-script.js:297
(Diff revision 1)
>  
>        Services.cpmm.addMessageListener("Schema:Add", this);
>      }
>    },
>  
> -  initExtension(data) {
> +  initExtensionPolicy(data, extension) {

this appears to only ever get called with one argument

::: toolkit/components/extensions/extension-process-script.js:384
(Diff revision 1)
>  
>  ExtensionProcessScript.prototype = {
>    classID: Components.ID("{21f9819e-4cdf-49f9-85a0-850af91a5058}"),
>    QueryInterface: XPCOMUtils.generateQI([Ci.mozIExtensionProcessScript]),
>  
> +  get wrappedJSObject() { return this; },

I don't understand the purpose of this line

	Andrew Swan [:aswan]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8871886 [details]
Bug 1368152: Part 3 - Remove ExtensionManagement.getURLForExtension.

https://reviewboard.mozilla.org/r/143410/#review148452

	Andrew Swan [:aswan]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8871887 [details]
Bug 1368152: Part 4 - Remove ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143412/#review148454

	Kris Maglione [:kmag] Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8871885 [details]
Bug 1368152: Part 2 - Move extension policy registration out of ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143408/#review148450

> this appears to only ever get called with one argument

Hrm. Yeah, it was supposed to be called with two arguments from ExtensionProcessScript.initExtension. This still works, but is a bit less efficient for the parent process.

> I don't understand the purpose of this line

It's basically a bit of XPConnect magic that lets JS code access the raw, underlying JS object that implements the service rather than going through XPConnect/XPIDL, since it's much more efficient and doesn't require declaring bindings for every JS-only method or property.

	Kris Maglione [:kmag] Assignee
	Comment 13 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3fdbc5921e3815356cc81af1fc7284a2e67bb76
Bug 1368152: Part 1 - Move API extension registration to ExtensionAPI.jsm. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/73737d0a388808aabadd578053374f01a46cd315
Bug 1368152: Part 2 - Move extension policy registration out of ExtensionManagement.jsm. r=aswan,mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/1733d96aacacb985f240dd7aae724c9de011b393
Bug 1368152: Part 3 - Remove ExtensionManagement.getURLForExtension. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3359583493bf0506c4fe1c81573b8a123413ec
Bug 1368152: Part 4 - Remove ExtensionManagement.jsm. r=aswan

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e3fdbc5921e3
https://hg.mozilla.org/mozilla-central/rev/73737d0a3888
https://hg.mozilla.org/mozilla-central/rev/1733d96aacac
https://hg.mozilla.org/mozilla-central/rev/1b3359583493

	Gary [:streetwolf52]
	Comment 15 • 7 years ago

I think that this patch might have broken a feature on the add-on uBlock Origin. At the top of the uBO menu if you click on it you are supposed to open the uBO dashboard.  Nothing happens.  This patch is in the regression range.

When I do click on it I see this in my Browser Console:

[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/ext-tabs.js :: query :: line 479"  data: no]  (unknown)
	query chrome://browser/content/ext-tabs.js:479:29
	next self-hosted:1157:9
tabs is undefined  vapi-background.js:481
	vAPI.tabs.open/< moz-extension://1b84d5a3-273d-4352-b6dc-0f845027c8b2/js/vapi-background.js:481:13
	runSafeSyncWithoutClone resource://gre/modules/ExtensionUtils.jsm:52:14
	runSafeWithoutClone resource://gre/modules/ExtensionCommon.jsm:176:38
	wrapPromise/</< resource://gre/modules/ExtensionCommon.jsm:365:15
	withLastError resource://gre/modules/ExtensionCommon.jsm:303:14
	wrapPromise/< resource://gre/modules/ExtensionCommon.jsm:359:11
Unchecked lastError value: Error: An unexpected error occurred  ExtensionCommon.jsm:306
	withLastError resource://gre/modules/ExtensionCommon.jsm:306:9
	wrapPromise/< resource://gre/modules/ExtensionCommon.jsm:359:11

	u287251
	Comment 16 • 7 years ago

The issue is with the webext version of uBlock Origin.

Looking at the webext documentation for browser.tabs.query[1], it's as if the method does not accept the second callback parameter, which is what uBO uses to handle the response. From the documentation, a Promise is returned.

If so, that makes the method incompatible with chrome.tabs.query, which accepts a callback and does not return a Promise. Currently, the webext version of uBO is pretty much the same code base as the Chromium version of uBO.

It is this incompatibility by design, or is it something which will be fixed?

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query

	Kris Maglione [:kmag] Assignee
	Comment 17 • 7 years ago

No, the problem is that tabs.query now only accepts valid match patterns for the `url` parameter, and uBlock is apparently passing an invalid one.

	Kris Maglione [:kmag] Assignee
	Comment 18 • 7 years ago

Ah, apparently they're trying to pass their moz-extension: URL as a match pattern, which we don't currently support.

	u287251
	Comment 19 • 7 years ago

Sorry, dismiss my comment above about callback/Primise, I see the code does handle the callback case.

> uBlock is apparently passing an invalid one.

It's using a URL from what browser.runtime.getURL() returns.

Using the debugger to debug uBO, if I type browser.runtime.getURL('/'), I get "moz-extension://7d41851d-0c47-40b9-82e5-40af4eae19ce/".

When opening the dashboard, uBO passes "moz-extension://7d41851d-0c47-40b9-82e5-40af4eae19ce/dashboard.html", as per the result returned by browser.runtime.getURL().

Why is this not valid?

	Kris Maglione [:kmag] Assignee
	Comment 20 • 7 years ago

See bug 1271354.