1425611 - [meta] Move/expose Cu.import to/on ChromeUtils or similar webidl machinery and rewrite chrome JS to use that instead

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[:Gijs (he/him)]

Every time we use Cu.import, we incur a tiny additional cost on top of whatever the actual implementation does, because xpconnect is really general, the implementation lives in C++, and callsites are in JS.

This wouldn't matter so much if we didn't use Cu.import a lot. But we do. Like, really a lot, and this will likely just increase given we now have shared compartments for jsms so we feel less constrained by cost there.

Even if the overhead of an individual xpconnect call on a pgo build on every user's machine was limited to 1µs (and I see slightly higher numbers on my opt (but non-pgo) build on a fast mbp, so that seems generous for most actual users on weaker hardware), we call this code thousands of time over normal browser usage. Bobby said overhead from webidl should be on the order of nanoseconds because it's all codegen. So it seems like it might be a good idea to either move this to webidl or add a webidl equivalent assuming that is a relatively easy thing to do.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a ChromeUtils.import that does what Cu.import does

Want to give this a shot?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a ChromeUtils.import that does what Cu.import does

Now with handling of null target obj arg fixed...

Comment 3

[:Gijs (he/him)]

I'm noticing 2 issues when mass-replacing Cu.import calls on top of this patch.

1) JS components don't have access to ChromeUtils, so this breaks them. Kind of annoying for my global search/replace because we don't have a good way to filter those...

Is there a reasonable way to get a ref to ChromeUtils from a JS component, or are those just out of luck?

2) something weird is happening in some of the webextension scripts. Specifically, I see:

ReferenceError: ExtensionParent is not defined[Learn More] ext-backgroundPage.js:10:1

and similar errors, coming from code that now does this:


ChromeUtils.import("resource://gre/modules/ExtensionParent.jsm");
var {
  HiddenExtensionPage,
  promiseExtensionViewLoaded,
} = ExtensionParent;

in ext-backgroundPage.js ( https://dxr.mozilla.org/mozilla-central/rev/1624b88874765bf57e9feba176d30149c748d9d2/toolkit/components/extensions/ext-backgroundPage.js#6 )

It seems like perhaps the global situation for that script is weird, or something? I'm not really sure what the issue is and if it's easy to fix.

Comment 4

[:Gijs (he/him)]

(I checked that ExtensionParent is in EXPORTED_SYMBOLS)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is there a reasonable way to get a ref to ChromeUtils from a JS component, or are those just out of luck?

Hmm.  I would expect ChromeUtils to be exposed in JS components, just like everything else that uses a BackstagePass as the global.  Can you point to a specific file where you're seeing this not work, and I'll take a look?

> ReferenceError: ExtensionParent is not defined[Learn More] ext-backgroundPage.js:10:1

I'll take a look.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> ReferenceError: ExtensionParent is not defined[Learn More] ext-backgroundPage.js:10:1

OK, this one I have sort of figured out.

We're running code from chrome://extensions/content/ext-backgroundPage.js in a sandbox, as far as I can tell.  I haven't checked where that sandbox comes from, but apparently it defines a Cu in that sandbox global.  So when Cu.import examines the caller compartment it sees the sandbox global and defines the "ExtensionParent" property on that global.

On the other hand, the ChromeUtils.import call is calling through a cross-compartment proxy.  By the time we land in import, our current compartment is a BackstagePass global, not the sandbox.  And that BackstagePass is where ChromeUtils.import ends up defining the property.  Then we unwind and the property doesn't exist on the sandbox global.  

OK, so why the difference?  The sandbox is coming from this code in ExtensionCommon.jsm, in _createExtGlobal:

    let global = Cu.Sandbox(Services.scriptSecurityManager.getSystemPrincipal(), {
      wantXrays: false,
      sandboxName: `Namespace of ext-*.js scripts for ${this.processType} (from: resource://gre/modules/ExtensionCommon.jsm)`,
    });

    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, ChromeUtils, ChromeWorker, ExtensionAPI, ExtensionCommon, MatchPattern, MatchPatternSet, MatchGlob, StructuredCloneHolder, extensions: this});

