Payment-request: The myth of JSON-serializable object

[marcoscaceres]

I might be wrong, but I recall "JSON-serializable object" coming up in the past in other specs and being problematic:

The term JSON-serializable object used in this specification means an object that can be serialized to a string using JSON.stringify and later deserialized back to an object using JSON.parse with no loss of data.

But I'm pretty sure it's a bit of a 🦄 - or it should be actually defined as to how one actually works that out. Right now, the spec basically implies:

try {
   JSON.parse(JSON.stringify(obj)); 
} catch(err){
   throw new Error("Not a JSON-serializable object");
}

That seems nasty. Could we check with public-script-coord what the right thing to do here is?

There might be an actual object type that we can use here.

[marcoscaceres]

Pinged public-script-coord:
https://lists.w3.org/Archives/Public/public-script-coord/2016OctDec/0071.html

[rsolomakhin]

We require JSON-serializable object to avoid this:

var obj = {foo: null};
obj.foo = obj;

This is valid JavaScript, but cannot be serialized into a JSON string.

Serialization is necessary for communication to payment apps. Android apps can't receive JavaScript objects, so they take in a JSON string instead.

[adrianba]

The goal here was broadly an author requirement for payment apps. Developers should not expect types that can't be readily serialised to work correctly here. I welcome any language to clarify this but it isn't something to lose too much sleep over.

[bzbarsky]

It's something to lose sleep over because this is a requirement on user agents (they are supposed to reject non-json-serializable cases) but nothing defines what "json-serializable" really means, which means there can't be an actual implementation of the spec as written.

As an implementor I would raise a formal objection to this whole bit. As a test writer, I'm quite certain I would be able to create tests that show that UAs don't implement this part of the spec interoperably (which would mean the spec cannot proceed to REC, per the two interoperable implementations criteria).

Please define what's actually supposed to happen.

[RByers]

I'm trying to understand the fundamental difference of opinion here, please bear with me.

Is there a concern that JSON.parse/stringify behavior isn't precisely specified by ECMA 262 and ECMA 404? From a naive skim of those specs it seems to me like they do precisely define which "objects can be serialized to a string using JSON.stringify and later deserialized back to an object using JSON.parse with no loss of data" (although "no loss of data" should probably be precisely defined with an algorithm, but I suspect we can all agree on what that algorithm would be).

Or is the concern that, despite being precisely specified, the definition is complex and so it's hard to get a conforming implementation in this context (and so existing implementations likely already don't conform to that definition)?

FWIW the blink implementation appears to rely just on our JSON.stringify method succeeding. So the main opportunities I'd see for this not to conform to the above rough definition are:

• Cases where stringify generates a string which will fail to parse (hopefully not allowed by the JSON spec?).
• Cases where stringify generates a string which, when parsed, produces an object graph that doesn't contain all the properties of the original.
• Cases where round-tripping via stringify and parse produce an object with different values. Eg. [undefined] stringifys to [null].

