771927 - [OS.File] (de)serialize ArrayBuffer, C arrays, C pointers, Strings

Status: RESOLVED FIXED

(Core :: Networking: File, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

To implement a multi-threaded version of OS.File, we need to be able to communicate a number of data structures between threads, which is generally not supported by the js-ctypes + chrome worker combo.

In particular, we need to send C arrays and C pointers.

Also, the semantics of communicating ArrayBuffer between threads does not allow MT implementations of |read(buffer, bytes)|.

Basically, we need to implement communication of everything that maps to a pointer in C.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Communicating pointers

After our discussion on bug, here is an implementation of pointer (de)serialization that does not rely upon offsetBy.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 645886 [details] [diff] [review]
Communicating pointers

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

I think the serialization seems more-or-less reasonable, but there are other issues with the patch that need to be addressed before it's in a reviewable state.

::: toolkit/components/osfile/osfile_shared.jsm
@@ +6,5 @@
>    if (typeof Components != "undefined") {
> +    var EXPORTED_SYMBOLS = ["OS"];
> +    Components.utils.import("resource://gre/modules/ctypes.jsm");
> +    Components.classes["@mozilla.org/net/osfileconstantsservice;1"].
> +      getService(Components.interfaces.nsIOSFileConstantsService).init();

Did you mean to change this stuff in this bug?  I see serialization, but I don't see uses of the serialization that would imply it's now safe to use osfile_shared.jsm on the main thread.

@@ +225,5 @@
> +           return null;
> +         } else if (typeof value == "string") {
> +           return { string: value };
> +         } else if ("byteLength" in value) { // ArrayBuffer
> +           value = exports.OS.Shared.Utils.Pointers.offsetBy(value, 0);

We WONTFIX'd the bug for providing offsetBy, so this doesn't seem correct.

@@ +247,5 @@
> +           return null;
> +         } else if ("string" in msg) {
> +           return msg.string;
> +         } else if ("ptr" in msg) {
> +           let address = ctypes.uintptr_t(msg.ptr);

If I'm reading CTypes.cpp correctly, this will implicitly convert the string to an integer; is that correct?  Will that do the right thing on 64-bit platforms with large pointer values?

In other words, does this correctly handle pointer addresses that would have issues being represented as a double?

@@ +812,1 @@
>        Pointers.offsetBy = declareFFI(libxul, "osfile_OffsetBy",

Did you submit this patch from a properly rebased tree?

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 645887 [details] [diff] [review]
Companion testsuite

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js
@@ +49,5 @@
> +      }},
> +    { typename: "OS.Shared.Type.char.in_ptr",
> +      valuedescr: "ArrayBuffer",
> +      value: (function() {
> +                let buf = new ArrayBuffer(15);

Please stick the 15 somewhere common and use that variable instead throughout samples.

@@ +74,5 @@
> +        if (ctypes.voidptr_t.size == 8) {
> +          address = ctypes.cast(candidate, ctypes.uint64_t).value.toString();
> +        } else {
> +          address = ctypes.cast(candidate, ctypes.uint32_t).value.toString();
> +        }

Is the uintptr_t-is-always-64-bit wart the reason you do explicit checking here?

@@ +96,5 @@
> +        }
> +      }
> +    }
> +  ];
> +  for each(let sample in samples) {

MDN says don't do this:

https://developer.mozilla.org/en/JavaScript/Reference/Statements/for_each...in#Example:_Using_for_each...in

Use samples.forEach instead.

@@ +148,5 @@
> +     + " aka " + JSON.stringify(serialized));
> +
> +    // 5. Test that serialization does not hurt
> +    // (which is not true when we serialize an ArrayBuffer without the
> +    // offsetBy trick)