When we do that Object.assign, we wrap all those things into the sandbox global's compartment.  The thing is, when we wrap "Cu", we notice it's an XPCWrappedNative for an XPCOM C++ object, and just create a new XPCWrappedNative for it in the new compartment (!).  But when we wrap "ChromeUtils", it's just a plain object as far as the wrapping machinery is concerned, so we give it a cross-compartment wrapper.

We could probably fix this specific case by adding a way to request that a ChromeUtils specific to the sandbox be defined on the sandbox (via the wantGlobalProperties machinery) and using it for this sandbox instead of grabbing the one from ExtensionCommon.jsm.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Give the sandboxes used for webextension loadScript their own copies of ChromeUtils instead of having them all share the ExtensionCommon.jsm one

With this patch, toolkit/components/extensions/test/mochitest/test_chrome_ext_background_page.html passes, which is a good sign.  We probably want something similar done to devtools/shared/builtin-modules.js but would need to hunt down where the relevant sandboxes are created.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> We're running code from chrome://extensions/content/ext-backgroundPage.js in
> a sandbox, as far as I can tell.  I haven't checked where that sandbox comes
> from, but apparently it defines a Cu in that sandbox global.  So when
> Cu.import examines the caller compartment it sees the sandbox global and
> defines the "ExtensionParent" property on that global.

I really hate these sandboxes... I've been hoping to get rid of them, if I ever get a review for bug 1393270.

Comment 9

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> > Is there a reasonable way to get a ref to ChromeUtils from a JS component, or are those just out of luck?
> 
> Hmm.  I would expect ChromeUtils to be exposed in JS components, just like
> everything else that uses a BackstagePass as the global.  Can you point to a
> specific file where you're seeing this not work, and I'll take a look?

This was XPIProviderUtils.js . Looks like it's being manually loaded onto a sandbox via the subscript loader etc., much like the stuff you're fixing in the other patch you put up.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ok, good.  Then just throwing in wantGlobalProperties bits on top of my other patch will fix it.

Comment 11

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #9)
> This was XPIProviderUtils.js . Looks like it's being manually loaded onto a
> sandbox via the subscript loader etc., much like the stuff you're fixing in
> the other patch you put up.

That's another sandbox that I hate and want to get rid of. r=me if you want to just replace it with a plain object.

Comment 12

[:Gijs (he/him)]

Sigh. So I couldn't get devtools to work quickly, so I excluded that from any changes. Then I pushed to try with talos to try to get numbers on how much this buys us (not sure that'd really show much, but I figured it was worth a shot), but tpaint and friends don't run to completion anymore, so one of my ~2900 replaces must be breaking them. There's no useful error messages, they just time out. I'll look at this more after the break.

Comment 13

[:Gijs (he/him)]

OK. On my local Windows machine, against a 366-ish ms baseline when running ts_paint at 50 cycles, these changes + replacing Cu.import in 950 files (excluding tests, excluding devtools/, and excluding testing/talos because it has other stuff that it subscript loads in strange ways on non-privileged things that have had extra permissions put on them via UniversalXPConnect), this nets us about 1-2ms on the median, 1.8ms on the average. That's basically 0.5%. Talos on infra is too noisy to draw any real conclusions, as far as I can tell, but https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ff9f99050bc6f23f5dd2595ef12b878365e40d86&newProject=try&newRevision=c4457b8f3e6e00c403ff9d33f2ad447d2bfec77a&framework=1 is there if people want to look at it.

