770778 - e10s support for JS TCP Socket

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jason Duell]

I can probably take this, or at least mentor/review someone else.

Note that for now I suspect the initial non-e10s-specific JS code in bug 733573 may seem to "just work" under e10s, as we're still running the socket transport thread in child processes (see bug 607924).   That's definitely not a long-term fix, but I wanted to toss it out there in case it's good enough for initial B2G purposes.   Note that TLS would not work (unless we fired up NSS in the child--bad idea from memory consumption angle and probably other reasons), so I'm guessing the answer is "no".

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

B2G's use-case for the JS TCP socket is e-mail, and SSL/TLS is indeed required.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Nominating for blocking-basecamp since it would probably suck for the system if the e-mail app could cause jank in the phone proper.  The TCP socket implementation itself is already a blocker (and blocks this bug).

Comment 3

[Andrew Overholt [:overholt]]

I'm taking a stab at an owner with Donovan but comment #1 seems to indicate Jason may also be a suitable assignee.  Please shuffle that if Donovan is not the right person to own this.  Thanks.

Comment 4

[Jason Duell]

Donovan, I can give you guidance on the architecture here when I get a moment.  I may also be able to take this, but it's not next on my list, so go for it if you have cycles.

This could possibly be fairly straightforward--lots of code, but mostly IPDL boilerplate.  But I'm marking 'M' as it's slightly different than our previous channel IPDL stuff, so there's some risk there.  Fortunately it will be easy to lock down the IPDL protocol here, as we'll always have a DOM window and nsILoadContext.

Comment 5

[Donovan Preston [:fzzzy]]

Hi Jason. Faramarz would really like for me to help with some gaia stuff now, so if you can get to it that would be great. If not, I can get to it eventually. Mail is the only thing that needs TCPSocket, so it may be ok if it does not run OOP for version 1.

Are there any docs or examples elsewhere for the code that will be required to bridge to OOP?

I just tested to see if TCPSocket magically just works OOP, but it does not appear to. I get a white screen and no error messages in the jsconsole or in the log.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Donovan Preston from comment #5)
> Hi Jason. Faramarz would really like for me to help with some gaia stuff
> now, so if you can get to it that would be great. If not, I can get to it
> eventually. Mail is the only thing that needs TCPSocket, so it may be ok if
> it does not run OOP for version 1.
> 

Absolutely not.  The mail app will be a memory hog and it loads third-party, untrusted code in <iframe>.  We can't do that as root.

We need a plan here.

Comment 7

[Andreas Gal :gal]

Jason can you take this?

Comment 8

[Josh Matthews [:jdm]]

Do we require the use of IPDL here, or can we use the message manager? It's going to be a bit obnoxious to switch back and forth from JS to C++.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We can easily use IPDL behind an XPCOM interface.

This will be somewhat perf sensitive.  I know how to efficiently get typed array data from JS objects in C++ and efficiently send them through IPDL.

I don't know if we can do that with mm + structured clone.  bent/smaug, thoughts?

Comment 10

[Andreas Gal :gal]

Structured cloning uses memcpy on the typed array data, so minus the copies its reasonably fast.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK.  So we'd pay an extra copy compared to C++, IIUC.  That's probably fine.

Comment 12

[Josh Matthews [:jdm]]

The obnoxious part is calling the event handlers and ensuring the JSAPI usage and principals are correct.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Which event handlers? There are no DOM event handlers in the current implementation if I read the code
correctly. But there are sure some JS callbacks. AFAIK calling those from chrome JS works ok
(but would be better to ask mrbkap or bholley).

Comment 14

[Josh Matthews [:jdm]]

Assuming we're going to use the MM here (which I think is a very sensible idea), we're going to need bug 776832 for the permissions checks. Here are some notes I wrote up while reading through the code:

get docshell from provided window; mm from docshell->getInterface(nsIContentFrameMessageManager)

in parent:
* create TCPSocket listener object that holds full TCPSocket object and receives messages from child (part of open call, probably)
* parent sends update message to keep readyState in child up to date (check readyState before making TCPSocket call, send update after if changed)