Why is this different than test 2?  I don't understand what the comment is attempting to explain.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 645887 [details] [diff] [review]
> Companion testsuite
> 
> Review of attachment 645887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js
> @@ +49,5 @@
> > +      }},
> > +    { typename: "OS.Shared.Type.char.in_ptr",
> > +      valuedescr: "ArrayBuffer",
> > +      value: (function() {
> > +                let buf = new ArrayBuffer(15);
> 
> Please stick the 15 somewhere common and use that variable instead
> throughout samples.

Can't do that. I am sending |value| as source between threads, so it cannot be a closure.

> 
> @@ +74,5 @@
> > +        if (ctypes.voidptr_t.size == 8) {
> > +          address = ctypes.cast(candidate, ctypes.uint64_t).value.toString();
> > +        } else {
> > +          address = ctypes.cast(candidate, ctypes.uint32_t).value.toString();
> > +        }
> 
> Is the uintptr_t-is-always-64-bit wart the reason you do explicit checking
> here?

No, you are right, I should just use uintptr_t.

> 
> @@ +96,5 @@
> > +        }
> > +      }
> > +    }
> > +  ];
> > +  for each(let sample in samples) {
> 
> MDN says don't do this:
> 
> https://developer.mozilla.org/en/JavaScript/Reference/Statements/for_each...
> in#Example:_Using_for_each...in
> 
> Use samples.forEach instead.

I took that shortcut for a test, but you are right, I should have used forEach.

> 
> @@ +148,5 @@
> > +     + " aka " + JSON.stringify(serialized));
> > +
> > +    // 5. Test that serialization does not hurt
> > +    // (which is not true when we serialize an ArrayBuffer without the
> > +    // offsetBy trick)
> 
> Why is this different than test 2?  I don't understand what the comment is
> attempting to explain.

