920553 - Implement a more disciplined mechanism for exposing DOM constructors on non-DOM globals

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

In bug 920179, bent flagged me for review on a patch to fix a problem resulting from eagerly defining IndexedDB constructors on all globals that pass through InitClassesWithNewWrappedGlobal. I wasn't aware that we were doing this and was kind of unhappy to find out we did, but apparently bz has been suggesting this to people (bug 890364), so I'll swallow any indignation and move on to proposing solutions. ;-)

My beefs with the current setup are as follows:
* Unexpected ongoing mutation of the global namespace for various system scopes. Doing this to the BSP impacts addons (both JS-implemented XPCOM components and JSMs), which may break in surprising ways when constructors suddenly appear. Moreover, the current code impacts the Frame Message Manager (which I'm not even sure is intentional).
* The introduction of system scope dependencies on various DOM constructors without any easy way to grep for them. Note that this, along with the above, are par for the course for the web. But our internal platform APIs are a different beast (less deliberate consideration of what gets exposed, more likelihood to depend on quirks, a longer tail of later-breaking compat issues, etc).
* The haphazard eager definition will get unwieldy (and potentially slow) as more constructors are added, and doing this with resolve hooks only serves to further scatter the relevant code.
* Duplication of work for Sandboxes.

Speaking of Sandboxes, gabor has been diligently solving this problem on that front, and I'd like to expand his solution to other system globals. The basic approach is to pass an array of properties to the sandbox constructor, like so: { wantGlobalProperties: ['XMLHttpRequest', 'TextEncoder', 'atob']}.

I think this solution can be pretty easily extended to operate à la module loading, whereby JSMs would do something like: Cu.importGlobalProperties([...]). We can then reuse most of the current Sandbox machinery, and make sure such work benefits both systems. This solves the beefs I listed above.

A couple of question:
* What should the import function be called? 'import' or 'define'? 'constructor' or 'globalProperty'? In the Sandbox world, we previously called them 'DOMConstructors', but then had to backpedal when adding atob and such. I'm less worried about atob and whatnot for BSP scopes, but we still might want to keep our options open.
* Do we want to mimic the Cu.import-like ability to pass an object upon which the stuff gets defined, or do we want it just to go onto the global?

Other thoughts?

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't see the issue with exposing DOM APIs to all scopes where they make sense.

Comment 2

[Masatoshi Kimura [:emk]]

It may break existing JavaScript code modules to appear too generic names (such as CSS or URL) suddenly. See bug 920015.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> It may break existing JavaScript code modules to appear too generic names
> (such as CSS or URL) suddenly. See bug 920015.

Precisely. This is my first point in comment 0.

Comment 4

[(no longer active)]

(In reply to Bobby Holley (:bholley) from comment #3)
> (In reply to Masatoshi Kimura [:emk] from comment #2)
> > It may break existing JavaScript code modules to appear too generic names
> > (such as CSS or URL) suddenly. See bug 920015.
> 
> Precisely. This is my first point in comment 0.

Are there issues besides the |const GloballyExposedName = foo;| bustage, which is a bug in the JS engine?

Comment 5

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)

> Are there issues besides the |const GloballyExposedName = foo;| bustage,
> which is a bug in the JS engine?

There are certainly other examples (e.g. |typeof URL| to determine whether window.URL has been set), though I can't really guess whether they'll come up. I'd rather be deliberate and safe here.

Comment 6

[(no longer active)]

Name suggestion: importGlobalProperties.  </bikeshed>

But who is supposed to call this function?  The creator of the sandbox?  Or the code running inside the sandbox?  I think we should try to minimize the number of places in our code base where we want to adjust when we expose a new global property.

Comment 7

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> Name suggestion: importGlobalProperties.  </bikeshed>

That sounds fine to me.

> But who is supposed to call this function?  The creator of the sandbox?  Or
> the code running inside the sandbox?

This bug isn't about sandboxes. Sandboxes are already doing this (at sandbox creation time). We _definitely_ can't be exposing constructors willy-nilly in Sandboxes, because that defeats their security guarantees and makes them distinctly un-sandboxed.

This bug is about non-DOMWindow things that go through InitClassesWithNewWrappedGlobal, which basically amounts to JSMs, JS-implemented components, frame scripts, and xpcshell. For that, I'm suggesting that the script seeking to use the property should explicitly import it.

> I think we should try to minimize the
> number of places in our code base where we want to adjust when we expose a
> new global property.

We won't have to adjust anything. By definition, there won't be any consumers of a constructor we haven't previously exported, and any new consumers can import it explicitly. It's just like Cu.import-ing a new JSM.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist GlobalProperties out of SandboxOptions. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make GlobalProperties::Parse follow JSAPI exception convention. v1

Anything that returns false should set an exception unless either (a) the
failure resulted from another JSAPI call that would have set an exception or
(b) the failure indicates OOM.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Introduce Cu.importGlobalProperties API. v1

Comment 11

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8ae6f35a526e

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0db39ee9026c

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist GlobalProperties out of SandboxOptions. v2

Comment 14

[Bobby Holley (:bholley)]

This is green.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 811112 [details] [diff] [review]
Part 1 - Hoist GlobalProperties out of SandboxOptions. v2

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

While you're already here, could you please also fix the JSContext* cx to JSContext *cx in these function signatures?

Comment 16

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 811068 [details] [diff] [review]
Part 2 - Make GlobalProperties::Parse follow JSAPI exception convention. v1

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

I wonder if the NS_ENSURE_TRUE(ok, NS_ERROR_INVALID_ARG); makes sense after this change or it should just be NS_ERROR_FAILURE... What's the role of this return value after an explicit error is already reported? I mean, does it matter which one we choose?

Comment 17

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 811069 [details] [diff] [review]
Part 3 - Introduce Cu.importGlobalProperties API. v1

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

+     * Imports global properties (like DOM constructors) into the scope, defining
+     * them on the caller's global. See xpc::GlobalProperties::Parse for the
+     * current list of supported properties.
+     */

Could you mention it briefly that we expect an array of strings here?

+nsXPCComponents_Utils::ImportGlobalProperties(const JS::Value& aPropertyList,
+                                              JSContext* cx)

Value & and JSContext *

Comment 18

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> Comment on attachment 811068 [details] [diff] [review]
> Part 2 - Make GlobalProperties::Parse follow JSAPI exception convention. v1
> 
> Review of attachment 811068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if the NS_ENSURE_TRUE(ok, NS_ERROR_INVALID_ARG); makes sense after
> this change or it should just be NS_ERROR_FAILURE... What's the role of this
> return value after an explicit error is already reported? I mean, does it
> matter which one we choose?

Are you talking about the case in which JS_IsArrayObject(cx, obj) returns false? In that case, I don't see any explicit JS_ReportError calls...

In general, XPConnect first checks for an exception on the cx. If it's there, it propagates that. If it's not, it makes a new JS exception out of the return code.

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0fad858e878
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/05f41a913bee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e3093bdd25

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0fad858e878
https://hg.mozilla.org/mozilla-central/rev/05f41a913bee
https://hg.mozilla.org/mozilla-central/rev/e3e3093bdd25

Comment 21

[Boris Zbarsky [:bzbarsky]]

This really needs to be documented, esp wrt sandboxes...  It's totally not discoverable.

Comment 22

[Will Bamberg [:wbamberg]]

I've added a page for this: https://developer.mozilla.org/en-US/docs/Components.utils.importGlobalProperties. Please let me know if it's OK.

Ont thing: the "-Promise" argument was accepted, when I tried it (unlike, say, "sausages", it didn't throw) but didn't have any effect: Promise was still defined after calling it. I've attached a scratchpad example. Passing -Promise in the wantGlobalProperties option to the Sandbox constructor worked fine. Am I doing something wrong? Is it somehow because I'm using a sandbox for the example? I know it's not designed for sandboxes, as per Bobby's comment 7,  but it just seemed like an easy way to create a system scope for testing.

Comment 23

[Will Bamberg [:wbamberg]]

Attachment: importPromise.js

Comment 24

[Masatoshi Kimura [:emk]]

Probably because the exposing policy has been changed since then.
Now non-Window non-Worker non-Sandbox globals will follow [Exposed=System] extended attribute in the webidl.
https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Promise.webidl?rev=652e8de626d2#19

Comment 25

[Bobby Holley (:bholley)]

Yeah I think we should move away from using importGlobalProperties, and just migrate stuff to [Exposed=Foo]. We'll still need {wantGlobalProperties: true} for sandboxes.