So we definitely have conformance bugs in blink (data:{a:undefined} and data:{a:navigator} should throw but don't in blink). I've filed this implementation issue.

Perhaps the primary issue for the WG here is conformance testing? If the definition is written is in fact mostly OK, how should we thoroughly test it such that bugs like blink's would be caught? Could we leverage the test262 JSON tests at all?

[RByers]

Or perhaps it would be simpler to just precisely define a validation along the lines of this:

The value must satisfy:

• has typeof of "number", "string", "boolean", or "object"
• if it's "object":
  
  
  
  • it's prototype is Object.prototype or Array.prototype
  
  
  • all it's ownProperties recursively satisfy these rules
  
  
  • if the prototype is Array.prototype:
    
    
    
    
    
    • it's only ownProperties are 'length' and all the integers between 0 and length-1
    
    
    
  
  
• there are no cycles

I'm guessing we might end up with such an implementation in blink, so perhaps making a definition that's somewhat redundant with the JSON spec (and so requiring a dedicated test suite separate from test262) is the more practical option?

[bzbarsky]

Is there a concern that JSON.parse/stringify behavior isn't precisely specified

No. But note that this spec never invokes JSON.parse/stringify. See also https://github.com/w3c/browser-payment-api/issues/338

From a naive skim of those specs it seems to me like they do precisely define which "objects can be serialized to a string using JSON.stringify and later deserialized back to an object using JSON.parse with no loss of data"

It really depends on your definition of "loss of data".

but I suspect we can all agree on what that algorithm would be

Maybe. See thread starting at https://lists.w3.org/Archives/Public/public-script-coord/2016OctDec/0071.html if you haven't seen it yet.

FWIW the blink implementation appears to rely just on our JSON.stringify method succeeding

Note that invoking JSON.stringify has side-effects, so if the intent is that it be invoked then the spec has to say _exactly_ when that happens. The spec clearly doesn't do that.

My post in the above linked public-script-coord thread attempts to outline a partial (necessary by not sufficient) test for a side-effect-free definition of "no loss of data", but as I note in that thread it's not only hard to specify but probably doesn't actually produce behavior anyone wants.

Cases where stringify generates a string which will fail to parse

Can't happen.

Cases where stringify generates a string which, when parsed, produces an object graph that doesn't contain all the properties of the original.

This really depends on how you define "properties". For example, stringify followed by parse will change your prototypes if you the parse in a different global from the stringify... Should that count? Probably not.

Cases where round-tripping via stringify and parse produce an object with different values. Eg. [undefined] stringifys to [null].

Lots of those cases, yes.

Perhaps the primary issue for the WG here is conformance testing?

The primary issue is that the behavior is NOT DEFINED.

The value must satisfy:

These are not nearly enough; see the public-script-coord spec.

there are no cycles

This is not an invariant of the object graph, when proxies are involved.

[webpayments-specs]

On Wed, Nov 30, 2016 at 1:20 PM, Rick Byers [e-mail]
wrote:

The value must satisfy:

• has typeof of "number", "string", "boolean", or "object"

Only "object" is allowed, because the spec says "JSON-serializable object".

[bzbarsky]

Only "object" is allowed, because the spec says "JSON-serializable object".

@RByers applies these rules recursively to the property values, so they better allow more than just "object" unless you want to encode everything in the shape of the object graph itself. ;)

[bzbarsky]

To be clear, the way I would spec this is that the spec algorithm just calls JSON.stringify on the "data". If that throws, propagate out the exception. Then hand around the string.

[RByers]

See thread starting at https://lists.w3.org/Archives/Public/public-script-coord/2016OctDec/0071.html if you haven't seen it yet.

Ah! I saw the post but somehow thought there weren't any replies yet (damn I hate the hypermail UI). Sorry for the noise.

To be clear, the way I would spec this is that the spec algorithm just calls JSON.stringify on the "data". If that throws, propagate out the exception. Then hand around the string.

I had avoided that proposal because I thought it would be a potential interop issue since it's impossible to test that the UA is actually discarding all the non-serializable properties. But that's probably a silly concern - easy to satisfy via code inspection. So I agree - that's a much better proposal than what I was suggesting.

[zkoch]

We chatted about this on the editor call this morning. We'd be happy to merge in a PR that updated the spec to use json.stringify and propagate the exception. Could either @bzbarsky or @marcoscaceres take a stab at a PR for this?

[bzbarsky]

Are we sure we want to take an object as input, not an existing JSON string?

If we are (e.g. if callers are expected to construct the object on the fly instead of producing it from JSON to start with), it would be good to sort out https://github.com/w3c/browser-payment-api/issues/346 and https://github.com/w3c/browser-payment-api/issues/338 too so the processing model is actually defined. It's hard to write a useful PR without knowing what should happen to the string after it's generated.

[bzbarsky]

Are we sure we want to take an object as input, not an existing JSON string?