open:
* observer needs to be in child
* transport needs to be in parent
* send async open: host, port, options to parent

close:
* always send close message to parent, regardless of readystate

send:
* sync operation that returns waitingForDrain and parent send return value
* send data to parent; don't process at all in child

suspend/resume:
* send async to parent, don't keep track in child

onTransportStatus/onInputStreamReady/onStartRequest/onStopRequest/onDataAvailable:
* only triggered in parent, trigger listener which forwards to child

all callbacks:
* send async callback message: type, data to child

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This work doesn't hard-block on 776832, though it would obviously be nice to have.  We currently don't apply checks for even telephony (!!!!), because we're missing that capability.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Josh, can you take this?  If so, please assign to yourself.  Thanks! :)

Comment 17

[Josh Matthews [:jdm]]

Sure.

Comment 18

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

I think it's fine to use the message manager here. If someone please otherwise, please clarify here. Otherwise I think we should use the message manager to avoid doing extra looping between C++ and JS which will introduce extra complexity.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Ah, hmm, TCP stuff is oddly enough implemented in JS, so perhaps using MM doesn't make the situation
much worse. There will be just possibly lots of JS garbage both in parent and child.

Comment 20

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

Indeed. Though no more because of the MM usage since even if we went through ipdl we'd generate ArrayBuffer objects in both processes.

Comment 21

[Josh Matthews [:jdm]]

Well shit. I just completed a first pass using IPDL and a JS intermediary in the parent. Should I completely switch gears or just continue doing what I'm doing? The JS side gets complicated because we don't have a message manager per socket, whereas IPDL hides that complexity for free.

Comment 22

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

It seems to me that sticking with the MM has some disadvantages, though pretty small ones. So depends on how much complexity using IPDL introduces. Up to you.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #20)
> Indeed. Though no more because of the MM usage since even if we went through
> ipdl we'd generate ArrayBuffer objects in both processes.
Why? If TCPSocket was implemented in C++, we'd need ArrayBuffer only in the process which
actually uses nsITCPSocket. If child would use it, parent sure shouldn't handle ArrayBuffers but
just plain data and forward it to child.

Comment 24

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

This patch makes test_tcpsocket.js pass when run in a content process. It still needs some cleanup and documentation work.

Comment 25

[Josh Matthews [:jdm]]

In particular, I have no idea what principle ends up being used when calling the JS callbacks in the content process. I'm doing a straight call into the XPCOM interface I defined, so maybe it ends up with the same principle as when the object was created?

Comment 26

[Josh Matthews [:jdm]]

Humph. Looks like I need to obtain a device to test this out in anger, since bug 782845 is striking my desktop builds.

Comment 27

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

