1038399 - Enable WebIDL dictionaries to serialize themselves to JSON

Status: RESOLVED FIXED

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

Description

[Richard Barnes [:rbarnes]]

WebIDL dictionary objects can be initialized from JSON via DictionaryBase::ParseJSON() and the Init(const nsAString& aJSON) methods generated for the individual classes.

What is needed here is:
* A generated method on derived dictionary classes that calls ToJSValue
* A method on DictionaryBase that calls JS_Stringify

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a ToJSON method to Web IDL dictionaries

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8465196 [details] [diff] [review]
Add a ToJSON method to Web IDL dictionaries

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

r=me with those things fixed.

::: dom/bindings/BindingDeclarations.h
@@ +36,5 @@
>    bool ParseJSON(JSContext* aCx, const nsAString& aJSON,
>                   JS::MutableHandle<JS::Value> aVal);
> +
> +  // For convienience, callers can pass false for aConvertedOK, in
> +  // which case we won't examine aValue.

I would say "For Codegen convenience" and "in which case this function is a no-op".

But really, it's not clear to me why this is useful enough to warrant the confusing API. Can't we just explicitly check + propagate ToObjectInternal before doing the StringifyToJSON call?

@@ +41,5 @@
> +  bool StringifyToJSON(JSContext* aCx, bool aConvertedOK,
> +                       JS::MutableHandle<JS::Value> aValue,
> +                       nsAString& aJSON);
> +private:
> +  // aString is expected to actually be an nsAString*.  Should only be called from 

Nit - Incomplete comment and trailing whitespace.

::: dom/bindings/BindingUtils.cpp
@@ +1614,5 @@
> +{
> +  if (!aConvertedOK ||
> +      !JS_Stringify(aCx, aValue, JS::NullPtr(), JS::NullHandleValue,
> +                    AppendJSONToString, &aJSON)) {
> +    JS_ClearPendingException(aCx);

Hm, why squelch the exception here? I think it should bubble up to the nearest AutoJSAPI.

::: dom/bindings/Codegen.py
@@ +10835,5 @@
> +            "ToJSON", "bool",
> +            [Argument('nsAString&', 'aJSON')],
> +            body=dedent("""
> +                MOZ_ASSERT(NS_IsMainThread());
> +                AutoSafeJSContext cx;

Please don't add more AutoSafeJSContexts. Instead, do:

AutoJSAPI jsapi;
jsapi.Init();
JSContext *cx = jsapi.cx();

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Can't we just explicitly check + propagate ToObjectInternal before doing the
> StringifyToJSON call?

We can, but it seems like more code (e.g. have to to the JS_ClearPendingException twice), and we have a lot of dictionary types.  I can do it the other way if you really think the codesize hit is not a big deal.

> Hm, why squelch the exception here? I think it should bubble up to the nearest AutoJSAPI.

What makes you think there is a nearest AutoJSAPI at all?

> Please don't add more AutoSafeJSContexts. Instead, do:

And enter which compartment?  The important part here is that I have to be in some compartment.  It doesn't even matter all that much which one, as long as I'm in one...

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #4)
> We can, but it seems like more code (e.g. have to to the
> JS_ClearPendingException twice)

I'm suggesting getting rid of both JS_ClearPendingException calls.

>, and we have a lot of dictionary types.  I
> can do it the other way if you really think the codesize hit is not a big
> deal.

My suggestion for Codegen.py is this:

MOZ_ASSERT(NS_IsMainThread());
AutoJSAPI jsapi;
jsapi.Init();
JSContext *cx = jsapi.cx();
JSAutoCompartment ac(cx, xpc::GetSafeJSContextGlobal()); // Usage approved by bholley
JS::Rooted<JS::Value> obj(cx);
if (!ToObjectInternal(cx, &obj)) {
  return false;
}
return StringifyToJSON(cx, &obj, aJSON);

I don't think that there's much codesize difference between that and the version where we pass the bool to StringifyToJSON.

> > Hm, why squelch the exception here? I think it should bubble up to the nearest AutoJSAPI.
> 
> What makes you think there is a nearest AutoJSAPI at all?

Because we're adding one (for the Codegen). And for everything else, AutoJSAPI is _almost_ the only place where JSContexts come from these days. Bob is on track to get rid of that "almost" within the next week or two.

> > Please don't add more AutoSafeJSContexts. Instead, do:
> 
> And enter which compartment?  The important part here is that I have to be
> in some compartment.  It doesn't even matter all that much which one, as
> long as I'm in one...

Yeah, just use xpc::GetSafeJSContextGlobal (per above). I'll rename all of these to something like |JSAutoEnterDummyCompartment ac(cx)| at some point.

We want to eliminate AutoSafeJSContext because 90% of the cases don't actually need to be in the dummy global.

Let me know if any of that doesn't make sense.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Because we're adding one (for the Codegen)

Ah, I see.  And it'll just report the exception and move on?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #6)
> > Because we're adding one (for the Codegen)
> 
> Ah, I see.  And it'll just report the exception and move on?

Correct, unless the caller steals and propagates it (which, in this case, it won't).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Great.  Then comment 5 sounds reasonable to me, except with:

  return ToObjectInternal(cx, &obj) && StringifyToJSON(cx, &obj, aJSON);

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c67ef1f9515

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c67ef1f9515

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c67ef1f9515

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465680 [details] [diff] [review]
Updated to comments

Approval Request Comment
[Feature/regressing bug #]: Not a regression
[User impact if declined]: Can't uplift bug 1033423 
[Describe test coverage new/current, TBPL]: 
[Risks and why]: Low risk.  Only changes shipping code in webcrypto, which is not enabled by default.
[String/UUID change made/needed]: None.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8465680 [details] [diff] [review]
Updated to comments

This is needed for dependent bug 1033423. Aurora+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2479de9ab541