If we are (e.g. if callers are expected to construct the object on the fly instead of producing it from JSON to start with), it would be good to sort out https://github.com/w3c/browser-payment-api/issues/346 and https://github.com/w3c/browser-payment-api/issues/338 too so the processing model is actually defined. It's hard to write a useful PR without knowing what should happen to the string after it's generated.

[littledan]

To the extent that you use JSON.stringify, I hope you're specific to refer to "the original value of" it, to be clear that monkey-patching doesn't affect it. Or, if you do want overwriting the stringify property of the JSON object to have this effect, then being clear about that in the spec would be good as well. The cleanest way layering-wise to refer into the ECMAScript spec would be a slight refactoring into an "abstract operation" that can't be overwritten. cc @domenic who has done a number of these layering adjustments in the past.

I agree with @bzbarsky that ECMAScript doesn't really define the machinery to detect without side effects whether an object is JSON-stringifyable as defined above. It is potentially possible to define it, but it would be pretty ugly (violating some invariants that we try to maintain about the JS object model) and a lot of work to get right. Given that users already have to worry about losing data when calling JSON.stringify directly, how much worse is it that they may also encounter that issue in this case?

Is this part of the API already shipped to users in some browsers? If so, compatibility would be a pretty strong reason to continue taking an object as input, to be serialized to JSON, rather than a string, even if it's not conceptually the cleanest model.

[rsolomakhin]

Is this part of the API already shipped to users in some browsers?

Shipped in Chrome since September 2016.

[bzbarsky]

And the thing that's shipped totally doesn't match what the spec is saying right now, which is the best part of the whole exercise, of course.

[rsolomakhin]

I'd like to think that the implementation follows the spirit of the spec. The spec language should be improved, of course. If the implementation is doing something horrible, I'm happy to change it.

[bzbarsky]

The whole point of specs is to follow their _letter_, not just their spirit. Otherwise, why bother with a spec? And implementors who discover they can't follow the letter for some reason should raise corresponding spec issues. Which means this issue should have been raised before Google shipped. :(

[littledan]

While the situation is not optimal, it seems like we have a concrete way to move forward here: continue with the reviews that Mozilla has kicked off, tighten up language and layering, and make web-compatible tweaks where it makes sense. This API gives a lot of value for users, and the Web has endured much worse, so I think we can get through this.

Has anyone from the Chrome side documented the way that the implementation follows the "spirit" of the spec and any potential mismatches? If not, that would probably be a good exercise to complement what Mozilla has been contributing in bugs like this one.

[rsolomakhin]

Has anyone from the Chrome side documented the way that the implementation follows the "spirit" of the spec and any potential mismatches?

We run the object through JSON.stringify() and propagate any exceptions.

[domenic]

I looked into fixing this and it's basically blocked on #346 and #338. That is, once I stringify the objects, what do we do with them? Just throw them away? Apparently they're passed on to the payment processor somehow, but that part of the spec is missing. I talked with @zkoch about what it should say roughly, so I can try a stab at it soon.

[domenic]

After working on https://github.com/w3c/webpayments-methods-card/issues/20 this is more confusing than ever. As far as I can tell the desired semantics for basic card are:

1. JSON-stringify then JSON-parse the data value just so that it throws exceptions
2. Then, ignore all that, and convert data to a dictionary
3. Presumably, at some point, the user agent uses the data in the dictionary (i.e. the C++ object) to perform basic card stuff. This is never specified anywhere (#338).

To me it seems like step 1 is entirely redundant, for basic card.

If the desire is to support HTTP-based payment methods that require serialization, then they should define steps to serialize to JSON and send over HTTP and use that to compose a response. But for basic card, this shouldn't be necessary at all, since the normalization is already done by the dictionary conversion step.

So JSON-serializability should be a payment-method-specific concern, not part of the overall PaymentRequest infrastructure.

[rsolomakhin]

@domenic, you probably know best about the appropriate places to specify serializability. Here's my understanding of why the spec was written like this:

Re: step 1, user agent should only JSON-stringify the data in order to pass it to the payment app. Here JSON-stringify is used as serialization for IPC. Payment apps are expected to be running in a different process, so all data destined for the payment app should be serializable.

Re: step 2, JSON-parse and "convert data to a dictionary" are equivalent, no? This should happen in the payment app process, which has received the data serialized as a string.

[domenic]

I think I see. Hmm, so if it's in another process, we can't do synchronous validation of the data, right? So we can't throw an exception in the constructor. In that case the flow should be something more like:

• In the constructor, do JSON.stringify. Store the resulting string.
• When we invoke the "user accepts the payment request algorithm", send the string to the payment method (e.g. to basic card).
• Basic card first does JSON.parse on the resulting string. This will never fail since it was produced by JSON.stringify.
• Then basic card converts the resulting parsed JS object to a dictionary. This can fail. At this point there would need to be a communication channel for showing an error back to the user.

Does this sound right?

[rsolomakhin]

This is right.

Then basic card converts the resulting parsed JS object to a dictionary. This can fail. At this point there would need to be a communication channel for showing an error back to the user.

I think we get this for free. If the payment app is unable to handle a payment request, then it will be filtered out of possible payment methods. If no payment app is available to handle the payment request, then .show() is rejected with NotSupportedError exception. Do you think that works? If that works, should we write it down (either normatively or as a note)?

[domenic]

I think we get this for free. If the payment app is unable to handle a payment request, then it will be filtered out of possible payment methods. If no payment app is available to handle the payment request, then .show() is rejected with NotSupportedError exception. Do you think that works? If that works, should we write it down (either normatively or as a note)?

We should definitely write something down normatively. I don't quite understand the envisioned flow here though. When would this filtering happen? How does it know it's unable to handle the payment request, until it parses and converts the object in the other process?

[domenic]

Stated another way: I had assumed steps 8/9 of https://w3c.github.io/browser-payment-api/#show-method were about very course-grained "this user agent accepts basic-card or not". But are you saying there is also some kind of implicit fine-grained introspection of the data there too?

[rsolomakhin]

When would this filtering happen?

The filtering happens in the .show() method.

User agent checks the intersection of all the payment methods that the merchant website specified in new PaymentRequest(supportedMethods, shoppingCart, options) constructor with all payment methods that the installed payment apps have declared to support. If the intersection is non-empty, user agent lets the user select one of the payment apps. If the intersection is empty, the user agent rejects the .show() promise with NotSupportedError exception.

How does it know it's unable to handle the payment request, until it parses and converts the object in the other process?

User agent cannot know this synchronously. The object needs to be serialized and passed to the payment apps first. @adamroach's latest take on the Payment App API has both enabledMethods sequence and canHandle(supportedMethods, shoppingCart) method that the payment app provides to the user agent at registration time. User agent can use these to asynchronously filter the payment apps that the user can select.

Are you saying there is also some kind of implicit fine-grained introspection of the data there too?

This depends on the payment app. Some payment app might tell user agent that it supports "basic-cad", period. That's coarse grained. An other payment app might inspect the supportedMethods of the payment request to make a fine-grained judgement of whether it can handle that specific instance of "basic-card" payment request.

[domenic]

OK, thanks so much! I think I see how this can work then. I'll try a new couple of PRs and you can tell me what you think of the result.

[RByers]

Note that one of the ways this could be a compat problem in practice is that (although it's obscure) some developers / libraries know to use a pattern like this to feature-detect support for dictionary members. From a quick skim I think this could be used today on Chrome to detect, for example, when some fields are being parsed and when they're not. If sites depended on that, it could become a compat problem to change it.

[domenic]

Oh, oops, this should not have been auto-closed as the term is still used for PaymentDetailsModifier. I accidentally said "does not completely fix #307" which triggered GitHub's heuristics. If someone could reopen that would be great :).

[zkoch]

Sorry, my fault! :)