ArrayBuffer are special wrt thread communications. Sending an ArrayBuffer from thread A to thread B sends the pointer and removes ownership, so after the communication thread A cannot access the ArrayBuffer anymore. I have added this test to explode in case we somehow reproduce such behavior. Say if someone attempts to refactor the implementation of |toMsg| erroneously at some point.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> Comment on attachment 645886 [details] [diff] [review]
> Communicating pointers
> 
> Review of attachment 645886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the serialization seems more-or-less reasonable, but there are other
> issues with the patch that need to be addressed before it's in a reviewable
> state.
> 
> ::: toolkit/components/osfile/osfile_shared.jsm
> @@ +6,5 @@
> >    if (typeof Components != "undefined") {
> > +    var EXPORTED_SYMBOLS = ["OS"];
> > +    Components.utils.import("resource://gre/modules/ctypes.jsm");
> > +    Components.classes["@mozilla.org/net/osfileconstantsservice;1"].
> > +      getService(Components.interfaces.nsIOSFileConstantsService).init();
> 
> Did you mean to change this stuff in this bug?  I see serialization, but I
> don't see uses of the serialization that would imply it's now safe to use
> osfile_shared.jsm on the main thread.
> 
> @@ +225,5 @@
> > +           return null;
> > +         } else if (typeof value == "string") {
> > +           return { string: value };
> > +         } else if ("byteLength" in value) { // ArrayBuffer
> > +           value = exports.OS.Shared.Utils.Pointers.offsetBy(value, 0);
> 
> We WONTFIX'd the bug for providing offsetBy, so this doesn't seem correct.
> 
> @@ +247,5 @@
> > +           return null;
> > +         } else if ("string" in msg) {
> > +           return msg.string;
> > +         } else if ("ptr" in msg) {
> > +           let address = ctypes.uintptr_t(msg.ptr);
> 
> If I'm reading CTypes.cpp correctly, this will implicitly convert the string
> to an integer; is that correct?  Will that do the right thing on 64-bit
> platforms with large pointer values?
> 
> In other words, does this correctly handle pointer addresses that would have
> issues being represented as a double?
> 
> @@ +812,1 @@
> >        Pointers.offsetBy = declareFFI(libxul, "osfile_OffsetBy",
> 
> Did you submit this patch from a properly rebased tree?

Sorry, wrong patch. Uploading the correct patch immediately.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Communicating pointers (the right patch)

My apologies for the confusion.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #5)
> > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js
> > @@ +49,5 @@
> > > +      }},
> > > +    { typename: "OS.Shared.Type.char.in_ptr",
> > > +      valuedescr: "ArrayBuffer",
> > > +      value: (function() {
> > > +                let buf = new ArrayBuffer(15);
> > 
> > Please stick the 15 somewhere common and use that variable instead
> > throughout samples.
> 
> Can't do that. I am sending |value| as source between threads, so it cannot
> be a closure.

Ugh.  Technically, that only matters for |check|, though, right?  The things we put up with...

> > @@ +148,5 @@
> > > +     + " aka " + JSON.stringify(serialized));
> > > +
> > > +    // 5. Test that serialization does not hurt
> > > +    // (which is not true when we serialize an ArrayBuffer without the
> > > +    // offsetBy trick)
> > 
> > Why is this different than test 2?  I don't understand what the comment is
> > attempting to explain.
> 
> ArrayBuffer are special wrt thread communications. Sending an ArrayBuffer
> from thread A to thread B sends the pointer and removes ownership, so after
> the communication thread A cannot access the ArrayBuffer anymore. I have
> added this test to explode in case we somehow reproduce such behavior. Say
> if someone attempts to refactor the implementation of |toMsg| erroneously at
> some point.

Where is the ownership passing described?  Is that something implemented in the guts of the JS engine?

It's not clear to me how that would actually get *enforced*, given that you're effectively passing pointers around, not actual JS objects.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 646138 [details] [diff] [review]
Communicating pointers (the right patch)

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

I don't see osfile_shared_allthreads.jsm in my source tree; is this something recent, or is it resulting from rearrangement of not-yet-committed patches lurking in other bugs?

Please answer questions in comment 8 first.  Bonus points if you address the ownership details in comment 10.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> Comment on attachment 645886 [details] [diff] [review]
> Communicating pointers
> 
> Review of attachment 645886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the serialization seems more-or-less reasonable, but there are other
> issues with the patch that need to be addressed before it's in a reviewable
> state.
> 
> ::: toolkit/components/osfile/osfile_shared.jsm
> @@ +6,5 @@
> >    if (typeof Components != "undefined") {
> > +    var EXPORTED_SYMBOLS = ["OS"];
> > +    Components.utils.import("resource://gre/modules/ctypes.jsm");
> > +    Components.classes["@mozilla.org/net/osfileconstantsservice;1"].
> > +      getService(Components.interfaces.nsIOSFileConstantsService).init();
> 
> Did you mean to change this stuff in this bug?  I see serialization, but I
> don't see uses of the serialization that would imply it's now safe to use
> osfile_shared.jsm on the main thread.

Well, osfile_shared.jsm has always been basically safe for use in the main thread. However, this was useless until now - I need it in particular for the testsuite.

> @@ +225,5 @@
> > +           return null;
> > +         } else if (typeof value == "string") {
> > +           return { string: value };
> > +         } else if ("byteLength" in value) { // ArrayBuffer
> > +           value = exports.OS.Shared.Utils.Pointers.offsetBy(value, 0);
> 
> We WONTFIX'd the bug for providing offsetBy, so this doesn't seem correct.

Sorry about that :/

> @@ +247,5 @@
> > +           return null;
> > +         } else if ("string" in msg) {
> > +           return msg.string;
> > +         } else if ("ptr" in msg) {
> > +           let address = ctypes.uintptr_t(msg.ptr);
> 
> If I'm reading CTypes.cpp correctly, this will implicitly convert the string
> to an integer; is that correct?  Will that do the right thing on 64-bit
> platforms with large pointer values?
> 
> In other words, does this correctly handle pointer addresses that would have
> issues being represented as a double?

Pointers (and uintptr_t) have a 64-bit representation on 64-bit platforms, so no worry here.

> 
> @@ +812,1 @@
> >        Pointers.offsetBy = declareFFI(libxul, "osfile_OffsetBy",
> 
> Did you submit this patch from a properly rebased tree?

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #10)
> > Can't do that. I am sending |value| as source between threads, so it cannot
> > be a closure.
> 
> Ugh.  Technically, that only matters for |check|, though, right?  The things
> we put up with...

Yes, I meant |check|, not |value|. If you really want me to change this, I can split the test in three files instead of two, with 1/ the main thread driver 2/ the worker thread code 3/ the set of samples, included by both files. I suspect that, for a test, this is not necessary, though.

> 
> > > @@ +148,5 @@
> > > > +     + " aka " + JSON.stringify(serialized));
> > > > +
> > > > +    // 5. Test that serialization does not hurt
> > > > +    // (which is not true when we serialize an ArrayBuffer without the
> > > > +    // offsetBy trick)
> > > 
> > > Why is this different than test 2?  I don't understand what the comment is
> > > attempting to explain.
> > 
> > ArrayBuffer are special wrt thread communications. Sending an ArrayBuffer
> > from thread A to thread B sends the pointer and removes ownership, so after
> > the communication thread A cannot access the ArrayBuffer anymore. I have
> > added this test to explode in case we somehow reproduce such behavior. Say
> > if someone attempts to refactor the implementation of |toMsg| erroneously at
> > some point.
> 
> Where is the ownership passing described?  Is that something implemented in
> the guts of the JS engine?

I could not find a description, but this was explained to me by the JSAPI team (either Jason or Blake, I'm not sure): ArrayBuffer have a zero-copy, no-sharing policy.

> It's not clear to me how that would actually get *enforced*, given that
> you're effectively passing pointers around, not actual JS objects.

I am not sure I understand the question. Perhaps the issue is just that I left |offsetBy| in the comment, while I should have updated the comment to something along the lines of "convert the ArrayBuffer to a C pointer". Is that it?

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #12)
> > @@ +247,5 @@
> > > +           return null;
> > > +         } else if ("string" in msg) {
> > > +           return msg.string;
> > > +         } else if ("ptr" in msg) {
> > > +           let address = ctypes.uintptr_t(msg.ptr);
> > 
> > If I'm reading CTypes.cpp correctly, this will implicitly convert the string
> > to an integer; is that correct?  Will that do the right thing on 64-bit
> > platforms with large pointer values?
> > 
> > In other words, does this correctly handle pointer addresses that would have
> > issues being represented as a double?
> 
> Pointers (and uintptr_t) have a 64-bit representation on 64-bit platforms,
> so no worry here.

Well, sure.  Let's say we have a pointer whose address is X.  X is converted to a string to send it across threads.  This string is then passed to |ctypes.uintptr_t|, which, I suppose, parses it into a number--is that parsing smart enough to handle it as a 64-bit number and not as a JS double?  e.g. the address:

8382839232580043227

is not exactly representable as a double.  We have {U,}Int64 bits in the JS for exactly this sort of situation in a different context.

If ctypes already gets this right--and it shouldn't be hard to write a test for this, that would be worth including--then hooray!  But if it doesn't, it should be addressed.

Granted, the likelihood of this mattering is up for debate.  (I'm not even sure that the 64-bit platforms we support--even including Win64--would ever produce pointers with addresses that hit this bug.)  But I think it's worth making sure the serialization process gets it right for round-trip sanity, if nothing else.

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #13)
> Yes, I meant |check|, not |value|. If you really want me to change this, I
> can split the test in three files instead of two, with 1/ the main thread
> driver 2/ the worker thread code 3/ the set of samples, included by both
> files. I suspect that, for a test, this is not necessary, though.

You're right, don't bother shuffling things around.

> > > ArrayBuffer are special wrt thread communications. Sending an ArrayBuffer
> > > from thread A to thread B sends the pointer and removes ownership, so after
> > > the communication thread A cannot access the ArrayBuffer anymore. I have
> > > added this test to explode in case we somehow reproduce such behavior. Say
> > > if someone attempts to refactor the implementation of |toMsg| erroneously at
> > > some point.
> > 
> > Where is the ownership passing described?  Is that something implemented in
> > the guts of the JS engine?
> 
> I could not find a description, but this was explained to me by the JSAPI
> team (either Jason or Blake, I'm not sure): ArrayBuffer have a zero-copy,
> no-sharing policy.
> 
> > It's not clear to me how that would actually get *enforced*, given that
> > you're effectively passing pointers around, not actual JS objects.
> 
> I am not sure I understand the question. Perhaps the issue is just that I
> left |offsetBy| in the comment, while I should have updated the comment to
> something along the lines of "convert the ArrayBuffer to a C pointer". Is
> that it?

Well, even if the comment said "convert the ArrayBuffer to a C pointer", I don't see where the test does that or how the test is materially different from case 2.

The ownership semantics in this case is a convention: thread A is telling thread B the address of some buffer X and then thread A is not supposed to touch X anymore, because thead B now owns X.  (It's not actually enforced; real enforcement would mean that A doesn't even have access to X.)  But the test isn't actually testing that, except by accident (the worker thread doesn't touch X anymore in this case).  Indeed, you can't really test it.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #14)
> > > In other words, does this correctly handle pointer addresses that would have
> > > issues being represented as a double?
> > 
> > Pointers (and uintptr_t) have a 64-bit representation on 64-bit platforms,
> > so no worry here.
> 
> Well, sure.  Let's say we have a pointer whose address is X.  X is converted
> to a string to send it across threads.  This string is then passed to
> |ctypes.uintptr_t|, which, I suppose, parses it into a number--is that
> parsing smart enough to handle it as a 64-bit number and not as a JS double?
> e.g. the address:
> 
> 8382839232580043227
> 
> is not exactly representable as a double.  We have {U,}Int64 bits in the JS
> for exactly this sort of situation in a different context.

Since ctypes.uintptr_t(...).value is precisely a UInt64 on a 64-bit platforms, I don't really worry about that. If UInt64 parsing does not work, well, js-ctypes has big issues :)
(plus I have quick-checked a few weeks ago and it seemed to work)

> If ctypes already gets this right--and it shouldn't be hard to write a test
> for this, that would be worth including--then hooray!  But if it doesn't, it
> should be addressed.
> 
> Granted, the likelihood of this mattering is up for debate.  (I'm not even
> sure that the 64-bit platforms we support--even including Win64--would ever
> produce pointers with addresses that hit this bug.)  But I think it's worth
> making sure the serialization process gets it right for round-trip sanity,
> if nothing else.

Well, I can open a js-ctypes bug on that topic, if you want.

> (In reply to David Rajchenbach Teller (away until early August) [:Yoric]
> from comment #13)
> > Yes, I meant |check|, not |value|. If you really want me to change this, I
> > can split the test in three files instead of two, with 1/ the main thread
> > driver 2/ the worker thread code 3/ the set of samples, included by both
> > files. I suspect that, for a test, this is not necessary, though.
> 
> You're right, don't bother shuffling things around.

ok

> > > It's not clear to me how that would actually get *enforced*, given that
> > > you're effectively passing pointers around, not actual JS objects.
> > 
> > I am not sure I understand the question. Perhaps the issue is just that I
> > left |offsetBy| in the comment, while I should have updated the comment to
> > something along the lines of "convert the ArrayBuffer to a C pointer". Is
> > that it?
> 
> Well, even if the comment said "convert the ArrayBuffer to a C pointer", I
> don't see where the test does that or how the test is materially different
> from case 2.
> 
> The ownership semantics in this case is a convention: thread A is telling
> thread B the address of some buffer X and then thread A is not supposed to
> touch X anymore, because thead B now owns X.  (It's not actually enforced;
> real enforcement would mean that A doesn't even have access to X.)  But the
> test isn't actually testing that, except by accident (the worker thread
> doesn't touch X anymore in this case).  Indeed, you can't really test it.

I have checked with an additional sample (an ArrayBuffer sent without toMsg/fromMsg), and it seems that that the test is indeed insufficient. I suspect that the message sent at step 4 between threads has not been actually sent by the time we hit step 5, and that consequently, the ArrayBuffer has not been "neutered" yet (term comes from comments in ArrayBuffer implementation, I do not know how official it is).

So, I guess I will just remove that test.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> Comment on attachment 646138 [details] [diff] [review]
> Communicating pointers (the right patch)
> 
> Review of attachment 646138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see osfile_shared_allthreads.jsm in my source tree; is this
> something recent, or is it resulting from rearrangement of not-yet-committed
> patches lurking in other bugs?

Yes, a not-landed-yet patch renames osfile_shared.jsm to osfile_shared_allthreads.jsm .

> Please answer questions in comment 8 first.  Bonus points if you address the
> ownership details in comment 10.

I hope that I have now answered all your questions. I'm eager to move to the following patches :)

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #15)
> > Well, sure.  Let's say we have a pointer whose address is X.  X is converted
> > to a string to send it across threads.  This string is then passed to
> > |ctypes.uintptr_t|, which, I suppose, parses it into a number--is that
> > parsing smart enough to handle it as a 64-bit number and not as a JS double?
> > e.g. the address:
> > 
> > 8382839232580043227
> > 
> > is not exactly representable as a double.  We have {U,}Int64 bits in the JS
> > for exactly this sort of situation in a different context.
> 
> Since ctypes.uintptr_t(...).value is precisely a UInt64 on a 64-bit
> platforms, I don't really worry about that. If UInt64 parsing does not work,
> well, js-ctypes has big issues :)
> (plus I have quick-checked a few weeks ago and it seemed to work)

OK, that's what I wanted to know. :)  I see jsvalToBigInteger in CTypes.cpp.  I don't see exactly how that gets mapped to the uintptr_t constructor, but I will take your word for it.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 646138 [details] [diff] [review]
Communicating pointers (the right patch)

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

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +6,5 @@
>    if (typeof Components != "undefined") {
> +    var EXPORTED_SYMBOLS = ["OS"];
> +    Components.utils.import("resource://gre/modules/ctypes.jsm");
> +    Components.classes["@mozilla.org/net/osfileconstantsservice;1"].
> +      getService(Components.interfaces.nsIOSFileConstantsService).init();

It lives!

@@ +220,5 @@
> +      */
> +     PtrType.prototype.toMsg = function ptr_toMsg(value) {
> +         if (value == null) {
> +           return null;
> +         } else if (typeof value == "string") {

Please write the first two cases thusly (no else after returning):

if (value == null) {
  return null;
}
if (typeof value == "string") {
  return { string: value };
}

Also introduce a temporary variable for the pointer address in the next several cases rather than reusing |value|.

@@ +242,5 @@
> +      */
> +     PtrType.prototype.fromMsg = function ptr_fromMsg(msg) {
> +         if (msg == null) {
> +           return null;
> +         } else if ("string" in msg) {

...and also here, no elses after returns.

@@ +819,5 @@
>       };
>  
> +     exports.OS.Shared.Utils = { Strings: Strings };
> +
> +

Extra whitespace.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: Communicating pointers

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=87960bb79cd4

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: Communicating pointers

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Rajchenbach Teller from comment #21)
> Try run: https://tbpl.mozilla.org/?tree=Try&rev=87960bb79cd4

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/11372010763f
https://hg.mozilla.org/integration/mozilla-inbound/rev/625e7031ee59

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/11372010763f
https://hg.mozilla.org/mozilla-central/rev/625e7031ee59