1246153 - Make dictionary init from a JSON string work on workers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Needed for webcrypto on workers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ugh.  The SpiderMonkey folks are going to hate me... ;)

The reason for that is that once we have parsed the JSON string into an object trying to init a dictionary with it will do [[Get]] calls on that object.  And nothing says the JSON string includes those properties, so we'll hit Object.prototype.

Now we can work around this in three ways, I think:

1) Use a clean scope.  This is what we tried to avoid in bug 1241349 for memory
   consumption reasons.
2) Hack JS_ParseJSON to add a mode whereby the objects and arrays it creates are
   given a null prototype.
3) Maybe use a reviver that goes through the graph and clones over all the
   objects/arrays with null protos and copies all the properties.

Jeff, option 2 is by far the easiest to implement and has the least performance suck, but I dunno how you feel about it...

Comment 2

[Jeff Walden [:Waldo]]

This is part of the same webcrypto for workers thing that had you wanting to add JS::ToJSONMaybeSafely, right?  So in addition to a hack to the stringification side, you're proposing a hack to the parsification side.  Could you spell out exactly the code that triggers any of this nonsense, so I have an idea exactly what it is you're doing, and why you're trying to shoehorn any of this into a JSON transport protocol versus using (say) structured cloning or similar?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Could you spell out exactly the code that triggers any of this nonsense

Sure.  The relevant bit of the webcrypto spec is JWK import and export.  Basically, the spec defines some mappings between some internal data structures and JSON and we need to go back and forth between them.

The way this is currently implemented in Gecko is that we define a Web IDL dictionary, and have ways to init it from a JSON string (this parses the JSON, which produces a JSObject* and then inits the dictionary from the resulting object) and to serialize it to a JSON string (this converts the dictionary struct to a JSObject* and then serializes that to JSON).  The current setup has the nice property that it doesn't involve us writing our own (likely buggy) JSON parser and serializer.

For the particular entry point for this bug, http://hg.mozilla.org/mozilla-central/file/815d689a6e1e/dom/crypto/WebCryptoTask.cpp#l1432 is the relevant bit: it has a key data string and wants to produce a JsonWebKey dictionary from it.  This is defined in the spec at https://www.w3.org/TR/WebCryptoAPI/#dfn-JsonWebKey and JSON is involved because <https://tools.ietf.org/html/draft-ietf-jose-json-web-key-41>.  So it's not _me_ trying to shoehorn things into JSON; that's been done for me already by the nice standards people.  The spec entry point here is https://www.w3.org/TR/WebCryptoAPI/#concept-parse-a-jwk and I expect they didn't realize that can have side-effects if done in the wrong global.  Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=29437

Anyway, that's the parsing end.  The serialization end is entered in our code at http://hg.mozilla.org/mozilla-central/file/815d689a6e1e/dom/crypto/WebCryptoTask.cpp#l3126 and corresponds to https://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-method-wrapKey step 14 the "jwk" case substep 2.  Again, raised spec issue at <https://www.w3.org/Bugs/Public/show_bug.cgi?id=29438>.

I totally agree that this all sucks, for sure.

Comment 4

[Jeff Walden [:Waldo]]

Bah.  I don't like option 2 at all, really.  But I guess it's a temporary path forward, until we can eliminate the middleman and parse JSON directly into the actual thingy wanted.  We really need a templated JSON parsing algorithm that doesn't have any of the nonsense overhead going through JS objects and junk does...  :-(

Comment 5

[Boris Zbarsky [:bzbarsky]]

So I started implementing this and realized that option 2 from comment 0 is totally busted, as is option 3.  They're busted for two reasons, one theoretical and one not:

1)  Theoretical: Just setting prototypes to null is not good enough to produce behavior that is black-box indistinguishable from having a separate global.  For example, if the JSON has an object where the dictionary wants a string, we would get an exception instead of "[object Object]".  Just try it: String({ __proto__: null }).

2)  Not theoretical: The particular dictionary involved for WebCrypto is JsonWebKey which has a sequence-type property.  That property can only be initialized from an iterable, and an Array with a null proto is not iterable, because the Symbol.iterator is on Array.prototype.  And even if we stuck it on the instance, we'd still be sharing ArrayIteratorPrototype with the page, which is bad.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Tim, how much do we expect SetJwkFromKeyData to be called and how much do we care about its performance?  We _could_ just create a new global in that function, use it for the parse, and then discard it (at least on workers), if that's not an insane thing to be doing....

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

