Remove ExtensionManagement.jsm

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 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
Attachment #8871884 - Flags: review?(aswan) → review+

Comment 6

2 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?
(Assignee)

Comment 7

2 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.

Comment 8

2 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
Attachment #8871885 - Flags: review?(mixedpuppy) → review+

Comment 9

2 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

Comment 10

2 years ago
mozreview-review
Comment on attachment 8871886 [details]
Bug 1368152: Part 3 - Remove ExtensionManagement.getURLForExtension.

https://reviewboard.mozilla.org/r/143410/#review148452
Attachment #8871886 - Flags: review?(aswan) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8871887 [details]
Bug 1368152: Part 4 - Remove ExtensionManagement.jsm.

https://reviewboard.mozilla.org/r/143412/#review148454
Attachment #8871887 - Flags: review?(aswan) → review+
(Assignee)

Comment 12

2 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.
(Assignee)

Comment 13

2 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
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

Comment 16

2 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
(Assignee)

Comment 17

2 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.
(Assignee)

Comment 18

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

Comment 19

2 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?
(Assignee)

Comment 20

2 years ago
See bug 1271354.
Attachment #8871885 - Flags: review?(aswan)

Updated

2 years ago
Blocks: 1370463

Updated

8 months ago
Product: Toolkit → WebExtensions

Updated

6 months ago
Depends on: 1485282

Updated

6 months ago
No longer depends on: 1485282
You need to log in before you can comment on or make changes to this bug.