453865 - Workers: Allow JSON-able objects to be passed as messages to worker threads

Status: VERIFIED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 1

[Robert Sayre]

link to spec?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Now waiting on bug 408838 before this will work.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Here we go.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 7

[Johnny Stenback (:jst)]

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 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 348646 [details] [diff] [review]
Patch, v1

New patch on the way.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Additional changes

Fixes comments, updates tests.

Comment 10

[Johnny Stenback (:jst)]

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

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 12

[Johnny Stenback (:jst)]

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

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well then, you could make it

ok = ok & JS_...

or if you want to be really sure:

ok = !!ok & !!JS_...

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Final patch

With sneakiness comment.

Comment 15

[Brendan Eich [:brendan]]

(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 16

[Johnny Stenback (:jst)]

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.

Comment 17

[Robert Sayre]

(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.

Comment 18

[Johnny Stenback (:jst)]

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.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 20

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/17842a2d0c7f

Comment 21

[Shawn Wilsher :sdwilsh]

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

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset b4bbb3c351a6 to mozilla-central.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset a856ffc040a2 to mozilla-1.9.1.