What I will add is that for the second run (which, though it's reported in the set of results unlike the first run, I think gets dropped from the averages/medians for being too skewed), I'm seeing larger impacts, more around the 1% mark both on talos and locally.

0.5-1% on startup is pretty slim, and is likely to be lower on machines where startup times are more IO-bound, and switching our main "module" system in chrome JS land to a different API is non-trivial (though the rewrite scripts help!), but I think on balance this is just about still worth doing, also because the patches are essentially 80% of the way there, AFAICT...

However, I'd like a second opinion. Florian/Mike? :-)

Comment 14

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #13)

> However, I'd like a second opinion. Florian/Mike? :-)

If we believe it's faster, and talos shows no regression and even a slight win, then the question becomes: do we think this makes the code nicer to read?

I'm leaning toward a yes, mostly because replacing it everywhere makes the code base more consistent, but I guess I could also be convinced that replacing "Components.utils" with "Cu" everywhere is another reasonable way to make this more consistent.

We could also use scripts to remove many now-unused 'Cu' variables, and that may add another tiny win.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

This seems like small-ish potatoes, but a win is a win. If this is almost done, and assuming the last 20% isn't 90% of the work, then this seems like it's worth finishing off.

Note that we'll need to update documentation, and do something about deprecating Cu.import. to avoid re-introducing it.

Comment 16

[Kris Maglione [:kmag]]

It may not be a huge improvement now, but moving the module import API to WebIDL may allow us to make much bigger improvements.

(In reply to Florian Quèze [:florian] from comment #14)
> I'm leaning toward a yes, mostly because replacing it everywhere makes the
> code base more consistent, but I guess I could also be convinced that
> replacing "Components.utils" with "Cu" everywhere is another reasonable way
> to make this more consistent.

That's not as likely to make a huge difference as you might expect. There's a layer of caching that makes subsequent access to Components properties relatively cheap, so switching to Cu everywhere isn't likely to be a huge win. And there's a chance we'll just wind up with extra, unused Cu definitions that just wind up being more expensive in the long run.

Comment 17

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #16)
> It may not be a huge improvement now, but moving the module import API to
> WebIDL may allow us to make much bigger improvements.

Can you elaborate a bit on this and what you have in mind? I'm still kind of on the fence about making this change. 


(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #15)
> This seems like small-ish potatoes, but a win is a win. If this is almost
> done, and assuming the last 20% isn't 90% of the work, then this seems like
> it's worth finishing off.

As far as I can tell, we'd need to:

1) get review on the ChromeUtils addition (ie patch 1 here)
2) fix the eslint plugin so it detects the ChromeUtils thing as adding globals to the relevant scope
3) create a custom eslint rule to produce errors for Cu.import
4) re-educate everyone and MDN on "how to import jsms in chrome scope"

5) either:
a) fix all the consumers that break with naive replacements, including tests, devtools and talos (devtools and talos being the ones I know cause some issues, but I expect there might be other edgecases that I'd run into when running stuff on try)

OR:
b) i)  auto-replace all the autoreplace-able instances
   ii) file followup bugs for the remainder
   iii) selectively make exceptions to the eslint rule


That's a bunch of work (I'm a bit worried about (4), especially that we're not moving to something more standards-based like ES modules (which I *get*, obviously, is an even/much bigger switch that we can't make quickly)) for 0.5-1% ts_paint gains.

To be clear, I'm not saying we *shouldn't* do it, but I'd be happier sinking in some more time to try to get there if I had a better idea of the gains it would enable.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8937227 [details] [diff] [review]
part 1.  Add a ChromeUtils.import that does what Cu.import does

Review of attachment 8937227 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ChromeUtils.cpp
@@ +376,5 @@
>  /* static */ void
> +ChromeUtils::Import(const GlobalObject& aGlobal,
> +                    const nsAString& aResourceURI,
> +                    const Optional<JS::Handle<JSObject*>>& aTargetObj,
> +                    JS::MutableHandle<JSObject*> aRetval,

Nit: JS::HandleObject and JS::MutableHandleObject, please.

@@ +384,5 @@
> +  RefPtr<mozJSComponentLoader> moduleloader = mozJSComponentLoader::Get();
> +  MOZ_ASSERT(moduleloader);
> +
> +  NS_ConvertUTF16toUTF8 registryLocation(aResourceURI);
> +  

Nit: Useless whitespace.

@@ +389,5 @@
> +  AUTO_PROFILER_LABEL_DYNAMIC_NSCSTRING(
> +    "ChromeUtils::Import", OTHER, registryLocation);
> +
> +  JSContext* cx = aGlobal.Context();
> +  JS::Rooted<JS::Value> targetObj(cx);

Nit: JS::RootedValue please.

@@ +408,5 @@
> +    return;
> +  }
> +
> +  // Import() on the component loader can return NS_OK while leaving an
> +  // exception on the JSContext.  Check for that case.

I wonder if we should fix that...