(In reply to Olli Pettay [:smaug] from comment #23)
> (In reply to Jonas Sicking (:sicking) from comment #20)
> > Indeed. Though no more because of the MM usage since even if we went through
> > ipdl we'd generate ArrayBuffer objects in both processes.
> Why? If TCPSocket was implemented in C++, we'd need ArrayBuffer only in the
> process which actually uses nsITCPSocket.

Sure. But TCPSocket isn't implemented in C++.

Comment 28

[Josh Matthews [:jdm]]

Woo! Running the email app OOP in today's gaia/m-c tips, everything works perfectly - fetching my mozilla.com email and sending from it were both successful. I'm going to clean this patch up and hit somebody up for review.

Comment 29

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

Cleaned up, documented, and tested thoroughly. I have one important question I would like answered by someone in the know - is it safe for me to call into the JS objects in the child (through nsITCPSocketInternal), or will that end up executing content code with non-content privileges?

Comment 30

[Josh Matthews [:jdm]]

One thing to note is that permission checks in the chrome process are currently missing. I need to look into what's available from C++.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If you're in C++, you want http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessPermissions.h .

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Review ping.

Comment 33

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

Comment on attachment 658560 [details] [diff] [review]
Make TCPSocket e10s-friendly.

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

This looks really good! There's a few big-ish things here, and some smaller issues, but I think this is going to work well.

::: dom/network/src/PTCPSocket.ipdl
@@ +12,5 @@
> +
> +using mozilla::void_t;
> +using IPC::Uint8Array;
> +
> +struct Error {

This is a pretty generic name, it should be in the net namespace below.

@@ +20,5 @@
> +  uint32_t columnNumber;
> +};
> +
> +union SendableData {
> +  Uint8Array;

Hm, rather than invent your own type (and have to write a custom serializer), could you just use 'uint8_t[]' instead?

@@ +47,5 @@
> +  __delete__();
> +
> +child:
> +  OnCallback(nsString type, CallbackData data,
> +             nsString readyState, uint32_t bufferedAmount);

So is there some higher-level thing guaranteeing that this message (sent from parent to child) can't race with the __delete__ (sent from child to parent)? Otherwise we'll need to add another round trip to make it safe.

Nit: Also, could you rename this to just 'Callback'? The cpp is awkward ("SendOnCallback", "RecvOnCallback")...

::: dom/network/src/TCPMessageUtils.h
@@ +6,5 @@
> +#define mozilla_dom_network_TCPMessageUtils_h
> +
> +#include "ipc/IPCMessageUtils.h"
> +#include "jsapi.h"
> +#include "jsfriendapi.h"

You don't need this here do you?

@@ +25,5 @@
> +  uint32_t mNumBytes;
> +};
> +
> +extern bool
> +DeserializeUint8Array(JSContext* aCx,

Nit: Add a comment saying which file you implement this in.

@@ +27,5 @@
> +
> +extern bool
> +DeserializeUint8Array(JSContext* aCx,
> +                       const Uint8Array& aBuffer,
> +                       jsval* aVal);

Nit: Your indent is off here.

::: dom/network/src/TCPSocket.js
@@ +150,5 @@
>  
>    _asyncCopierActive: false,
>    _waitingForDrain: false,
>    _suspendCount: 0,
> +  

Nit: trailing whitespace here and several other places in this file.

@@ +173,5 @@
>    },
>    get bufferedAmount() {
> +    if (this._inChild) {
> +      return this._bufferedAmount;
> +    } {

Nit: No need for this extra set of braces

@@ +342,5 @@
>  
>    // nsIDOMTCPSocket
>    open: function ts_open(host, port, options) {
> +    this._inChild = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
> +                       .processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;

Hm, I wonder if you should do != PROCESS_TYPE_DEFAULT here instead.

@@ +377,5 @@
>  
>      LOG("SSL: " + that.ssl + "\n");
> +    
> +    if (this._inChild) {
> +      that._socketBridge = Cc["@mozilla.org/tcp-socket-child;1"]

You didn't add |_socketBridge| to the prototype (as null). Probably should?

@@ +379,5 @@
> +    
> +    if (this._inChild) {
> +      that._socketBridge = Cc["@mozilla.org/tcp-socket-child;1"]
> +                             .createInstance(Ci.nsITCPSocketChild);
> +      that._socketBridge.open(that, host, port, that._ssl != false, that._binaryType);

Nit: |!!that._ssl| maybe?

::: dom/network/src/TCPSocketChild.cpp
@@ +15,5 @@
> +DeserializeUint8Array(JSContext* aCx,
> +                      const Uint8Array& aBuffer,
> +                      jsval* aVal)
> +{
> +  JSAutoRequest ar(aCx);

Hm, any time you need a request you should probably also enter a compartment, but to do so you'll need a JSObject. I wonder if this is needed at all.

@@ +24,5 @@
> +  uint8_t* data = JS_GetArrayBufferData(obj, aCx);
> +  if (!data)
> +    return false;
> +  memcpy(data, aBuffer.mBuffer, aBuffer.mNumBytes);
> +  JSObject* arr = JS_NewUint8ArrayWithBuffer(aCx, obj, 0, aBuffer.mNumBytes);

This can fail and you should return false if it does.

@@ +37,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_1(TCPSocketChild, mSocket)
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TCPSocketChild)
> +
> +NS_IMETHODIMP_(nsrefcnt) TCPSocketChild::Release(void)

Hm, unrolling these macros is always kind of dangerous (hard to know if something breaks when the macros change). How about this:

You could make a small base class that holds your cycle collected members

  class TCPSocketChildBase : public nsISupports
  {
  public:
    NS_DECL_CYCLE_COLLECTION_CLASS(TCPSocketChildBase)
    NS_DECL_CYCLE_COLLECTING_ISUPPORTS

  protected:
    TCPSocketChildBase();
    void ReleaseIPDLReference();
    void AddIPDLReference();

    nsCOMPtr<nsITCPSocketInternal> mSocket;
  };

Then you just use NS_DECL_ISUPPORTS_INHERITED in TCPSocketChild:

  class TCPSocketChild : public TCPSocketChildBase,
                         public mozilla::net::PTCPSocketChild,
                         public nsITCPSocketChild
  {
  public:
    NS_DECL_ISUPPORTS_INHERITED
    NS_DECL_NSITCPSOCKETCHILD

    // ...
  };

Then, override TCPSocketChild::Release:

  NS_IMETHODIMP_(nsrefcnt) TCPSocketChild::Release()
  {
    nsrefcnt refcnt = TCPSocketChildBase::Release();
    if (refcnt == 1 && mIPCOpen) {
      // ...
      return 0;
    }
    return refcnt;
  }

Make sense?

@@ +65,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +TCPSocketChild::TCPSocketChild()
> +: mCx(NULL), mIPCOpen(false)

nullptr

@@ +74,5 @@
> +TCPSocketChild::Open(nsITCPSocketInternal* aSocket, const nsAString& aHost,
> +                     uint16_t aPort, bool aUseSSL, const nsAString& aBinaryType,
> +                     JSContext* aCx)
> +{
> +  mCx = aCx;

Hm, I'm not sure this is ok. If this context is coming from a window, for instance, what guarantees that this context will stay alive as long as you use it?

@@ +77,5 @@
> +{
> +  mCx = aCx;
> +  mSocket = aSocket;
> +  AddIPDLReference();
> +  net::gNeckoChild->SendPTCPSocketConstructor(this, nsString(aHost), aPort,

Can you do some sort of 'using' at the top so you don't have to specify 'net::' here? Otherwise please use 'mozilla::net::'.

@@ +85,5 @@
> +
> +void
> +TCPSocketChild::ReleaseIPDLReference()
> +{
> +  mIPCOpen = false;

Please assert that mIPCOpen is true before modifying.

@@ +92,5 @@
> +
> +void
> +TCPSocketChild::AddIPDLReference()
> +{
> +  mIPCOpen = true;

Please assert that mIPCOpen is false before modifying.

@@ +103,5 @@
> +                               const nsString& aReadyState,
> +                               const uint32_t& aBuffered)
> +{
> +  nsresult rv = mSocket->UpdateReadyStateAndBuffered(aReadyState, aBuffered);
> +  NS_ENSURE_SUCCESS(rv, true);

Hm, as I read it this should never fail. How about:

  if (NS_FAILED(mSocket->UpdateReadyStateAndBuffered(...))) {
    NS_ERROR("Shouldn't fail!");
  }

@@ +129,5 @@
> +      MOZ_NOT_REACHED();
> +    }
> +
> +  } else {
> +    MOZ_NOT_REACHED();

Let's be slightly more verbose, "Invalid callback type" perhaps?

@@ +159,5 @@
> +
> +NS_IMETHODIMP
> +TCPSocketChild::Send(const JS::Value& aData, JSContext* aCx)
> +{
> +  JSAutoRequest ar(aCx);

If you're being called by XPConnect you shouldn't need a request here.

::: dom/network/src/TCPSocketChild.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class TCPSocketChild : public net::PTCPSocketChild

I've seen bizarre build failures for not specifying the full base class here, please add 'mozilla::' to that

@@ +28,5 @@
> +  void ReleaseIPDLReference();
> +  void AddIPDLReference();
> +
> +  bool RecvOnCallback(const nsString& aType, const CallbackData& aData,
> +                      const nsString& aReadyState, const uint32_t& aBuffered);

Can you mark this as both virtual and MOZ_OVERRIDE?

::: dom/network/src/TCPSocketParent.cpp
@@ +26,5 @@
> +                      const nsString& aBinaryType)
> +{
> +  nsresult rv;
> +  mIntermediary = do_CreateInstance("@mozilla.org/tcp-socket-intermediary;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, true);

Wait, what happens if this (or open(), below) fails? I think the correct behavior is probably to trigger an error callback on the child object, right? Looks like this will just silently eat the error and the child will sit around waiting for onopen or onerror forever.

@@ +35,5 @@
> +  return true;
> +}
> +
> +NS_IMETHODIMP
> +TCPSocketParent::InitJSContext(JSContext* aCx)

Doesn't look like you ever use mCx... Can you remove this?

@@ +44,5 @@
> +
> +bool
> +TCPSocketParent::RecvSuspend()
> +{
> +  NS_ENSURE_TRUE(!!mSocket, true);

Here and below, you don't need the !!

@@ +69,5 @@
> +  switch (aData.type()) {
> +    case SendableData::TUint8Array: {
> +      const IPC::Uint8Array& buffer = aData.get_Uint8Array();
> +      jsval val;
> +      IPC::DeserializeUint8Array(mCx, buffer, &val);

You could just put |aData.get_Uint8Array()| here instead of using the stack temporary.

@@ +81,5 @@
> +      NS_ENSURE_SUCCESS(rv, true);
> +      break;
> +
> +    default:
> +      return false;

Add MOZ_NOT_REACHED here.

@@ +82,5 @@
> +      break;
> +
> +    default:
> +      return false;
> +  } 

Nit: trailing whitespace

@@ +102,5 @@
> +                              const nsAString& aReadyState, uint32_t aBuffered,
> +                              JSContext* aCx)
> +{
> +  if (!mIPCOpen)
> +    return NS_ERROR_FAILURE;

I'd just return NS_OK here, it seems like this can happen if the child crashes and we should handle it.

Nit: Also, please use braces

@@ +104,5 @@
> +{
> +  if (!mIPCOpen)
> +    return NS_ERROR_FAILURE;
> +
> +  JSAutoRequest ar(aCx);

You're being called by XPConnect, so this shouldn't be needed.

@@ +111,5 @@
> +  if (aDataVal.isString()) {
> +    JSString* jsstr = aDataVal.toString();
> +    nsDependentJSString str;
> +    DebugOnly<bool> ok = str.init(aCx, jsstr);
> +    MOZ_ASSERT(ok);

This can fail so please don't assert that it can't.

@@ +122,5 @@
> +    JSObject* obj = &aDataVal.toObject();
> +    if (JS_IsTypedArrayObject(obj, aCx)) {
> +      NS_ENSURE_TRUE(JS_IsUint8Array(obj, aCx), NS_ERROR_FAILURE);
> +      uint32_t nbytes = JS_GetTypedArrayByteLength(obj, aCx);
> +      uint8_t* buffer = JS_GetUint8ArrayData(obj, aCx);

This can return null in some cases I think.

@@ +131,5 @@
> +      uint32_t lineNumber = 0;
> +      uint32_t columnNumber = 0;
> +
> +      jsval val;
> +      JS_GetProperty(aCx, obj, "message", &val);

All of these JS_GetProperty calls can fail. Please check, we need warnings at least.

@@ +132,5 @@
> +      uint32_t columnNumber = 0;
> +
> +      jsval val;
> +      JS_GetProperty(aCx, obj, "message", &val);
> +      if (JSVAL_IS_STRING(val)) {

What if this isn't a string? Seems like we should assert that it is. And below too.

@@ +134,5 @@
> +      jsval val;
> +      JS_GetProperty(aCx, obj, "message", &val);
> +      if (JSVAL_IS_STRING(val)) {
> +        bool ok = message.init(aCx, JSVAL_TO_STRING(val));
> +        NS_WARN_IF_FALSE(ok, "couldn't initialize string");

This pattern works better:

  if (!message.init(...)) {
    NS_WARNING(...);
  }

@@ +158,5 @@
> +      err.message() = message;
> +      err.filename() = filename;
> +      err.lineNumber() = lineNumber;
> +      err.columnNumber() = columnNumber;
> +      data = err;

You should be able to just do this:

  data = Error(message, filename, ...);

@@ +172,5 @@
> +
> +void
> +TCPSocketParent::ActorDestroy(ActorDestroyReason why)
> +{
> +  mIPCOpen = false;

Please assert mIPCOpen before setting it here.

@@ +173,5 @@
> +void
> +TCPSocketParent::ActorDestroy(ActorDestroyReason why)
> +{
> +  mIPCOpen = false;
> +  mSocket = nullptr;

Shouldn't you call close here?

::: dom/network/src/TCPSocketParent.h
@@ +8,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsIDOMTCPSocket.h"
> +
> +//class nsITCPSocketIntermediary;
> +//class nsIDOMTCPSocket;

Nit: remove these

@@ +14,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class TCPSocketParent : public net::PTCPSocketParent

Same comment about including mozilla::

@@ +29,5 @@
> +            const bool& useSSL, const nsString& aBinaryType);
> +  bool RecvSuspend();
> +  bool RecvResume();
> +  bool RecvClose();
> +  bool RecvData(const SendableData& aData);

Mark these as virtual and MOZ_OVERRIDE

@@ +32,5 @@
> +  bool RecvClose();
> +  bool RecvData(const SendableData& aData);
> +
> +private:
> +  void ActorDestroy(ActorDestroyReason why);

This too.

::: dom/network/src/TCPSocketParentIntermediary.js
@@ +17,5 @@
> +  open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType) {
> +    aParentSide.initJSContext();
> +    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
> +    let socket = this._socket = baseSocket.open(aHost, aPort, {useSSL: aUseSSL,
> +                                                               binaryType: aBinaryType});

Hm. You're jumping straight from createInstance() to open(). It looks like the DOM code usually calls init() first. That will, at least, check some prefs, check permissions, etc. It requires a window in order to call, but I think you should try to do as much security checking as possible here. Maybe you should move checks that don't depend on the window to another function (initWindowless?).

@@ +29,5 @@
> +                                   socket.bufferedAmount);
> +        };
> +      }
> +    );
> +    

Nit: Got several lines that are whitespace-only here.

Comment 34

[Josh Matthews [:jdm]]

>@@ +74,5 @@
>> +TCPSocketChild::Open(nsITCPSocketInternal* aSocket, const nsAString& aHost,
>> +                     uint16_t aPort, bool aUseSSL, const nsAString& aBinaryType,
>> +                     JSContext* aCx)
>> +{
>> +  mCx = aCx;
>
>Hm, I'm not sure this is ok. If this context is coming from a window, for instance, what
>guarantees that this context will stay alive as long as you use it?

I've changed this to take a JSObject pointer to the TCPSocket object as well, now. If I root that somehow for the lifetime of TCPSocketChild, will that be enough to guarantee the context's existence?

>::: dom/network/src/TCPSocketParentIntermediary.js
>@@ +17,5 @@
>> +  open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType) {
>> +    aParentSide.initJSContext();
>> +    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
>> +    let socket = this._socket = baseSocket.open(aHost, aPort, {useSSL: aUseSSL,
>> +                                                               binaryType: aBinaryType});
>
>Hm. You're jumping straight from createInstance() to open(). It looks like the DOM code 
>usually calls init() first. That will, at least, check some prefs, check permissions, etc. 
>It requires a window in order to call, but I think you should try to do as much security 
>checking as possible here. Maybe you should move checks that don't depend on the window to 
>another function (initWindowless?).

C++ permissions check added.

>@@ +35,5 @@
>> +  return true;
>> +}
>> +
>> +NS_IMETHODIMP
>> +TCPSocketParent::InitJSContext(JSContext* aCx)
>
>Doesn't look like you ever use mCx... Can you remove this?

It's used for deserializing.

>::: dom/network/src/TCPSocketChild.cpp
>@@ +15,5 @@
>> +DeserializeUint8Array(JSContext* aCx,
>> +                      const Uint8Array& aBuffer,
>> +                      jsval* aVal)
>> +{
>> +  JSAutoRequest ar(aCx);
>
>Hm, any time you need a request you should probably also enter a compartment, but to do so 
>you'll need a JSObject. I wonder if this is needed at all.

JSObject added, since the request is definitely necessary (the deserialization can trigger lazy initialization of global classes).

>::: dom/network/src/PTCPSocket.ipdl
>@@ +12,5 @@
>> +
>> +using mozilla::void_t;
>> +using IPC::Uint8Array;
>> +
>> +struct Error {
>
>This is a pretty generic name, it should be in the net namespace below.

I had difficulty dealing with it when it was in the namespace, so I just gave it a slightly clearer name instead.

Comment 35

[Josh Matthews [:jdm]]

Attachment: Interdiff

Here's the diff of my changes to address your comments.

Comment 36

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

(In reply to Josh Matthews [:jdm] from comment #34)
> I've changed this to take a JSObject pointer to the TCPSocket object as
> well, now. If I root that somehow for the lifetime of TCPSocketChild, will
> that be enough to guarantee the context's existence?

No, there's no owning relationship between a JSContext and a JSObject.

If you're coming from a window you need to somehow grab a strong ref to the nsIScriptContext that it's using. If you're in a JS component or module then the context will live for the lifetime of the JS component loader (basically the same as the lifetime of the app).

However, I don't see why you need to save this context, actually. Can you just use the safe JSContext in RecvOnCallback to create the array buffer?

> >Doesn't look like you ever use mCx... Can you remove this?
> 
> It's used for deserializing.

Oops, yep. Can you use the safe JSContext here too?

Comment 37

[Josh Matthews [:jdm]]

Ah yes, that is the missing piece of the puzzle. Thanks!

Comment 38

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

Comment 39

[Josh Matthews [:jdm]]

Comment on attachment 662708 [details] [diff] [review]
Make TCPSocket e10s-friendly.

Psyke, forgot to make the remaining changes.

Comment 40

[Josh Matthews [:jdm]]

Attachment: Interdiff for JSContext changes

Comment 41

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

Comment 42

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

Comment on attachment 663033 [details] [diff] [review]
Interdiff for JSContext changes

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

r=me with these things addressed.

::: dom/network/src/TCPSocketChild.cpp
@@ +16,5 @@
>  
>  namespace IPC {
>  
>  bool
> +DeserializeUint8Array(JSRawObject aObj,

Nit: This is easily confused with 'obj' below. Let's rename it to 'aCompartmentObj' or something.

@@ +21,4 @@
>                        const nsCString& aBuffer,
>                        jsval* aVal)
>  {
> +  JSContext* cx = nsContentUtils::GetSafeJSContext();

I think it's technically possible for this to fail, best to handle and return false.

@@ +29,1 @@
>    if (!obj)

Nit: These all need braces.

Comment 43

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=2465ff329c0e

I am totally confused why this patch causes every xpcshell test run to fail on try, since I cannot reproduce the problem at all locally.

Comment 44

[Jason Duell]

jdm:  time to ask for a build node to play with?  You can open a bug to get one temporarily--see bug 740534 for example.

Comment 45

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=326bc0cfe913

Yeah, I've got it down to _just_ test_tcpsocket_ipc.js failing on all platforms. I'll file a bug.

Comment 46

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=eddfb8a925a8

http://i.qkme.me/3r319b.jpg

Comment 47

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

Comment 48

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

Comment 49

[Honza Bambas (:mayhemer)]

Josh, can you please explain in a comment to this bug why you need the JS intermediary class?  It is not clear from the patch either from the bug.  Thanks.

Comment 50

[Josh Matthews [:jdm]]

The intermediary is present to handle all of the callbacks in the simplest way. It was much clearer to have the intermediary receive the callbacks and call a well-defined C++ function rather than trying to muck around with the JSAPI to have the same effect from the parent actor.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

(In reply to Josh Matthews [:jdm] from comment #46)
> https://tbpl.mozilla.org/?tree=Try&rev=eddfb8a925a8

Looks like this has xpcshell failures.

Comment 52

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=9bcfd16a9b75

Comment 53

[Josh Matthews [:jdm]]

Attachment: Make TCPSocket e10s-friendly.

Comment 54

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/806fa3680a91

Comment 55

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The email app seems to be working about the same when run out-of-process, which is, sadly to say, not that well :/.

Comment 56

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Not so hot on activesync right now, but IMAP is much better.

Comment 57

[Honza Bambas (:mayhemer)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #55)
> The email app seems to be working about the same when run out-of-process,
> which is, sadly to say, not that well :/.

Can you report a bug on that and cc me?  Maybe I can take a look.  Please also add some instructions how to run the email app in both IP and OOP modes.

Comment 58

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

On which?  I don't know what the baseline functionality of activesync email is.  We need to catch up asuth and squib.

Comment 59

[Donovan Preston [:fzzzy]]

To see if an app is in-process or OOP, check the Blacklist:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L787

You can use "adb shell b2g-procrank" to see if it really is running OOP. (Assuming testing on a phone. On Desktop, check ps or the equivalent)

There are two (three) lists of bugs related to email:

https://github.com/mozilla-b2g/gaia-email-libs-and-more/issues

https://github.com/mozilla-b2g/gaia/issues?labels=email&page=1&state=open

There are also gaia issues with the email-ui tag but those would obviously be more related to ui.

Comment 60

[Andrew Sutherland [:asuth] (he/him)]

ActiveSync uses XHR (with the magic system flags to escape CORS), only IMAP uses TCP.  Autoconfig *just* landed, so you should now be able to create various accounts that are known to Thunderbird's autoconfiguration database.

Please note that we have mapped gmail to use ActiveSync, not IMAP (because you need to turn on IMAP manually in the settings).  We may eventually change things so that we probe for IMAP and use that if possible since the sync strategy for IMAP allows arbitrary scrolling into the deep past and ActiveSync limits us to strict filter sizes.

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK, that's interesting.  My moco-over-IMAP account was working much better than gmail-over-XHR.  Both in- and out-of-process, gmail was much slower to load, and no folders were able to load more than two messages.

Is that a known bug?

Comment 62

[Donovan Preston [:fzzzy]]

The bug is probably that activesync is a crazy complicated protocol compared to IMAP, and that's crazy too because IMAP itself is complicated.

But seriously it doesn't surprise me given the bare to-the-metal nature of the tcpsocket implementation and imap's wire format. It doesn't have to decode wbxml every time and make a million roundtrips to get one piece of information.

Comment 63

[Andrew Sutherland [:asuth] (he/him)]

(offtopic-ish)

ActiveSync is a more straightforward replication protocol, but it is less flexible.  In regards to your bug, when you synchronize a folder, it tells you about the messages in the folder that meet the filter criteria and gives you a synckey.  When you next want to synchronize, you tell it the synckey and it tells you what changed.  The main problems are that 1) it decides what order you get to hear about the messages (hotmail sends you the oldest messages first; I think gmail is more sane), and 2) the filter is something like "sync 1 day/3 days/1 week/2 weeks/1 month/..." (hardcoded enumerated values as part of the protocol) and that's basically all you can do within the sync mechanism, although it's conceivable there are things with the search mechanism that can be done that would bring us closer to IMAP parity.

The default filter value for ActiveSync right now is 3 days.  If you have only received 2 new messages in the last 3 days, that explains that.  Otherwise, there could be something bad happening.  Jim has patches to make the time-span of the filter configurable that should be landing tonight.  Realistically, we probably aren't going to be able to get to investigating anything this week because of the feature push, but you can do what https://wiki.mozilla.org/Gaia/Email/SecretDebugMode says and send us the log and we can take a look next week if you want.

Comment 64

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/806fa3680a91