How much is quite dependent on how often/fast the website/app calls crypto.subtle.importKey("jwk"). I wouldn't expect this to show up in a hot loop, and given that it's an async operation I doubt that people would complain about a few ms spent on creating and discarding a new global, especially on a worker that is likely used to not block the main thread when doing something rather expensive.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  Creating a new window (by appending an iframe to the DOM) takes about a millisecond on my laptop.  Creating a vanilla global should be somewhat faster.  So sounds like that's the way to go...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Restrict initialization of dictionaries from JSON to dictionaries that can actually be represented in JSON

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Create a way to ask for a clean new global that works on both mainthread and workers

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use the new clean global setup for doing from-JSON creation of dictionaries

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8735045 [details] [diff] [review]
part 2.  Create a way to ask for a clean new global that works on both mainthread and workers

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

::: dom/bindings/BindingUtils.cpp
@@ +3275,5 @@
>  }
> +
> +// XXXbz this setup for clean globals is a lot like the worker debugger sandbox,
> +// but we can't just share the code because there are explicit checks for its
> +// jsclass...

It seems like we _could_ actually share most of the code - the hooks, the creation code, and most of the jsclass stuff, and just have a macro that declares different instances of the JSClass, right?

We've spent a long time trying to reduce the proliferation of random from-scratch JSClasses throughout gecko, so I'm not really wild about starting that trend again if we can avoid it.

Looking at the WorkerDebuggerGlobalScope stuff, it's also very unfortunate that that code is trying to reinvent the wheel of sandboxPrototype and doing it badly. Ideally we could one-day merge the XPC sandbox machinery into this unified setup as well.

@@ +3330,5 @@
> +    }, JS_NULL_OBJECT_OPS
> +};
> +
> +JSObject*
> +CreateCleanNewGlobal()

We could templatize this on the concrete nsIGlobalObject type and share it as well.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8735046 [details] [diff] [review]
part 3.  Use the new clean global setup for doing from-JSON creation of dictionaries

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