::: dom/webidl/ChromeUtils.webidl
@@ +201,5 @@
>    void idleDispatch(IdleRequestCallback callback,
>                      optional IdleRequestOptions options);
> +
> +  /**
> +   *

Nit: Remove blank line.

@@ +216,5 @@
> +   * on the caller's global object. If 'EXPORTED_SYMBOLS' is not
> +   * found, an error is thrown.
> +   *
> +   * @param aResourceURI A resource:// URI string to load the module from.
> +   * @param aTargetObj  the object to install the exported properties on.

Nit: Or null.

Comment 19

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #17)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #16)
> > It may not be a huge improvement now, but moving the module import API to
> > WebIDL may allow us to make much bigger improvements.
> 
> Can you elaborate a bit on this and what you have in mind? I'm still kind of
> on the fence about making this change.

Probably the biggest thing would be moving the lazy import machinery to native
code. Just defining and re-defining the lazy property descriptors is pretty
expensive in JS. But, beyond that, every time we delazify a property, we:

- Create a temporary object.
- Call Cu.import, which invokes all of the normal import machinery.
- Look up the EXPORTED_SYMBOLS property, first on the module lexical scope, then
  on the module global.
- For each property listed, enter the module compartment, look up that symbol
  first on the module lexical scope, then on the module global. Enter the target
  object scope, wrap the value, assign it to a property on that object.
- Lookup the symbol on the temporary object.
- Redefine the property descriptor.

If ChromeUtils were responsible for the module import machinery, we could skip
most of the above, and just lookup the property that we currently care about.

We could probably do that without Cu.import to ChromeUtils, but I'd rather have
all of these things in one place.

> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and
> needinfos from comment #15)
> As far as I can tell, we'd need to:
>
> 1) get review on the ChromeUtils addition (ie patch 1 here)

r=me

> That's a bunch of work (I'm a bit worried about (4), especially that we're
> not moving to something more standards-based like ES modules (which I *get*,
> obviously, is an even/much bigger switch that we can't make quickly)) for
> 0.5-1% ts_paint gains.

It's not really that much work. The ESLint changes are pretty trivial, and the
docs on MDN are mostly archived at this point.

Comment 20

[Nick Alexander :nalexander [he/him]]

May I ask a naieve question: why do we allow a second argument to at all?

If the goal is to migrate to the `import` of ES modules, the second argument prevents a context-free syntactic reorganization.  Is it that we'd need to do this migration at some point, and there's no advantage to doing it now?  Is there some behaviour that can't be expressed without the second argument?

Perhaps this is too much of a change?  I see that we prevent `Cu.import("foo")` in https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-single-arg-cu-import.js and even `let foo = Cu.import("foo", bar)` in https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-import-into-var-and-global.js.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Nit: JS::HandleObject and JS::MutableHandleObject, please.
> Nit: JS::RootedValue please.

Hmm.  This is DOM vs XPConnect style differences.  Looks like this file is now a mix of styles already (started out DOM-style).  I was assuming it was still DOM-style...

> Nit: Useless whitespace.

Fixed.

> I wonder if we should fix that...

Depends on whether we plan to keep the xpidl bits at all.

> Nit: Remove blank line.

Fixed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to the non-style review comments

Comment 23

[Mark Banner (:standard8)]

(In reply to Nick Alexander :nalexander from comment #20)
> May I ask a naieve question: why do we allow a second argument to at all?
> 
> If the goal is to migrate to the `import` of ES modules, the second argument
> prevents a context-free syntactic reorganization.  Is it that we'd need to
> do this migration at some point, and there's no advantage to doing it now? 
> Is there some behaviour that can't be expressed without the second argument?

The second argument prevents importing all exported items into the global scope. For example for these statements:

1) Cu.import("resource://gre/modules/AddonManager.jsm");
2) var { AddonManager } = Cu.import("resource://gre/modules/AddonManager.jsm");
3) var { AddonManager } = Cu.import("resource://gre/modules/AddonManager.jsm", {});

For 1) AddonManager, AddonManagerPrivate are defined in the global scope.
For 2) AddonManager, AddonManagerPrivate are defined in the global scope, and AddonManager would be additionally defined as a variable in whatever scope the statement was in.
For 3) only the AddonManager variable gets defined, nothing extra in the global scope.

Having everything imported into the global scope makes it difficult for ESLint currently - we have to jump through a few hoops to work out what globals are actually defined (e.g. https://searchfox.org/mozilla-central/source/tools/lint/eslint/modules.json).

Additionally, the single argument form makes it harder to work out if we're importing files into different scopes unnecessarily.

Personally, I'd much rather we moved towards a form which only allowed importing named items from modules as it would make it clearer what was expected to be used where.

From a quick scan, I think most of the two-argument forms in the tree equate to either an empty object or the global scope itself being passed in, which could potentially be replaced fairly easily. 

> Perhaps this is too much of a change?  I see that we prevent
> `Cu.import("foo")` in
> https://searchfox.org/mozilla-central/rev/
> 41925c0b6c6d58578690121de439d2a8d3d690f3/tools/lint/eslint/eslint-plugin-
> mozilla/lib/rules/no-single-arg-cu-import.js and even `let foo =
> Cu.import("foo", bar)` in
> https://searchfox.org/mozilla-central/rev/
> 41925c0b6c6d58578690121de439d2a8d3d690f3/tools/lint/eslint/eslint-plugin-
> mozilla/lib/rules/no-import-into-var-and-global.js.

no-single-arg-cu-import is only used by devtools currently. Mossop and I discussed moving to it for all of mozilla-central around the time we started doing major work on ESLint, and decided it would be too much work at that time (and possibly risky).

no-import-into-var-and-global.js was designed to avoid case 2 above, which wasn't very clear to developers.

I'll leave the NI to Gijs in case he has additional thoughts.

Comment 24

[Nick Alexander :nalexander [he/him]]

Thanks Mark, very helpful.

(In reply to Mark Banner (:standard8) from comment #23)
> (In reply to Nick Alexander :nalexander from comment #20)
> > May I ask a naieve question: why do we allow a second argument to at all?
> > 
> > If the goal is to migrate to the `import` of ES modules, the second argument
> > prevents a context-free syntactic reorganization.  Is it that we'd need to
> > do this migration at some point, and there's no advantage to doing it now? 
> > Is there some behaviour that can't be expressed without the second argument?
> 
> The second argument prevents importing all exported items into the global
> scope. For example for these statements:
> 
> 1) Cu.import("resource://gre/modules/AddonManager.jsm");
> 2) var { AddonManager } =
> Cu.import("resource://gre/modules/AddonManager.jsm");
> 3) var { AddonManager } =
> Cu.import("resource://gre/modules/AddonManager.jsm", {});
> 
> For 1) AddonManager, AddonManagerPrivate are defined in the global scope.
> For 2) AddonManager, AddonManagerPrivate are defined in the global scope,
> and AddonManager would be additionally defined as a variable in whatever
> scope the statement was in.
> For 3) only the AddonManager variable gets defined, nothing extra in the
> global scope.
> 
> Having everything imported into the global scope makes it difficult for
> ESLint currently - we have to jump through a few hoops to work out what
> globals are actually defined (e.g.
> https://searchfox.org/mozilla-central/source/tools/lint/eslint/modules.json).
> 
> Additionally, the single argument form makes it harder to work out if we're
> importing files into different scopes unnecessarily.
> 
> Personally, I'd much rather we moved towards a form which only allowed
> importing named items from modules as it would make it clearer what was
> expected to be used where.

Ah, what I really think we should move toward is one argument with implicit {} for the second argument.  That is, don't _ever_ pollute global scope, and make consumers be explicit about what they import into their local scope.  That's what syntax-`import` does, right?

> From a quick scan, I think most of the two-argument forms in the tree equate
> to either an empty object or the global scope itself being passed in, which
> could potentially be replaced fairly easily. 
> 
> > Perhaps this is too much of a change?  I see that we prevent
> > `Cu.import("foo")` in
> > https://searchfox.org/mozilla-central/rev/
> > 41925c0b6c6d58578690121de439d2a8d3d690f3/tools/lint/eslint/eslint-plugin-
> > mozilla/lib/rules/no-single-arg-cu-import.js and even `let foo =
> > Cu.import("foo", bar)` in
> > https://searchfox.org/mozilla-central/rev/
> > 41925c0b6c6d58578690121de439d2a8d3d690f3/tools/lint/eslint/eslint-plugin-
> > mozilla/lib/rules/no-import-into-var-and-global.js.
> 
> no-single-arg-cu-import is only used by devtools currently. Mossop and I
> discussed moving to it for all of mozilla-central around the time we started
> doing major work on ESLint, and decided it would be too much work at that
> time (and possibly risky).

Thanks for this context.
 
> no-import-into-var-and-global.js was designed to avoid case 2 above, which
> wasn't very clear to developers.

I agree!

Comment 25

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #21)
> > Nit: JS::HandleObject and JS::MutableHandleObject, please.
> > Nit: JS::RootedValue please.
> 
> Hmm.  This is DOM vs XPConnect style differences.  Looks like this file is
> now a mix of styles already (started out DOM-style).  I was assuming it was
> still DOM-style...

Fair enough. Most of this code came from devtools and xpconnect, which use JS style, so it wound up with JS naming conventions. But it is technically a DOM module.

> > I wonder if we should fix that...
> 
> Depends on whether we plan to keep the xpidl bits at all.

I think we should probably keep them until this is available on all supported branches, to make migration easier for system add-ons. After that, I think we can remove them.

Comment 26

[Kris Maglione [:kmag]]

(In reply to Nick Alexander :nalexander from comment #24)
> Ah, what I really think we should move toward is one argument with implicit
> {} for the second argument.  That is, don't _ever_ pollute global scope, and
> make consumers be explicit about what they import into their local scope. 
> That's what syntax-`import` does, right?

That's easier said than done. Assuming that everywhere we currently import into the global scope, we can actually get a reference to the global scope object (which I'm not 100% sure of), we could probably make this work.

The problem with that is that the default behavior in that case would be to just return the module global. And using the module global is something we generally want to discourage, but still unfortunately need to support.

It would be nice if we could just return a temporary object containing the exported symbols when no second argument is passed, but that would break existing use cases that depend on getting the module global.

The alternative would probably be to either only return the module global when `null` is passed, or to add a separate import method that returns the module global. That's probably doable, but is a lot more work. And since I'd rather we get this done sooner than later, I'd rather we stick with the mostly compatible interface for now, and aim for a larger refactoring in a follow-up.

Comment 27

[Boris Zbarsky [:bzbarsky]]

bzexport fail...

Comment 28

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #23)
> I'll leave the NI to Gijs in case he has additional thoughts.

I don't really have any. I think ES.next modules are a good idea, but it's going to be a long slog, and I'd like to keep this focused on the incremental improvement of switching to something (even only slightly) better than Cu.import. As comment 19 suggests, there's further improvements that this will enable. If one of them is eventually ES.next modules, so much the better, but not right here/now, because of lazy loading, process-singleton semantics, and other concerns. AFAICT runtime/dynamic import of modules is still tbd for ES.next as well, so perhaps in the near future that story will improve for us through standards rather than through making up our own story for it. :-)

Comment 29

[:Gijs (he/him)]

Attachment: Use ChromeUtils in extensions sandboxes

This is the non-JS/xpconnect bit of the earlier patches that'll need to land when ChromeUtils support and the eslint fix in bug 1431069 has landed.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d463ab1b36b
part 2.  Allow sandboxes to import the ChromeUtils API.  r=kmag

Comment 31

[Boris Zbarsky [:bzbarsky]]

Er, I pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e03a6d354571b39cba0ba911df5634cfa14019 too but messed up the bug number there...

Comment 32

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7d463ab1b36b

Comment 33

[Kris Maglione [:kmag]]

I wound up doing the ESLint part of this in bug 1431533, as part of the rule I added for that API. That takes care of migrating most of the callers in code that ESLint runs on, but leaves devtools and the parts of the tree in the ESLint ignore list mostly untouched.

Comment 34

[:Gijs (he/him)]

bug 1431533 does most of the remaining work here, so going to just mark it as a dep. :-)

Comment 35

[:Gijs (he/him)]

The lion's share of the work here is done, so going to close this as wfm. Thanks everyone, especially Kris and bz, for all the work!