We should auto-JSON and auto-un-JSON objects that are passed as messages to make life easier.

link to spec?

The spec currently does not allow for this. It would be a mozilla extension.

Now waiting on bug 408838 before this will work.

Here we go.

Hmm.. so if calling into the JSON code throws, we should probably rethrow the exact same error.

Currently all I can do is return an nsresult. Chatting with sicking offline we decided that we'll wait for a spec before getting picky about which errors are thrown.

Comment on attachment 348646 [details] [diff] [review] Patch, v1 - In GetStringForArgument(): + if (JSVAL_IS_PRIMITIVE(argv[0]) || JSVAL_IS_VOID(argv[0])) { JSVAL_IS_PRIMITIVE() covers JSVAL_IS_VOID(), so no need for that check here. + JSObject* obj = JS_NewObject(cx, NULL, NULL, NULL); + NS_ENSURE_TRUE(obj, NS_ERROR_OUT_OF_MEMORY); + + jsonVal = OBJECT_TO_JSVAL(obj); + holder = jsonVal; + + ok = JS_DefineProperty(cx, obj, JSON_PRIMITIVE_PROPNAME, argv[0], NULL, + NULL, JSPROP_ENUMERATE); You might want to comment what you're doing and why. + else { + NS_ASSERTION(JSVAL_IS_OBJECT(argv[0]), "Bad jsval!"); This'll hit if you ever get an XML object in here, so maybe just remove the assertion since you check for that further down? + ok = JS_TryJSON(cx, vp); + if (!(ok && !JSVAL_IS_PRIMITIVE(*vp) && + (type = JS_TypeOfValue(cx, *vp)) != JSTYPE_FUNCTION && + type != JSTYPE_XML)) { + return NS_ERROR_INVALID_ARG; + } + + nsJSONWriter writer; + + ok = JS_Stringify(cx, vp, NULL, &WriteCallback, &writer); Before you call JS_Stringify() you should reset holder to hold *vp, as it could be unprotected here. - In nsDOMWorkerMessageEvent::GetData(): + JSONParser* parser = JS_BeginJSONParse(cx, &jsonVal); + NS_ENSURE_TRUE(parser, NS_ERROR_UNEXPECTED); + + ok = JS_ConsumeJSONText(cx, parser, (jschar*)mData.get(), + (uint32)mData.Length()); + NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED); + + ok = JS_FinishJSONParse(cx, parser); Seems like you always have to call JS_FinishJSONParse() after you've called JS_BeginJSONParse() or you'll leak the parser. + cc->SetReturnValueWasSet(PR_TRUE); return NS_OK; As you pointed out, you might also want to cache the jsval you're returning here so repeated calls to GetData() doesn't re-parse the JSON. r+sr=jst with that.

Comment on attachment 348646 [details] [diff] [review] Patch, v1 New patch on the way.

Fixes comments, updates tests.

Comment on attachment 349298 [details] [diff] [review] Additional changes ok = JS_ConsumeJSONText(cx, parser, (jschar*)mData.get(), (uint32)mData.Length()); - NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED); - ok = JS_FinishJSONParse(cx, parser); - NS_ENSURE_TRUE(ok, NS_ERROR_UNEXPECTED); + ok = JS_FinishJSONParse(cx, parser) && ok; + if (!ok) { Sneaky way to avoid another JSBool :) Maybe add a comment so others don't miss that, as I did? :) r+sr=jst

Or make it ok = ok && JS_...

No, JS_FinishJSONParse() must be called even if ok is false.

Well then, you could make it ok = ok & JS_... or if you want to be really sure: ok = !!ok & !!JS_...

With sneakiness comment.

(In reply to comment #13) > Well then, you could make it > > ok = ok & JS_... We use ok &= JS_...; in other places. It's kosher. > or if you want to be really sure: > > ok = !!ok & !!JS_... That's ugly and unnecessary :-P. /be

Comment on attachment 349341 [details] [diff] [review] Final patch I can't see this as a blocker, but given that we have a patch that's ready to go I don't see a reason not to take this now.

(In reply to comment #16) > (From update of attachment 349341 [details] [diff] [review]) > I can't see this as a blocker, but given that we have a patch that's ready to > go I don't see a reason not to take this now. That is not the standard for 1.9.1.

True, it's not, but this has been talked about at length offline and we see no reason not to include this in 1.9.1. This adds great value at no risk of regressing existing code (as this is a tweak to a new feature). This would have been included in the initial landing of the feature had we realized then that we should do this, and if we'd had the time to do so. I stand by my decision here, and I know Jonas and Ben feel the same way.

Yeah, the patch is low risk (given the small size and isolated in what it touches) and high value (makes using workers a lot easier and also keeps us compatible with a future change that is likely to the spec) I think we should take it. I do agree that it's not an obvious call though, and if we have to spend any time on bug hunting we should just back out rather than try to fix.

Backed out due to test failures: http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1227823516.1227823714.29578.gz#err0 http://hg.mozilla.org/mozilla-central/rev/ec6ae3ecb881

Oh, lame, the fix for bug 465371 snuck into the patch. Sorry.

Pushed changeset b4bbb3c351a6 to mozilla-central.

Pushed changeset a856ffc040a2 to mozilla-1.9.1.