e10s support for JS TCP Socket

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jduell.mcbugs, Assigned: jdm)

Tracking

({feature})

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:M][WebAPI:P2])

Attachments

(1 attachment, 8 obsolete attachments)

Reporter

Description

7 years ago
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".
B2G's use-case for the JS TCP socket is e-mail, and SSL/TLS is indeed required.
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).
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
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.
Assignee: nobody → dpreston
Reporter

Comment 4

7 years ago
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.
Whiteboard: [LOE:M]
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.
(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

7 years ago
Jason can you take this?
Assignee

Comment 8

7 years ago
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++.
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

7 years ago
Structured cloning uses memcpy on the typed array data, so minus the copies its reasonably fast.
OK.  So we'd pay an extra copy compared to C++, IIUC.  That's probably fine.
Assignee

Comment 12

7 years ago
The obnoxious part is calling the event handlers and ensuring the JSAPI usage and principals are correct.
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).
Assignee

Comment 14

7 years ago
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
Depends on: 776832
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.
No longer depends on: 776832
Josh, can you take this?  If so, please assign to yourself.  Thanks! :)
Assignee

Comment 17

7 years ago
Sure.
Assignee: dpreston → josh
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.
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.
Indeed. Though no more because of the MM usage since even if we went through ipdl we'd generate ArrayBuffer objects in both processes.
Assignee

Comment 21

7 years ago
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.
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.
(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.
Assignee

Comment 24

7 years ago
Posted patch Make TCPSocket e10s-friendly. (obsolete) — Splinter Review
This patch makes test_tcpsocket.js pass when run in a content process. It still needs some cleanup and documentation work.
Assignee

Comment 25

7 years ago
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?
Assignee

Comment 26

7 years ago
Humph. Looks like I need to obtain a device to test this out in anger, since bug 782845 is striking my desktop builds.
(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++.
Assignee

Comment 28

7 years ago
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.
Assignee

Comment 29

7 years ago
Posted patch Make TCPSocket e10s-friendly. (obsolete) — Splinter Review
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?
Attachment #658560 - Flags: review?(bent.mozilla)
Assignee

Updated

7 years ago
Attachment #657056 - Attachment is obsolete: true
Assignee

Comment 30

7 years ago
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++.
Whiteboard: [LOE:M] → [LOE:M][WebAPI:P2]
Keywords: feature
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.
Assignee

Comment 34

7 years ago
>@@ +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.
Assignee

Comment 35

7 years ago
Posted patch Interdiff (obsolete) — Splinter Review
Here's the diff of my changes to address your comments.
Attachment #662161 - Flags: review?(bent.mozilla)
(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?
Assignee

Comment 37

7 years ago
Ah yes, that is the missing piece of the puzzle. Thanks!
Assignee

Comment 38

7 years ago
Posted patch Make TCPSocket e10s-friendly. (obsolete) — Splinter Review
Attachment #662708 - Flags: review?(bent.mozilla)
Assignee

Updated

7 years ago
Attachment #658560 - Attachment is obsolete: true
Attachment #658560 - Flags: review?(bent.mozilla)
Assignee

Updated

7 years ago
Attachment #662161 - Flags: review?(bent.mozilla)
Assignee

Updated

7 years ago
Attachment #662161 - Attachment is obsolete: true
Assignee

Comment 39

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

Psyke, forgot to make the remaining changes.
Attachment #662708 - Flags: review?(bent.mozilla)
Assignee

Comment 40

7 years ago
Attachment #663033 - Flags: review?(bent.mozilla)
Assignee

Comment 41

7 years ago
Posted patch Make TCPSocket e10s-friendly. (obsolete) — Splinter Review
Attachment #663035 - Flags: review?(bent.mozilla)
Assignee

Updated

7 years ago
Attachment #662708 - Attachment is obsolete: true
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.
Attachment #663033 - Flags: review?(bent.mozilla) → review+
Attachment #663035 - Flags: review?(bent.mozilla) → review+
Assignee

Comment 43

7 years ago
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.
Reporter

Comment 44

7 years ago
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.
Assignee

Comment 45

7 years ago
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.
Assignee

Comment 47

7 years ago
Posted patch Make TCPSocket e10s-friendly. (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #663035 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #663033 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Keywords: checkin-needed
Assignee

Updated

7 years ago
Keywords: checkin-needed
Assignee

Comment 48

7 years ago
Posted patch Make TCPSocket e10s-friendly. (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #665010 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Keywords: checkin-needed
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.
Assignee

Comment 50

7 years ago
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.
(In reply to Josh Matthews [:jdm] from comment #46)
> https://tbpl.mozilla.org/?tree=Try&rev=eddfb8a925a8

Looks like this has xpcshell failures.
Keywords: checkin-needed
Assignee

Updated

7 years ago
Attachment #665029 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Keywords: checkin-needed
The email app seems to be working about the same when run out-of-process, which is, sadly to say, not that well :/.
Not so hot on activesync right now, but IMAP is much better.
(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.
On which?  I don't know what the baseline functionality of activesync email is.  We need to catch up asuth and squib.
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.
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.
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?
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/806fa3680a91
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 798338
Depends on: 819425
Depends on: 820415
Blocks: 825539
You need to log in before you can comment on or make changes to this bug.