All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Unassigned)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla27
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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?
I don't see the issue with exposing DOM APIs to all scopes where they make sense.
OS: Mac OS X → All
Hardware: x86 → All
It may break existing JavaScript code modules to appear too generic names (such as CSS or URL) suddenly. See bug 920015.
(Reporter)

Comment 3

5 years ago
(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

5 years ago
(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?
(Reporter)

Comment 5

5 years ago
(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

5 years ago
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.
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Updated

5 years ago
Depends on: 921399
(Reporter)

Comment 8

5 years ago
Created attachment 811067 [details] [diff] [review]
Part 1 - Hoist GlobalProperties out of SandboxOptions. v1
Attachment #811067 - Flags: review?(gkrizsanits)
(Reporter)

Comment 9

5 years ago
Created attachment 811068 [details] [diff] [review]
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.
Attachment #811068 - Flags: review?(gkrizsanits)
(Reporter)

Comment 10

5 years ago
Created attachment 811069 [details] [diff] [review]
Part 3 - Introduce Cu.importGlobalProperties API. v1
Attachment #811069 - Flags: review?(gkrizsanits)
(Reporter)

Comment 13

5 years ago
Created attachment 811112 [details] [diff] [review]
Part 1 - Hoist GlobalProperties out of SandboxOptions. v2
Attachment #811067 - Attachment is obsolete: true
Attachment #811067 - Flags: review?(gkrizsanits)
Attachment #811112 - Flags: review?(gkrizsanits)
(Reporter)

Updated

5 years ago
Blocks: 921478
(Reporter)

Comment 14

5 years ago
This is green.
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?
Attachment #811112 - Flags: review?(gkrizsanits) → review+
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?
Attachment #811068 - Flags: review?(gkrizsanits) → review+
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 *
Attachment #811069 - Flags: review?(gkrizsanits) → review+
(Reporter)

Comment 18

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/f0fad858e878
https://hg.mozilla.org/mozilla-central/rev/05f41a913bee
https://hg.mozilla.org/mozilla-central/rev/e3e3093bdd25
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
This really needs to be documented, esp wrt sandboxes...  It's totally not discoverable.
Keywords: dev-doc-needed
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.
Flags: needinfo?(bobbyholley)
Created attachment 8530530 [details]
importPromise.js
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
(Reporter)

Comment 25

4 years ago
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.
Flags: needinfo?(bobbyholley)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.