Enable WebIDL dictionaries to serialize themselves to JSON

RESOLVED FIXED in Firefox 33

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: rbarnes, Assigned: bz)

Tracking

unspecified
mozilla34
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
Duplicate of this bug: 1046530
Created attachment 8465196 [details] [diff] [review]
Add a ToJSON method to Web IDL dictionaries
Attachment #8465196 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
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();
Attachment #8465196 - Flags: review?(bobbyholley) → review+
> 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...
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
> Because we're adding one (for the Codegen)

Ah, I see.  And it'll just report the exception and move on?
(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).
Great.  Then comment 5 sounds reasonable to me, except with:

  return ToObjectInternal(cx, &obj) && StringifyToJSON(cx, &obj, aJSON);
Attachment #8465196 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c67ef1f9515
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla34
https://hg.mozilla.org/mozilla-central/rev/9c67ef1f9515
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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.
Attachment #8465680 - Flags: approval-mozilla-aurora?
Comment on attachment 8465680 [details] [diff] [review]
Updated to comments

This is needed for dependent bug 1033423. Aurora+
Attachment #8465680 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2479de9ab541
status-firefox33: --- → fixed
status-firefox34: --- → fixed
You need to log in before you can comment on or make changes to this bug.