(and sorry for the terrible lag here.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note to self: Make the JSClass definition public in bindings, hang an enum off our basic global object C++ class so we can determine worker sandboxes, add an arg for that enum and the proto (Value, undefined for "don't mess") to CreateCleanNewGlobal(), and make worker debugger sandbox just use CreateCleanNewGlobal().

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 2 to the new setup

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Create a way to ask for a clean new global that works on both mainthread and workers

I'm a bit torn about whether to just put the new function in BasicGlobalObject.h/cpp instead of BindingUtils....

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8736967 [details] [diff] [review]
part 2.  Create a way to ask for a clean new global that works on both mainthread and workers

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

r=me with those things fixed.

::: dom/bindings/BasicGlobalObject.h
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * A basic nsIGlobalObject implementation that can be used to set up a new
> + * global without anything interesting in it other than the JS builtins.  This

I might prefer 'PlainGlobalObject' or 'SimpleGlobalObject' to 'BasicGlobalObject', but I don't care much. Up to you

@@ +26,5 @@
> +                          public nsWrapperCache
> +{
> +public:
> +  enum class GlobalType {
> +    General,

I'd call this type BindingDetail.

@@ +29,5 @@
> +  enum class GlobalType {
> +    General,
> +    WorkerDebuggerSandbox
> +  };
> +    

Nit: Whitespace.

::: dom/bindings/BindingUtils.cpp
@@ +3318,5 @@
> +    static_cast<BasicGlobalObject*>(JS_GetPrivate(obj));
> +  globalObject->UpdateWrapper(obj, old);
> +}
> +
> +const js::Class CleanGlobalClass = {

It seems like all this should go in BasicGlobalObject.cpp, and s/Clean/Basic/.

@@ +3341,5 @@
> +    }, JS_NULL_OBJECT_OPS
> +};
> +
> +JSObject*
> +CreateCleanNewGlobal(BasicGlobalObject::GlobalType globalType,

CreateBasicGlobal, and I agree it should go in BasicGlobal.{cpp,h}.

@@ +3351,5 @@
> +  JS::CompartmentOptions options;
> +  options.creationOptions().setInvisibleToDebugger(true);
> +
> +  JS::Rooted<JSObject*> global(cx,
> +    JS_NewGlobalObject(cx, js::Jsvalify(&CleanGlobalClass), nullptr,

We shouldn't be creating any JS globals on the main thread without a principal. The best thing to do here is probably to check NS_IsMainThread, and mint a new nsNullPrincipal in that case.

::: dom/bindings/BindingUtils.h
@@ +3356,5 @@
> +//
> +// Note that creating new globals is not cheap and should not be done
> +// gratuitously.  Please think carefully before you use this function.
> +JSObject* CreateCleanNewGlobal(BasicGlobalObject::GlobalType globalType =
> +                                 BasicGlobalObject::GlobalType::General,

I think this shouldn't be a default param.

::: dom/workers/WorkerScope.cpp
@@ +909,5 @@
> +
> +  nsIGlobalObject* globalObj = xpc::NativeGlobal(object);
> +  BasicGlobalObject* basicGlobal = static_cast<BasicGlobalObject*>(globalObj);
> +  return
> +    basicGlobal->Type() == BasicGlobalObject::GlobalType::WorkerDebuggerSandbox;

I think this should move into a helper method in BasicGlobalObject.h called BasicGlobalType(JSObject*).

This might involve adding a sentinel value to the enum or something, which is fine.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I might prefer 'PlainGlobalObject' or 'SimpleGlobalObject' to 'BasicGlobalObject', but I don't care much. Up to you

SimpleGlobalObject it is.

> I'd call this type BindingDetail.

That doesn't really make sense to me.  What "binding"?  This is really describing the type of global we're dealing with...

Unless you meant the enum value, not the name of the enum class?  If so, that sort of makes sense.  I will do that, with a comment.

> It seems like all this should go in BasicGlobalObject.cpp, and s/Clean/Basic/.

Done, with s/Clean/Simple/

> CreateBasicGlobal, and I agree it should go in BasicGlobal.{cpp,h}.

Done, with CreateSimpleGlobal.  I didn't make it a static method in SimpleGlobalObject, though maybe I should: SimpleGlobalObject::Create, but returning a JSObject*....

> The best thing to do here is probably to check NS_IsMainThread, and mint a new nsNullPrincipal in that case.

Done.  Do I need to keep the relevant nsIPrincipal alive via SimpleGlobalObject, or will the JS engine keep it alive?  Yay lack of docs for JS_NewGlobalObject.  :(

> I think this shouldn't be a default param.

OK, done.

> I think this should move into a helper method in BasicGlobalObject.h called BasicGlobalType(JSObject*).

Done.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to review comments

Comment 20

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #18)
> > I might prefer 'PlainGlobalObject' or 'SimpleGlobalObject' to 'BasicGlobalObject', but I don't care much. Up to you
> 
> SimpleGlobalObject it is.
> 
> > I'd call this type BindingDetail.
> 
> That doesn't really make sense to me.  What "binding"?  This is really
> describing the type of global we're dealing with...
> 
> Unless you meant the enum value, not the name of the enum class?  If so,
> that sort of makes sense.  I will do that, with a comment.

Yes, that is what I meant - "type" in the "type of global" rather than "C++ type" sense.

> 
> > It seems like all this should go in BasicGlobalObject.cpp, and s/Clean/Basic/.
> 
> Done, with s/Clean/Simple/
> 
> > CreateBasicGlobal, and I agree it should go in BasicGlobal.{cpp,h}.
> 
> Done, with CreateSimpleGlobal.  I didn't make it a static method in
> SimpleGlobalObject, though maybe I should: SimpleGlobalObject::Create, but
> returning a JSObject*....

CreateAndReflect? Create is also fine.

> > The best thing to do here is probably to check NS_IsMainThread, and mint a new nsNullPrincipal in that case.
> 
> Done.  Do I need to keep the relevant nsIPrincipal alive via
> SimpleGlobalObject, or will the JS engine keep it alive?  Yay lack of docs
> for JS_NewGlobalObject.  :(

The JS engine will refcount nsJSPrincipals.

> 
> > I think this shouldn't be a default param.
> 
> OK, done.
> 
> > I think this should move into a helper method in BasicGlobalObject.h called BasicGlobalType(JSObject*).
> 
> Done.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> CreateAndReflect? Create is also fine.

Does that mean you prefer mozilla::dom::SimpleGlobalObject::Create() to mozilla::dom::CreateSimpleGlobal()?

Comment 22

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> > CreateAndReflect? Create is also fine.
> 
> Does that mean you prefer mozilla::dom::SimpleGlobalObject::Create() to
> mozilla::dom::CreateSimpleGlobal()?

I think so, though I don't have a strong opinion.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f15eba10b3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0041fb22002
https://hg.mozilla.org/integration/mozilla-inbound/rev/d911a849718c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f15eba10b3e
https://hg.mozilla.org/mozilla-central/rev/e0041fb22002
https://hg.mozilla.org/mozilla-central/rev/d911a849718c