Closed Bug 1109338 Opened 9 years ago Closed 9 years ago

WebRTC e10s/IPC traffic needs to avoid MainThread and use PBackground instead

Categories

(Core :: WebRTC: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
e10s + ---
firefox41 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files, 14 obsolete files)

5.74 KB, patch
dragana
: review+
Details | Diff | Splinter Review
14.90 KB, patch
dragana
: review+
Details | Diff | Splinter Review
15.03 KB, patch
bwc
: review+
Details | Diff | Splinter Review
10.05 KB, patch
bent.mozilla
: review-
Details | Diff | Splinter Review
28.59 KB, patch
Details | Diff | Splinter Review
29.72 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
For many, many reasons webrtc networking access in e10s situations needs to avoid MainThread like the plague, and in specific avoid using IPC that hits mainthread.  See Bug 1091240 where a busy MainThread in B2G Master process (due to a network activity anim) caused 3000ms RTT for a loopback connection.  It also seriously will mess up bandwidth estimation, if for no other reason than adding an unknown amount of jitter to packet reception times in the content process.

For an example of how to use PBackground, see:
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundChild.h#28
and dom/indexedDB/PBackgroundIDBDatabase.ipdl
This bug is going to show up in our e10s triage session.. a couple questions:

1) How serious is this and what's the time frame we're hoping for here?
2) This sounds like multiple bugs to me (netwerk, webrtc, maybe front end)?
Flags: needinfo?(rjesup)
It may end up being a metabug, but for now this bug is focused on the network e10s code as used by webrtc (aka mtransport in this case), which talks to stuff in netwerk to do port allocation/binding/dns/UDP/TCP/etc.  We could spin out sub-bugs for the different bits of UDP/TCP/TURN/etc as needed, but moving the IPC channel to background might fix them all in one fell swoop.

It is serious. The bandwidth allocation issue alone is a real problem.  This won't stop webrtc from working, but may make it work poorly in a range of situations.  The b2g problem has been wallpapered for now (by trying to limit the mainthread b2g delay), but it certainly will be wanting this fix too.

Note that Hello-in-e10s-mode is not a good test, since Hello is using WebRTC from chrome it appears (i.e. on the master process in e10s).  You can see this by the fact that getUserMedia from a testpage (webrtc_landing/gum_test.html) is blocked by a running Hello window.  That's a separate bug.  (As is I think GMP/EME's use of IPC! which is a bug even without e10s!  Also any sandboxing IPC should look at this...)
Flags: needinfo?(rjesup)
builds and runs, though I haven't actually done anything *with* the ability for it to be a child of Necko or PBackground
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Hooked up more, especially UDPSocketChild, and poke it from nr_socket_prsock to switch to PBackground.  Builds and runs in non-e10s (didn't torch anything too badly); fails with !Is_MainThread() assertion in my first e10s run
Attachment #8576899 - Attachment is obsolete: true
Note: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=935838 changes  how permissions are checked. It is going to use Principals. 

Bug 935838 is finish, just need a last check.
Rank: 10
Flags: firefox-backlog+
Priority: -- → P1
Attachment #8577387 - Attachment is obsolete: true
patchset now works!

Moderate cleanup and dealing with a few papered-over issues remains.

The difference should be significant (especially delay) when forced to share MainThread with something making the content process "janky" with e10s (and not in Loop, which sidesteps e10s by running in Master).
Attachment #8579153 - Attachment is obsolete: true
Comment on attachment 8579158 [details] [diff] [review]
Part 1: Separate UDPSocket logging

I'll note I only added logs I thought I'd need; one could add more
Attachment #8579158 - Flags: review?(jduell.mcbugs)
Comment on attachment 8579154 [details] [diff] [review]
Part 2: Sharing UDPSocket between PNecko and PBackground

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

A few loose ends, but most of this is reviewable
Attachment #8579154 - Flags: feedback?(dd.mozilla)
Attachment #8579154 - Flags: feedback?(bent.mozilla)
Attachment #8579155 - Flags: review?(jduell.mcbugs)
Comment on attachment 8579156 [details] [diff] [review]
Part 4: Use separate IO thread for mtransport

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

This is basically done, except for dealing with the MainThreadPtrHolder<> stuff.
Options: a) make UDPSocketChild threadsafe (overhead, but easy), b) make a generic version of MainThreadPtrHolder that can be used for a specific non-Main thread (and maybe make MainThreadPtrHolder a specialization of that) c) analyze things and decide we don't need a PtrHolder at all (haven't checked this, likely not possible as I assume it's there for a reason).
Attachment #8579156 - Flags: feedback?(ekr)
Comment on attachment 8579156 [details] [diff] [review]
Part 4: Use separate IO thread for mtransport

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

::: media/mtransport/nr_socket_prsock.cpp
@@ +1164,5 @@
>  };
>  
> +// XXX Fix! static constructor
> +// FIX! clean up for shutdown!
> +nsCOMPtr<nsIThread> gMTransportThread;

This needs to be dealt with as well
Depends on: 1145354
part 5 is the LazySingletonThread patch
folded the two media/mtransport patches
r? to ekr/bwc; will take whomever I can get
Attachment #8579156 - Attachment is obsolete: true
Attachment #8580279 - Attachment is obsolete: true
Attachment #8579156 - Flags: feedback?(ekr)
Attachment #8580289 - Flags: review?(ekr)
Attachment #8580289 - Flags: review?(docfaraday)
Comment on attachment 8580289 [details] [diff] [review]
Part 4: Use  LazySingletonThread for all mtransport IPC sends/etc with PBackground

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

Can't really make a call on this until we know more about the life-cycle of nsIUDPSocketChild, but not noticing anything major. Once we get that detail nailed down, I can look again.

::: dom/network/UDPSocketChild.h
@@ +17,5 @@
>  namespace dom {
>  
>  class UDPSocketChildBase : public nsIUDPSocketChild {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Whether we need this change is dependent on how we end up handling the lifecycle of these, so I cannot make a call here yet.

::: media/mtransport/nr_socket_prsock.cpp
@@ +748,3 @@
>      : err_(false),
>        state_(NR_INIT),
> +      io_thread_(GetThread()),

At first glance this looks like it is getting the current thread to me. Maybe call this GetIOThread().

@@ +1066,5 @@
>      mon.NotifyAll();
>      return;
>    }
>  
> +  socket_child_ = socketChild; // XXX MainThreadPtrHolder

Is the assumption here that |socket_child_| will need to be destroyed on the io thread? I could get behind having a generic smart pointer that throws its reference away on a specific thread, as that could be useful.

::: media/mtransport/nr_socket_prsock.h
@@ +237,5 @@
>  
> +#ifdef MOZILLA_INTERNAL_API
> +  static LazySingletonThread* GetThread() {
> +    // c++11 guarantees this is threadsafe!
> +    static nsRefPtr<LazySingletonThread> sThread =

This is only ever called from STS, right? Also, I'm not sure whether the guarantee that only one thread will execute the c'tor of nsRefPtr<LazySingletonThread> extends to the creation of the arguments passed to that c'tor (ie; can more than one thread create a LazySingletonThread, but only one is allowed to create the nsRefPtr).

@@ +246,5 @@
> +#else
> +  static nsIThread* GetThread() {
> +    static nsCOMPtr<nsIThread> sThread;
> +    if (!sThread) {
> +      (void) NS_NewNamedThread("mtransport", getter_AddRefs(sThread));

Do we have any unit-test that uses NrSocketIpc? Will this code ever run?

::: media/mtransport/stun_udp_socket_filter.h
@@ +11,5 @@
>        { 0xa3, 0x4e, 0x62, 0xd9, 0xe4, 0xc9, 0xf9, 0x93 } };
>  
>  class nsStunUDPSocketFilterHandler : public nsIUDPSocketFilterHandler {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Again, we need to nail down the life-cycle before I can decide whether this is necessary.
Attachment #8580289 - Flags: review?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #18)
> Comment on attachment 8580289 [details] [diff] [review]
> Part 4: Use  LazySingletonThread for all mtransport IPC sends/etc with
> PBackground
> 
> Review of attachment 8580289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't really make a call on this until we know more about the life-cycle of
> nsIUDPSocketChild, but not noticing anything major. Once we get that detail
> nailed down, I can look again.
> 
> ::: dom/network/UDPSocketChild.h
> @@ +17,5 @@
> >  namespace dom {
> >  
> >  class UDPSocketChildBase : public nsIUDPSocketChild {
> >  public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Whether we need this change is dependent on how we end up handling the
> lifecycle of these, so I cannot make a call here yet.
> 
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +748,3 @@
> >      : err_(false),
> >        state_(NR_INIT),
> > +      io_thread_(GetThread()),
> 
> At first glance this looks like it is getting the current thread to me.
> Maybe call this GetIOThread().
> 
> @@ +1066,5 @@
> >      mon.NotifyAll();
> >      return;
> >    }
> >  
> > +  socket_child_ = socketChild; // XXX MainThreadPtrHolder
> 
> Is the assumption here that |socket_child_| will need to be destroyed on the
> io thread? I could get behind having a generic smart pointer that throws its
> reference away on a specific thread, as that could be useful.

The comment was to the old impl; with the THREADSAFE change above it isn't needed.

> 
> ::: media/mtransport/nr_socket_prsock.h
> @@ +237,5 @@
> >  
> > +#ifdef MOZILLA_INTERNAL_API
> > +  static LazySingletonThread* GetThread() {
> > +    // c++11 guarantees this is threadsafe!
> > +    static nsRefPtr<LazySingletonThread> sThread =
> 
> This is only ever called from STS, right? Also, I'm not sure whether the
> guarantee that only one thread will execute the c'tor of
> nsRefPtr<LazySingletonThread> extends to the creation of the arguments
> passed to that c'tor (ie; can more than one thread create a
> LazySingletonThread, but only one is allowed to create the nsRefPtr).

A valid question; I'll see if I can munge into whatever syntax is needed to create the object in the constructor.  Worst case: a class that wraps the nsRefPtr<> = new blah() and does nothing else.  So easily solvable.

> 
> @@ +246,5 @@
> > +#else
> > +  static nsIThread* GetThread() {
> > +    static nsCOMPtr<nsIThread> sThread;
> > +    if (!sThread) {
> > +      (void) NS_NewNamedThread("mtransport", getter_AddRefs(sThread));
> 
> Do we have any unit-test that uses NrSocketIpc? Will this code ever run?

Probably not... the unit tests don't do e10s. (none of them).  However, this is what you'd need.  LaxzyIdleThread.h is not includeable in a non-INTERNAL compile.

> 
> ::: media/mtransport/stun_udp_socket_filter.h
> @@ +11,5 @@
> >        { 0xa3, 0x4e, 0x62, 0xd9, 0xe4, 0xc9, 0xf9, 0x93 } };
> >  
> >  class nsStunUDPSocketFilterHandler : public nsIUDPSocketFilterHandler {
> >  public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Again, we need to nail down the life-cycle before I can decide whether this
> is necessary.
Comment on attachment 8580289 [details] [diff] [review]
Part 4: Use  LazySingletonThread for all mtransport IPC sends/etc with PBackground

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

::: media/mtransport/nr_socket_prsock.cpp
@@ +1166,5 @@
>  int nr_socket_local_create(nr_transport_addr *addr, nr_socket **sockp) {
>    RefPtr<NrSocketBase> sock;
>  
> +  int r, _status;
> +

Why did this move?

::: media/mtransport/nr_socket_prsock.h
@@ +235,5 @@
>  
>    DISALLOW_COPY_ASSIGN(NrSocketIpc);
>  
> +#ifdef MOZILLA_INTERNAL_API
> +  static LazySingletonThread* GetThread() {

This seems like it would be better if it returned nsIThread*, which is a base class of LazySingletonThread

@@ +255,5 @@
> +
> +  // Main or private thread executors of the NrSocketBase APIs
> +  void create_io(const nsACString &host, const uint16_t port);
> +  void sendto_io(const net::NetAddr &addr, nsAutoPtr<DataBuffer> buf);
> +  void close_io();

If the _io suffix means it runs on the io thread, then _i would be better.

@@ +263,5 @@
>    bool err_;
>    NrSocketIpcState state_;
>    std::queue<RefPtr<nr_udp_message> > received_msgs_;
>  
> +  nsRefPtr<nsIUDPSocketChild> socket_child_; // XXX MainThreadPtrHandle

What is this XXX doing?

Also convention in this code is TODO(user), preferably with a bug #.
Attachment #8580289 - Flags: review?(ekr)
> > +  int r, _status;
> > +
> 
> Why did this move?

An earlier version of the patch needed them above where they were.   That's no longer the case, so I'll restore them to the old location.
 
> > +#ifdef MOZILLA_INTERNAL_API
> > +  static LazySingletonThread* GetThread() {
> 
> This seems like it would be better if it returned nsIThread*, which is a
> base class of LazySingletonThread

ok

> > +  void create_io(const nsACString &host, const uint16_t port);
> > +  void sendto_io(const net::NetAddr &addr, nsAutoPtr<DataBuffer> buf);
> > +  void close_io();
> 
> If the _io suffix means it runs on the io thread, then _i would be better.

ok

> > +  nsRefPtr<nsIUDPSocketChild> socket_child_; // XXX MainThreadPtrHandle
> 
> What is this XXX doing?
> 
> Also convention in this code is TODO(user), preferably with a bug #.

The XXX comments are no longer needed; they were a tickler to me about what used to be there.
> >  class UDPSocketChildBase : public nsIUDPSocketChild {
> >  public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Whether we need this change is dependent on how we end up handling the
> lifecycle of these, so I cannot make a call here yet.

Reviewed the lifecycle: this is no longer needed; SocketChild is solely accessed from the io_thread, except for one null-check in sendto() which I moved to sendto_i(), and an implicit delete (from sts thread) in ~NrSocketIpc(), which I've now made an explicit release_child() (effectively a static close() on the IO thread), which will release socket_child from the io_thread.  Very likely the reason socket_child_ had to be a MainThreadPtrHolder before was the lack of (deletion-safe) close() in ~NrSocketIpc().

> Is the assumption here that |socket_child_| will need to be destroyed on the
> io thread? I could get behind having a generic smart pointer that throws its
> reference away on a specific thread, as that could be useful.

See above; I agree a thread-specific-delete generalization of MainThreadPtrHolder would be handy.  WrapRelease() is close (requires a manual RUN_ON_THREAD/etc), but we want to close the IPC channel as well.  (We probably could just abruptly release socket_child on the io_thread, but it seems wrong to do.)

If you want, we could instead just RUN_ON_THREAD(io_thread_, &mozilla::WrapRelease(socket_thread_.forget()), NS_DISPATCH_NORMAL) from ~NrSocketIpc().

> ::: media/mtransport/nr_socket_prsock.h
> @@ +237,5 @@
> >  
> > +#ifdef MOZILLA_INTERNAL_API
> > +  static LazySingletonThread* GetThread() {
> > +    // c++11 guarantees this is threadsafe!
> > +    static nsRefPtr<LazySingletonThread> sThread =
> 
> This is only ever called from STS, right? Also, I'm not sure whether the
> guarantee that only one thread will execute the c'tor of
> nsRefPtr<LazySingletonThread> extends to the creation of the arguments
> passed to that c'tor (ie; can more than one thread create a
> LazySingletonThread, but only one is allowed to create the nsRefPtr).

Expanded the comments there.  We are only on sts (indirectly from StartGathering())

> 
> @@ +246,5 @@
> > +#else
> > +  static nsIThread* GetThread() {
> > +    static nsCOMPtr<nsIThread> sThread;
> > +    if (!sThread) {
> > +      (void) NS_NewNamedThread("mtransport", getter_AddRefs(sThread));
> 
> Do we have any unit-test that uses NrSocketIpc? Will this code ever run?
> 
> ::: media/mtransport/stun_udp_socket_filter.h
> @@ +11,5 @@
> >        { 0xa3, 0x4e, 0x62, 0xd9, 0xe4, 0xc9, 0xf9, 0x93 } };
> >  
> >  class nsStunUDPSocketFilterHandler : public nsIUDPSocketFilterHandler {
> >  public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Again, we need to nail down the life-cycle before I can decide whether this
> is necessary.

For this one, we need THREADSAFE -- it's released on MainThread from FreeFactoryEntries(), but is created off-MainThread now.

Added a comment.
Comment on attachment 8580541 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

Updated per comments.  If you prefer to get rid of release_child*() and not bother with close(), we can just replace that (as mentioned) with a RUN_ON_THREAD(io_thread_, mozilla::WrapRelease(...), NS_DISPATCH_NORMAL)
Attachment #8580541 - Flags: review?(ekr)
Attachment #8580541 - Flags: review?(docfaraday)
Attachment #8580289 - Attachment is obsolete: true
    // c++11 guarantees static Foo foo(...) is threadsafe!

Note that reviewing http://stackoverflow.com/questions/2576022/efficient-thread-safe-singleton-in-c indicates that this NOT supported in VS2013 (but is in 2014).  Just as well that the analysis above says we don't need to care (always on STS thread).

Sigh...  I'll revise the comment.
small changes to release_child_i() and NrSocketIpc() to support update to LazySingletonThread API
Attachment #8580541 - Attachment is obsolete: true
Attachment #8580541 - Flags: review?(ekr)
Attachment #8580541 - Flags: review?(docfaraday)
Attachment #8580737 - Flags: review?(ekr)
Attachment #8580737 - Flags: review?(docfaraday)
Comment on attachment 8580737 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

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

::: media/mtransport/nr_socket_prsock.cpp
@@ +751,5 @@
>        monitor_("NrSocketIpc") {
> +#ifdef MOZILLA_INTERNAL_API
> +  // Mark that we're using the shared thread and need it to stick around
> +  LazySingletonThread* thread = static_cast<LazySingletonThread*>(GetIOThread());
> +  thread->AddUse();

Maybe having the MOZILLA_INTERNAL_API version of GetIOThread return a LazySingletonThread* isn't a bad idea given this new API. We'd at least avoid the static_casts that way.

@@ +982,5 @@
> +  std::swap(received_msgs_, empty);
> +}
> +
> +// close(), but transfer the socket_child_ reference to die as well
> +void NrSocketIpc::release_child() {

If we inlined this stuff in the destructor, we wouldn't need anything but the call to |release_child_i| (no need to set state, no need to clear out the received messages). And, if we inline the |ReleaseUse| as well we'll be doing it on the right thread (STS), and |release_child_i| will be named a little more accurately.

@@ +1112,4 @@
>  
> +  if (!socket_child_) {
> +    err_ = true;
> +    return;

Even though we are no longer checking this in |sendto|, this is still something that shouldn't happen (sendto should never be called after close), and probably still requires an assert.

@@ +1140,5 @@
> +    socket_child_ref->Close();
> +  }
> +  // Tell LazySingletonThread we're done with it
> +#ifdef MOZILLA_INTERNAL_API
> +  LazySingletonThread* thread = static_cast<LazySingletonThread*>(GetIOThread());

This is not on STS.

::: media/mtransport/nr_socket_prsock.h
@@ +245,5 @@
> +    // MSVS 2014 does (yeah!)  So for now, we rely on always calling on sts thread
> +
> +    // It's less certain that static Foo foo = new bar() is threadsafe, however we're
> +    // always on sts_thread anyways.  Otherwise, wrap the nsRefPtr<> in a class so
> +    // we can do the assign in the constructor safely, and use static FooWrapper foo(...);

Let's just call this GetIOThread_s(), and say that it can only be called on STS (especially considering that the non MOZILLA_INTERNAL_API version isn't threadsafe at all).
Attachment #8580737 - Flags: review?(docfaraday)
Attachment #8579154 - Attachment is obsolete: true
Attachment #8579154 - Flags: feedback?(dd.mozilla)
Attachment #8579154 - Flags: feedback?(bent.mozilla)
Attachment #8580795 - Flags: review?(wmccloskey)
Attachment #8580795 - Flags: review?(bent.mozilla)
Attachment #8580795 - Flags: feedback?(dd.mozilla)
Comment on attachment 8579155 [details] [diff] [review]
Part 3: Use separate proxy and UDPMessage class for PBackground

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

I'm going to delegate this review to dragana
Attachment #8579155 - Flags: review?(jduell.mcbugs) → review?(dd.mozilla)
Comment on attachment 8579158 [details] [diff] [review]
Part 1: Separate UDPSocket logging

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

I'm going to delegate this review to dragana
Attachment #8579158 - Flags: review?(jduell.mcbugs) → review?(dd.mozilla)
Comment on attachment 8579155 [details] [diff] [review]
Part 3: Use separate proxy and UDPMessage class for PBackground

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

looks good.
Attachment #8579155 - Flags: review?(dd.mozilla) → review+
Attachment #8580737 - Flags: review?(docfaraday)
Comment on attachment 8579158 [details] [diff] [review]
Part 1: Separate UDPSocket logging

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

With some small changes r=me.

::: dom/network/UDPSocket.h
@@ +24,5 @@
> +//
> +extern PRLogModuleInfo *gUDPSocketLog;
> +#endif
> +#define UDPSOCKET_LOG(args)     PR_LOG(gUDPSocketLog, PR_LOG_DEBUG, args)
> +#define UDPSOCKET_LOG_ENABLED() PR_LOG_TEST(gUDPSocketLog, PR_LOG_DEBUG)

The UDPSOCKET_LOG is only used in UDPSocket.cpp, I don't know if this going to be use somewhere else in the future, You can move it there, but it is no that important....

::: dom/network/UDPSocketChild.cpp
@@ +76,5 @@
>                       uint16_t aPort,
>                       bool aAddressReuse,
>                       bool aLoopback)
>  {
> +  UDPSOCKET_LOG(("%s: %s:%u", __FUNCTION__, nsCString(aHost).get(), aPort));

It is better to use  PromiseFlatCString instead of nsCString. And in the other places...

@@ +104,5 @@
>                       uint32_t aByteLength)
>  {
>    NS_ENSURE_ARG(aData);
>  
> +  UDPSOCKET_LOG(("%s: %s:%u - %u bytes", __FUNCTION__, nsCString(aHost).get(), aPort, aByteLength));

I think for uint32_t, it should be %lu

::: dom/network/UDPSocketParent.cpp
@@ +155,5 @@
>                                const bool& aAddressReuse, const bool& aLoopback)
>  {
>    nsresult rv;
>  
> +  UDPSOCKET_LOG(("%s: %s:%u", __FUNCTION__, nsCString(aHost).get(), aPort));

Does this need nsCString()?
Attachment #8579158 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8580737 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

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

See review comments in comment 27
Attachment #8580737 - Flags: review?(docfaraday) → review-
* * *
Bug 1109338: Part 4: interdiffs for update
Attachment #8580737 - Attachment is obsolete: true
Attachment #8580737 - Flags: review?(ekr)
Comment on attachment 8582632 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

See interdiffs in separate patch
Attachment #8582632 - Flags: review?(docfaraday)
> @@ +104,5 @@
> >                       uint32_t aByteLength)
> >  {
> >    NS_ENSURE_ARG(aData);
> >  
> > +  UDPSOCKET_LOG(("%s: %s:%u - %u bytes", __FUNCTION__, nsCString(aHost).get(), aPort, aByteLength));
> 
> I think for uint32_t, it should be %lu

sizeof(uint32_t) is <= sizeof(int) for all platforms we support, so %u is fine.  For uint64_t, this is how you "should" do it: (("...%s:" PRIu64 " - ...", string, uint64)).  However, %llu works well for all platforms we care about for now.
Comment on attachment 8582632 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

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

There's some simplification I'd like to see here.

::: media/mtransport/nr_socket_prsock.h
@@ +193,5 @@
>    DISALLOW_COPY_ASSIGN(nr_udp_message);
>  };
>  
> +#ifdef MOZILLA_INTERNAL_API
> +static StaticRefPtr<LazySingletonThread> sThread;

I'd prefer to avoid polluting the namespace like this, let's move it to the cpp file.

@@ +195,5 @@
>  
> +#ifdef MOZILLA_INTERNAL_API
> +static StaticRefPtr<LazySingletonThread> sThread;
> +
> +class ClearIpcThread : public nsIRunnable

Why not use WrapRunnableNM and a static function that calls ClearOnShutdown()?

@@ +266,5 @@
> +      new LazySingletonThread(500, NS_LITERAL_CSTRING("mtransport"));
> +    static bool created = false;
> +    if (!created) {
> +      created = true;
> +      sThread = thread; // set owning reference

Couldn't this all be simplified to

if (!sThread) {
   sThread = new LazySingletonThread(...);

and the comments about the threadsafety of local statics removed, since we're now so far from being threadsafe it doesn't bear mentioning?
Attachment #8582632 - Flags: review?(docfaraday)
Comment on attachment 8580795 [details] [diff] [review]
Part 2: Sharing UDPSocket between PNecko and PBackground

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

I am not very familiar with PBackground, but it looks sane.

::: dom/network/UDPSocketChild.cpp
@@ +63,5 @@
>  }
>  
>  UDPSocketChild::UDPSocketChild()
> +:mChannel(nullptr)
> +,mLocalPort(0)

space

@@ +112,5 @@
> +  using mozilla::ipc::BackgroundChild;
> +
> +  MOZ_ASSERT(!BackgroundChild::GetForCurrentThread());
> +
> +  bool done = false;

should this be atomic?

::: dom/network/UDPSocketChild.h
@@ +54,5 @@
>    nsresult SendDataInternal(const UDPSocketAddr& aAddr,
>                              const uint8_t* aData,
>                              const uint32_t aByteLength);
>  
> +  mozilla::ipc::PBackgroundChild *mChannel;

maybe change the variable name, it reminds me of channels(httpChannel...) from networking, but it is just a suggestion.

::: dom/network/UDPSocketParent.cpp
@@ +85,5 @@
>  UDPSocketParent::GetAppId()
>  {
>    uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
> +  if (!mNeckoManager) {
> +    return appId; // XXX Is there a way to get PContentParent or equivalent for PBackground?

this will be change in bug 935838. The nsIPrincipal will be given in Init() and appid will be retrieved from it.

@@ +141,2 @@
>    }
> +  // XXX else { ? } for PBackground

the same as with appid.

::: dom/network/interfaces/nsIUDPSocketChild.idl
@@ +6,5 @@
>  #include "nsINetAddr.idl"
>  
>  interface nsIUDPSocketInternal;
>  interface nsIInputStream;
> +interface nsIPrincipal;

Do you need this?

::: ipc/glue/PBackground.ipdl
@@ +46,5 @@
>    PVsync();
>  
> +  PUDPSocket(nsCString filter);
> +
> +PBroadcastChannel(PrincipalInfo pInfo, nsString origin, nsString channel);

space
Attachment #8580795 - Flags: feedback?(dd.mozilla) → feedback+
Attachment #8582632 - Attachment is obsolete: true
Comment on attachment 8582738 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

See interdiffs 2 as well
Attachment #8582738 - Flags: review?(docfaraday)
Comment on attachment 8582738 [details] [diff] [review]
Part 4: Use LazySingletonThread for mtransport IPC IO

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

lgtm
Attachment #8582738 - Flags: review?(docfaraday) → review+
Attachment #8582628 - Attachment is obsolete: true
Attachment #8582737 - Attachment is obsolete: true
Comment on attachment 8580795 [details] [diff] [review]
Part 2: Sharing UDPSocket between PNecko and PBackground

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

I'm mostly worried here about the fact that all the code in UDPSocketParent is now running off the main thread. I looked it over and found one problem, I think: bug 1148161. That's pre-existing and probably not a big deal though. However, the privilege checks are going to be difficult.

I'd be much happier if bent looked this over at least superficially. This code is pretty far outside my comfort zone. Maybe he can think of some better ideas here.

::: dom/network/UDPSocketChild.cpp
@@ +106,5 @@
> +
> +NS_IMPL_ISUPPORTS(UDPSocketBackgroundChildCallback, nsIIPCBackgroundChildCreateCallback)
> +
> +nsresult
> +UDPSocketChild::SynchronouslyCreatePBackground()

This is really unfortunate, but I don't see a way around it aside from keeping a buffer of all the socket commands to send once the PBackgroundChild is available. I don't think we should do that though.

@@ +112,5 @@
> +  using mozilla::ipc::BackgroundChild;
> +
> +  MOZ_ASSERT(!BackgroundChild::GetForCurrentThread());
> +
> +  bool done = false;

Yes. The callback can run on any thread.

::: dom/network/UDPSocketChild.h
@@ +54,5 @@
>    nsresult SendDataInternal(const UDPSocketAddr& aAddr,
>                              const uint8_t* aData,
>                              const uint32_t aByteLength);
>  
> +  mozilla::ipc::PBackgroundChild *mChannel;

Yeah, calling this mBackgroundChild would be a lot clearer.

::: dom/network/UDPSocketParent.cpp
@@ +85,5 @@
>  UDPSocketParent::GetAppId()
>  {
>    uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
> +  if (!mNeckoManager) {
> +    return appId; // XXX Is there a way to get PContentParent or equivalent for PBackground?

Even with bug 935838, we're still going to have a problem. That bug does a bunch of permissions asserting from Init that can only happen on the main thread. With this bug, we're running Init on the PBackground thread.

The best way to fix this would probably be to pass whatever permission information we need to the PBackgroundParent when it's constructed from ContentParent. If that doesn't work, an alternate way would be to use one of those horrible SyncRunnable things. If it comes to that, you can get the ContentParent via BackgroundParent::GetContentParent(Manager()). Then you can manipulate it on the main thread.

::: dom/network/UDPSocketParent.h
@@ +57,5 @@
>    void FireInternalError(uint32_t aLineNo);
>  
> +  // One of these will be null and the other non-null.
> +  PBackgroundParent* mBackgroundManager;
> +  PNeckoParent* mNeckoManager; // ownership?

These should get nulled out in ActorDestroy.

::: dom/network/interfaces/nsIUDPSocketChild.idl
@@ +26,5 @@
>    readonly attribute AUTF8String localAddress;
>    attribute AUTF8String filterName;
>  
> +  // Allow hosting this over PBackground instead of PNecko
> +  void setBackground();

Can we make this C++-only?

::: ipc/glue/BackgroundParentImpl.cpp
@@ +255,5 @@
>  }
>  
> +auto
> +BackgroundParentImpl::AllocPUDPSocketParent(const nsCString& /* unused */)
> +  -> PUDPSocketParent*

Why is the type auto if you end up declaring it anyway?
Attachment #8580795 - Flags: review?(wmccloskey)
Comment on attachment 8580795 [details] [diff] [review]
Part 2: Sharing UDPSocket between PNecko and PBackground

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

Sorry it took so long to get this done, my queue has been nuts :(

Now that I understand what it's doing and have gone through it once subsequent  reviews should be much quicker, promise!

::: dom/network/UDPSocketChild.cpp
@@ +112,5 @@
> +  using mozilla::ipc::BackgroundChild;
> +
> +  MOZ_ASSERT(!BackgroundChild::GetForCurrentThread());
> +
> +  bool done = false;

This is actually fine, PBackground callbacks always come in on the thread you requested them.

@@ +122,5 @@
> +  }
> +
> +  nsIThread* thread = NS_GetCurrentThread();
> +  while (!done) {
> +    if (NS_WARN_IF(!NS_ProcessNextEvent(thread, true /* aMayWait */))) {

Spinning the event loop is really dangerous...

We need to have some assertions here to make sure we don't have any code on the stack that doesn't handle reentrancy. At a minimum I think you should add |MOZ_ASSERT(!nsContentUtils::GetCurrentJSContext());|, that would at least keep us safe with respect to JS.

And we should try to think about what would break if the parent tries to shut this process down while you're still spinning.

@@ +145,5 @@
> +  PBackgroundChild* existingBackgroundChild =
> +    BackgroundChild::GetForCurrentThread();
> +  // If it's not spun up yet, block until it is, and retry
> +  if (!existingBackgroundChild) {
> +    SynchronouslyCreatePBackground();

You're not checking the return value here...

@@ +150,5 @@
> +    existingBackgroundChild =
> +      BackgroundChild::GetForCurrentThread();
> +  }
> +  // By now PBackground is guaranteed to be up
> +  MOZ_RELEASE_ASSERT(existingBackgroundChild);

It could fail, especially if the process is shutting down.

::: dom/network/UDPSocketChild.h
@@ +39,5 @@
>  
>    UDPSocketChild();
>    virtual ~UDPSocketChild();
>  
> +  nsresult SynchronouslyCreatePBackground();

I would change this name, to me it sounds like it blocks. What about "CreatePBackgroundWhileSpinningEventLoop"? Something like that to indicate that it spins because spinning the event loop breaks lots of code.

::: dom/network/UDPSocketParent.cpp
@@ +85,5 @@
>  UDPSocketParent::GetAppId()
>  {
>    uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
> +  if (!mNeckoManager) {
> +    return appId; // XXX Is there a way to get PContentParent or equivalent for PBackground?

Yeah, permission checking from PBackground is annoying right now. We have a helper that may make your life a little easier:

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.cpp#494

Basically you need to pass a PrincipalInfo struct to your UDPSocketParent constructor. That will then make sure that the app that sent the message has a plausible principal. You can save off any info you need from that struct (appId, etc).

@@ +141,2 @@
>    }
> +  // XXX else { ? } for PBackground

You can verify the app permission as part of the principal checking step.

::: dom/network/interfaces/nsIUDPSocketChild.idl
@@ +26,5 @@
>    readonly attribute AUTF8String localAddress;
>    attribute AUTF8String filterName;
>  
> +  // Allow hosting this over PBackground instead of PNecko
> +  void setBackground();

Yes, please mark this [noscript]

And like with the other method on UDPSocketChild I think this name needs to somehow indicate that it may spin the event loop.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +219,5 @@
>  
> +PUDPSocketChild*
> +BackgroundChildImpl::AllocPUDPSocketChild(const nsCString& aFilter)
> +{
> +  NS_NOTREACHED("AllocPUDPSocket should not be called");

Please do MOZ_CRASH instead.

::: ipc/glue/BackgroundChildImpl.h
@@ +77,5 @@
>    virtual bool
>    DeallocPVsyncChild(PVsyncChild* aActor) MOZ_OVERRIDE;
>  
> +  virtual PUDPSocketChild* AllocPUDPSocketChild(const nsCString& aFilter) MOZ_OVERRIDE;
> +  virtual bool DeallocPUDPSocketChild(PUDPSocketChild* aActor) MOZ_OVERRIDE;

Nit: Return types on their own lines, wrap at 80 chars.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +18,5 @@
>  #include "mozilla/ipc/BackgroundUtils.h"
>  #include "mozilla/ipc/PBackgroundSharedTypes.h"
>  #include "mozilla/ipc/PBackgroundTestParent.h"
>  #include "mozilla/layout/VsyncParent.h"
> +#include "mozilla/net/PUDPSocketParent.h"

This shouldn't be needed.

@@ +36,5 @@
>  using mozilla::ipc::AssertIsOnBackgroundThread;
>  using mozilla::dom::cache::PCacheParent;
>  using mozilla::dom::cache::PCacheStorageParent;
>  using mozilla::dom::cache::PCacheStreamControlParent;
> +using mozilla::net::PUDPSocketParent;

Nor this

::: ipc/glue/BackgroundParentImpl.h
@@ +113,5 @@
>    virtual bool
>    DeallocPCacheStreamControlParent(dom::cache::PCacheStreamControlParent* aActor) MOZ_OVERRIDE;
> +
> +  virtual PUDPSocketParent* AllocPUDPSocketParent(const nsCString& aFilter) MOZ_OVERRIDE;
> +  virtual bool RecvPUDPSocketConstructor(PUDPSocketParent*, const nsCString& aFilter) MOZ_OVERRIDE;

Nit: Return types on their own lines, wrap at 80 chars.

::: ipc/glue/PBackground.ipdl
@@ +10,5 @@
>  include protocol PCacheStorage;
>  include protocol PCacheStreamControl;
>  include protocol PFileDescriptorSet;
>  include protocol PVsync;
> +include protocol PUDPSocket;

Nit: Can you alphabetize this please? And the 'manages' line below.

::: media/mtransport/nr_socket_prsock.cpp
@@ +1067,5 @@
>      return;
>    }
>  
>    socket_child_ = new nsMainThreadPtrHolder<nsIUDPSocketChild>(socketChild);
> +  socket_child_->SetBackground();

This will spin the event loop while holding a monitor! Any other runnables posted could attempt to grab the same monitor and deadlock. You must drop the monitor before spinning here.
Attachment #8580795 - Flags: review?(bent.mozilla) → review-
> Sorry it took so long to get this done, my queue has been nuts :(
> 
> Now that I understand what it's doing and have gone through it once
> subsequent  reviews should be much quicker, promise!

Thanks!  I'm not bothering to do the "Ok" bit to minor issues.

> @@ +122,5 @@
> > +  }
> > +
> > +  nsIThread* thread = NS_GetCurrentThread();
> > +  while (!done) {
> > +    if (NS_WARN_IF(!NS_ProcessNextEvent(thread, true /* aMayWait */))) {
> 
> Spinning the event loop is really dangerous...

We're totally safe here; the thread we run this on is our LazySingletonThread for mtransport IO.

> And we should try to think about what would break if the parent tries to
> shut this process down while you're still spinning.

Ok.

> ::: dom/network/UDPSocketParent.cpp
> @@ +85,5 @@
> >  UDPSocketParent::GetAppId()
> >  {
> >    uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
> > +  if (!mNeckoManager) {
> > +    return appId; // XXX Is there a way to get PContentParent or equivalent for PBackground?
> 
> Yeah, permission checking from PBackground is annoying right now. We have a
> helper that may make your life a little easier:
> 
> http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundParentImpl.
> cpp#494
> 
> Basically you need to pass a PrincipalInfo struct to your UDPSocketParent
> constructor. That will then make sure that the app that sent the message has
> a plausible principal. You can save off any info you need from that struct
> (appId, etc).

Ok... so Dragana's patches landed in the meantime, which changed a bunch of this.  In her landing she adds a Principal* to the code for UDPSocketChild/Parent.  She queried sicking about this since mtransport didn't have anything here currently, and was told it was fine to pass null (and null-check) for the Principal, since mtransport doesn't check it today.

So, I want to break this bug into two: one to move stuff to PBackground, and a second to resolve the existing null-Principal issue for mtransport.  

Also, I'm unsure if we need to test this against the principal; we don't have specific permissions-by-app for mtransport access today, so I'm unsure what doing this check would buy us. If anything, the issue here would be that in general UDPSocket is allowed (today) if a compromised child sends up a request with no principal.

> @@ +141,2 @@
> >    }
> > +  // XXX else { ? } for PBackground
> 
> You can verify the app permission as part of the principal checking step.

There really isn't app permission for mtransport aka ICE/TURN/etc.  We don't want to allow general UDPSocket use to a hacked child, so we may need to move a subset or most of the ICE code into the master, which is a *Major* refactor, in order to resolve this.  In any case, that's a bigger bug than this one (which is about performance).

> ::: dom/network/interfaces/nsIUDPSocketChild.idl
> @@ +26,5 @@
> >    readonly attribute AUTF8String localAddress;
> >    attribute AUTF8String filterName;
> >  
> > +  // Allow hosting this over PBackground instead of PNecko
> > +  void setBackground();
> 
> Yes, please mark this [noscript]
> 
> And like with the other method on UDPSocketChild I think this name needs to
> somehow indicate that it may spin the event loop.

Ok.  I can add a !MainThread() assertion as well

> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +1067,5 @@
> >      return;
> >    }
> >  
> >    socket_child_ = new nsMainThreadPtrHolder<nsIUDPSocketChild>(socketChild);
> > +  socket_child_->SetBackground();
> 
> This will spin the event loop while holding a monitor! Any other runnables
> posted could attempt to grab the same monitor and deadlock. You must drop
> the monitor before spinning here.

I'll check

NI-ing back to ensure this gets noticed
Flags: needinfo?(bent.mozilla)
See Also: → 1126232
> > @@ +141,2 @@
> > >    }
> > > +  // XXX else { ? } for PBackground
> > 
> > You can verify the app permission as part of the principal checking step.
> 
> There really isn't app permission for mtransport aka ICE/TURN/etc.  We don't
> want to allow general UDPSocket use to a hacked child, so we may need to
> move a subset or most of the ICE code into the master, which is a *Major*
> refactor, in order to resolve this.  In any case, that's a bigger bug than
> this one (which is about performance).

This principal issue is bug 1126232
Attachment #8599411 - Flags: review?(bent.mozilla)
Comment on attachment 8599411 [details] [diff] [review]
interdiffs from review (almost all to patch 2)

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

::: dom/network/UDPSocketChild.cpp
@@ +114,5 @@
>  {
>    using mozilla::ipc::BackgroundChild;
>  
> +  // Spinning the event loop in MainThread would be dangerous
> +  MOZ_ASSERT(!NS_IsMainThread());

Spinning the event loop is not just dangerous on the main thread (though it's especially dangerous there because it can let us reenter JS in scary ways). Basically this means events can run out of order.

I've only looked briefly here so I'm sure I've missed important details, but I think this scenario might race and fail without some additional logic:

1. (main thread) nr_socket_local_create(), creates NrSocketIpc object, queues NrSocketIpc::create_i() on the transport thread.
2. (transport thread) NrSocketIpc::create_i() runs, calls SetBackgroundSpinsEvents(), starts spinning the event loop.
3. (main thread) nr_socket_local_destroy(), queues NrSocketIpc::close_i() on the transport thread.
4. (transport thread) NrSocketIpc::close_i() runs, sees nothing in |socket_child_| and returns.
5. (transport thread) PBackground actor is finally created, NrSocketIpc::create_i() completes and you continue initializing. It's unclear to me if this would just leak or if it would crash somewhere or eventually clean itself up otherwise.

How do you guard against this case?

@@ +158,3 @@
>      existingBackgroundChild =
>        BackgroundChild::GetForCurrentThread();
> +    if (NS_WARN_IF(!existingBackgroundChild)) {

You should be able to assert this here.

@@ +181,5 @@
>    mSocket = aSocket;
>    AddIPDLReference();
>  
>    if (mChannel) {
> +    mozilla::ipc::PrincipalInfo principal_info = mozilla::ipc::NullPrincipalInfo();

A "null principal" is actually a real object with special restrictions and is not meant to represent the case where we have no principal. I think what you want here is to use OptionalPrincipalInfo and set the type void if aPrincipal is null.

@@ +183,5 @@
>  
>    if (mChannel) {
> +    mozilla::ipc::PrincipalInfo principal_info = mozilla::ipc::NullPrincipalInfo();
> +    if (aPrincipal) {
> +      nsresult rv = PrincipalToPrincipalInfo(aPrincipal,&principal_info);

Nit: Space before &
(I haven't finished going through the patch but wanted to get the event loop question in before I head out tonight)
> ::: dom/network/UDPSocketChild.cpp
> @@ +114,5 @@
> >  {
> >    using mozilla::ipc::BackgroundChild;
> >  
> > +  // Spinning the event loop in MainThread would be dangerous
> > +  MOZ_ASSERT(!NS_IsMainThread());
> 
> Spinning the event loop is not just dangerous on the main thread (though
> it's especially dangerous there because it can let us reenter JS in scary
> ways). Basically this means events can run out of order.
> 
> I've only looked briefly here so I'm sure I've missed important details, but
> I think this scenario might race and fail without some additional logic:
> 
> 1. (main thread) nr_socket_local_create(), creates NrSocketIpc object,
> queues NrSocketIpc::create_i() on the transport thread.

Actually this is STS thread, not mainthread.  Almost all mtransport code runs on sts unless otherwise mentioned. The "old" transport thread was in fact MainThread until this patch lands (create_i() was create_m() i.e. run-on-mainthread)

> 2. (transport thread) NrSocketIpc::create_i() runs, calls
> SetBackgroundSpinsEvents(), starts spinning the event loop.
> 3. (main thread) nr_socket_local_destroy(), queues NrSocketIpc::close_i() on
> the transport thread.
> 4. (transport thread) NrSocketIpc::close_i() runs, sees nothing in
> |socket_child_| and returns.
> 5. (transport thread) PBackground actor is finally created,
> NrSocketIpc::create_i() completes and you continue initializing. It's
> unclear to me if this would just leak or if it would crash somewhere or
> eventually clean itself up otherwise.
> 
> How do you guard against this case?

Right after dispatching create_i() to the transport thread:
Yes, this blocks STS thread in the content process entirely during this.  And yes, it did this before (but you'd have to wait on mainthread's event queue to process the create_m() call!)

  // Wait until socket creation complete.
  mon.Wait();
Comment on attachment 8599411 [details] [diff] [review]
interdiffs from review (almost all to patch 2)

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

r- to fix the null principal part and revert the other principal stuff below.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +224,2 @@
>  {
> +  MOZ_CRASH(); // "AllocPUDPSocket should not be called"

Nit: Pass that string as an arg to MOZ_CRASH?

::: ipc/glue/BackgroundParentImpl.cpp
@@ +255,5 @@
>  }
>  
> +namespace {
> +
> +class CheckPrincipalCallback : public nsRunnable

So since we're always sending no principal (currently NullPrincipal, but soon you'll change that to void_t) these changes are all basically unnecessary. Let's just revert them for now.
Attachment #8599411 - Flags: review?(bent.mozilla) → review-
(In reply to Randell Jesup [:jesup] from comment #52)
>   // Wait until socket creation complete.
>   mon.Wait();

Hrm. This is the part I was missing I guess. I wish we weren't having to do that, but oh well. I guess as long as the only thread that could send messages is blocked while the actor thread is spinning we should be ok.
Flags: needinfo?(bent.mozilla)
> @@ +181,5 @@
> >    mSocket = aSocket;
> >    AddIPDLReference();
> >  
> >    if (mChannel) {
> > +    mozilla::ipc::PrincipalInfo principal_info = mozilla::ipc::NullPrincipalInfo();
> 
> A "null principal" is actually a real object with special restrictions and
> is not meant to represent the case where we have no principal. I think what
> you want here is to use OptionalPrincipalInfo and set the type void if
> aPrincipal is null.

So I hear you, but figuring out the correct syntax for this is ... challenging ... and I can't find any use in the tree to illuminate it (one reference: "if (hostParams.principal().type() == OptionalPrincipalInfo::Tvoid_t) {" is all)

I presume PBackground.ipdl should have "PUDPSocket(OptionalPrincipalInfo pInfo, nsCString filter);"

But generating the code to call SendPUDPSocketConstructor() that will pass the compiler is hard.
I can build an OptionalPrincipalInfo(mozilla::void_t) and OptionalPrincipalInfo(principal_info), but SendPUDPSocketConstructor() doesn't seem to like this whole sequence.
Flags: needinfo?(bent.mozilla)
(In reply to Randell Jesup [:jesup] from comment #55)
> So I hear you, but figuring out the correct syntax for this is ...
> challenging ... 

Yeah :(

> if (hostParams.principal().type() == OptionalPrincipalInfo::Tvoid_t) {

That's it!

> I presume PBackground.ipdl should have "PUDPSocket(OptionalPrincipalInfo
> pInfo, nsCString filter);"

Right.

> but SendPUDPSocketConstructor() doesn't seem to like this whole sequence.

I think you basically just do:

  SendPUDPSocketConstructor(void_t(), filter);
Flags: needinfo?(bent.mozilla)
backlog: --- → webRTC+
Flags: firefox-backlog+
Ok, finally found the problem with non-e10s test_ipc and this patch (was perma-orange on Linux); it's a timing issue with MainThread opened by the need to touch Mainthread in RecvPUDPSocketConstructor.  This is solved for the mtransport/Null-principal case; it is not solved for a real principal as would exist if we moved all of UDPSocket to PBackground - they'd need to have RecvBind probably queue until the principal is finished being checked.  That will need to be handled in a followup bug.  Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77bc1ca31f35   If we want to ifdef or otherwise disable the non-null-principalinfo case in RecvPUDPSocketConstructor, that's fine.  I'll fill in the Bug Xxxxxxx values once I file, before landing
Attachment #8606837 - Flags: review?(bent.mozilla)
Attachment #8599411 - Attachment is obsolete: true
Comment on attachment 8606837 [details] [diff] [review]
interdiffs second review

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

So, I thought we had decided that we were not sending any principals yet... Did that change? I don't think it's a good idea to check in dead code that handles principals if we will never exercise it...

::: dom/network/UDPSocketChild.cpp
@@ +187,3 @@
>      if (aPrincipal) {
> +      mozilla::ipc::PrincipalInfo principal_info;
> +      nsresult rv = PrincipalToPrincipalInfo(aPrincipal, &principal_info);

After this succeeds it would be good to MOZ_ASSERT(principal_info.type() != PrincipalInfo::TNullPrincipal)

@@ +190,5 @@
>        if (NS_WARN_IF(NS_FAILED(rv))) {
>          return rv;
>        }
> +      mozilla::ipc::OptionalPrincipalInfo optional_principal(principal_info);
> +      mChannel->SendPUDPSocketConstructor(this, optional_principal,

Nit: Just pass |principalInfo| directly here, no need for the temporary stack var.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +426,1 @@
>    nsRefPtr<InitUDPSocketParentCallback> callback =

Hm, I don't see where this is defined...

@@ +426,5 @@
>    nsRefPtr<InitUDPSocketParentCallback> callback =
>      new InitUDPSocketParentCallback(static_cast<UDPSocketParent*>(aActor),
>                                      aFilter);
>  
> +  if (aOptionalPrincipal.type() != OptionalPrincipalInfo::Tvoid_t) {

I think it makes more sense to check that |aOptionalPrincipal.type() == OptionalPrincipalInfo::TPrincipalInfo| here.

@@ +427,5 @@
>      new InitUDPSocketParentCallback(static_cast<UDPSocketParent*>(aActor),
>                                      aFilter);
>  
> +  if (aOptionalPrincipal.type() != OptionalPrincipalInfo::Tvoid_t) {
> +    // If the ContentParent is null we are dealing with a same-process actor.

Nit: Explain why this is relevant (e.g. "we always trust the main process" or something).

@@ +430,5 @@
> +  if (aOptionalPrincipal.type() != OptionalPrincipalInfo::Tvoid_t) {
> +    // If the ContentParent is null we are dealing with a same-process actor.
> +    nsRefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this);
> +    if (!parent) {
> +      MOZ_ASSERT(PrincipalInfo(aOptionalPrincipal).type() != PrincipalInfo::TNullPrincipalInfo);

MOZ_ASSERT(aOptionalPrincipal.get_PrincipalInfo().type() !=
           PrincipalInfo::TNullPrincipalInfo);

@@ +432,5 @@
> +    nsRefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this);
> +    if (!parent) {
> +      MOZ_ASSERT(PrincipalInfo(aOptionalPrincipal).type() != PrincipalInfo::TNullPrincipalInfo);
> +      // we can't release parent here!
> +      unused << parent.forget();

This doesn't make sense, you just branched on |parent| being null.

@@ +436,5 @@
> +      unused << parent.forget();
> +      return true;
> +    }
> +
> +    nsRefPtr<CheckPrincipalWithCallbackRunnable> runnable =

If this principal is coming from another process then it can send any data it wants, including a null principal. Since you don't allow that on the sending side you can enforce that right away here:

  if (aOptionalPrincipal.get_PrincipalInfo().type() == PrincipalInfo::TNullPrincipalInfo) {
    return false;
  }

That will kill any child process that sent bad data.

@@ +437,5 @@
> +      return true;
> +    }
> +
> +    nsRefPtr<CheckPrincipalWithCallbackRunnable> runnable =
> +      new CheckPrincipalWithCallbackRunnable(parent.forget(), PrincipalInfo(aOptionalPrincipal),

s/PrincipalInfo(aOptionalPrincipal)/aOptionalPrincipal.get_PrincipalInfo()/

@@ +441,5 @@
> +      new CheckPrincipalWithCallbackRunnable(parent.forget(), PrincipalInfo(aOptionalPrincipal),
> +                                             callback);
> +    // Note: this will likely fail since this is a synchronous call, and we haven't finished checking
> +    // the principal.  To make this code work RecvBind() likely needs to stall returning a port until
> +    // the principal checking is complete.  See Bug Xxxxxx

This seems really undesirable... Why would we check in code that we know is racy and explicitly ignore the race?

@@ +445,5 @@
> +    // the principal checking is complete.  See Bug Xxxxxx
> +    nsresult rv = NS_DispatchToMainThread(runnable);
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));
> +  } else {
> +    // No principal - This must be from mtransport (WebRTC/ICE) - see bug Xxxxxx - we want to do this:

Nit: Need a real bug number here. And don't leave commented-out code in here, just put it in the bug.
Attachment #8606837 - Flags: review?(bent.mozilla) → review-
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #59)
> Comment on attachment 8606837 [details] [diff] [review]
> interdiffs second review
> 
> Review of attachment 8606837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, I thought we had decided that we were not sending any principals yet...
> Did that change? I don't think it's a good idea to check in dead code that
> handles principals if we will never exercise it...

I'll push that off to a followup bug for "Support PUDPBackground for general use of UDPSocket" bug, and put the parts here that apply to that into a separate patch and just leave some comments/stubs

> ::: ipc/glue/BackgroundParentImpl.cpp
> @@ +426,1 @@
> >    nsRefPtr<InitUDPSocketParentCallback> callback =
> 
> Hm, I don't see where this is defined...

Line 381 in the patched file

> @@ +432,5 @@
> > +    nsRefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this);
> > +    if (!parent) {
> > +      MOZ_ASSERT(PrincipalInfo(aOptionalPrincipal).type() != PrincipalInfo::TNullPrincipalInfo);
> > +      // we can't release parent here!
> > +      unused << parent.forget();
> 
> This doesn't make sense, you just branched on |parent| being null.

Good point!  (Left over from older version)
 
> @@ +436,5 @@
> > +      unused << parent.forget();
> > +      return true;
> > +    }
> > +
> > +    nsRefPtr<CheckPrincipalWithCallbackRunnable> runnable =
> 
> If this principal is coming from another process then it can send any data
> it wants, including a null principal. Since you don't allow that on the
> sending side you can enforce that right away here:
> 
>   if (aOptionalPrincipal.get_PrincipalInfo().type() ==
> PrincipalInfo::TNullPrincipalInfo) {
>     return false;
>   }
> 
> That will kill any child process that sent bad data.

We're inside "if (aOptionalPrincipal.type() == OptionalPrincipalInfo::TPrincipalInfo)" so that if wouldn't do anything.

> @@ +441,5 @@
> > +      new CheckPrincipalWithCallbackRunnable(parent.forget(), PrincipalInfo(aOptionalPrincipal),
> > +                                             callback);
> > +    // Note: this will likely fail since this is a synchronous call, and we haven't finished checking
> > +    // the principal.  To make this code work RecvBind() likely needs to stall returning a port until
> > +    // the principal checking is complete.  See Bug Xxxxxx
> 
> This seems really undesirable... Why would we check in code that we know is
> racy and explicitly ignore the race?

This will be part of the follow-on non-mtransport patch

> @@ +445,5 @@
> > +    // the principal checking is complete.  See Bug Xxxxxx
> > +    nsresult rv = NS_DispatchToMainThread(runnable);
> > +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));
> > +  } else {
> > +    // No principal - This must be from mtransport (WebRTC/ICE) - see bug Xxxxxx - we want to do this:
> 
> Nit: Need a real bug number here. And don't leave commented-out code in
> here, just put it in the bug.

yup
Blocks: 1167039
rollup of part 2 with all modifications.  Support for non-mtransport use of UDPSocket pushed off to another bug; fails any use of a non-Null principal, and for Null principals it verifies that the security filter is 'stun'.
Attachment #8608773 - Flags: review?(bent.mozilla)
Attachment #8580795 - Attachment is obsolete: true
(In reply to Randell Jesup [:jesup] from comment #60)
> > >    nsRefPtr<InitUDPSocketParentCallback> callback =
> > 
> > Hm, I don't see where this is defined...
> 
> Line 381 in the patched file

Huh? The patch I reviewed didn't have any definition for InitUDPSocketParentCallback. Are you talking about another patch?

> >   if (aOptionalPrincipal.get_PrincipalInfo().type() ==
> > PrincipalInfo::TNullPrincipalInfo) {
> >     return false;
> >   }
> We're inside "if (aOptionalPrincipal.type() ==
> OptionalPrincipalInfo::TPrincipalInfo)" so that if wouldn't do anything.

No, look closer: |aOptionalPrincipal.type() == OptionalPrincipalInfo::TPrincipalInfo| means that you were passed a real PrincipalInfo, not void_t. |aOptionalPrincipal.get_PrincipalInfo().type() == PrincipalInfo::TNullPrincipalInfo| means that the real PrincipalInfo you were passed is not the special NullPrincipalInfo.
> is not the special NullPrincipalInfo.

s/not//
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #62)
> (In reply to Randell Jesup [:jesup] from comment #60)
> > > >    nsRefPtr<InitUDPSocketParentCallback> callback =
> > > 
> > > Hm, I don't see where this is defined...
> > 
> > Line 381 in the patched file

> Huh? The patch I reviewed didn't have any definition for
> InitUDPSocketParentCallback. Are you talking about another patch?

Perhaps you were only looking at the interdiffs; in any case the rollup patch up for review now includes it.
 
> > >   if (aOptionalPrincipal.get_PrincipalInfo().type() ==
> > > PrincipalInfo::TNullPrincipalInfo) {
> > >     return false;
> > >   }
> > We're inside "if (aOptionalPrincipal.type() ==
> > OptionalPrincipalInfo::TPrincipalInfo)" so that if wouldn't do anything.
> 
> No, look closer: |aOptionalPrincipal.type() ==
> OptionalPrincipalInfo::TPrincipalInfo| means that you were passed a real
> PrincipalInfo, not void_t. |aOptionalPrincipal.get_PrincipalInfo().type() ==
> PrincipalInfo::TNullPrincipalInfo| means that the real PrincipalInfo you
> were passed is not the special NullPrincipalInfo.

Aha.  Ok; though it's non-intuitive that .type() could == ::TPrincipalInfo *and* == ::TNullPrincipalInfo (of course == can make soup for you if it's overloaded).  However, in the revised patch up to you now, this is all pushed off to bug 1167039, and we just return 'false' for all real PricipalInfo's.
Comment on attachment 8608773 [details] [diff] [review]
Part 2: Sharing UDPSocket between PNecko and PBackground

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

This is mostly nits but there are a few bigger things here, dead code, etc.

::: dom/network/UDPSocketChild.cpp
@@ +86,5 @@
> +  explicit UDPSocketBackgroundChildCallback(bool* aDone)
> +  : mDone(aDone)
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    MOZ_ASSERT(mDone);

Should also MOZ_ASSERT(!*mDone)?

@@ +134,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +
> +  if (NS_WARN_IF(!BackgroundChild::GetForCurrentThread())) {

You have to call this anyway in SetBackgroundSpinsEvents() right after you call CreatePBackgroundSpinUntilDone() so just remove this redundant call?

@@ +191,5 @@
> +        return rv;
> +      }
> +      MOZ_ASSERT(principal_info.type() != mozilla::ipc::PrincipalInfo::TNullPrincipalInfo);
> +      mChannel->SendPUDPSocketConstructor(this, principal_info,
> +                                          mFilterName);

This should all be dead code, please remove it.

Just do:

  if (mChannel) {
    mChannel->SendPUDPSocketConstructor(this, void_t(), mFilterName);
  } else {
    // ...
  }

::: dom/network/UDPSocketChild.h
@@ +56,5 @@
>    nsresult SendDataInternal(const UDPSocketAddr& aAddr,
>                              const uint8_t* aData,
>                              const uint32_t aByteLength);
>  
> +  mozilla::ipc::PBackgroundChild *mChannel;

Nit: * on the left, and maybe call it "mBackgroundManager"? Or something to that effect? "mChannel" is vague and overloaded with necko channels.

::: dom/network/UDPSocketParent.cpp
@@ +41,5 @@
> +  , mNeckoManager(nullptr)
> +  , mIPCOpen(true)
> +{
> +  mObserver = new mozilla::net::OfflineObserver(this);
> +  unused << mBackgroundManager;

This doesn't do anything.

@@ +98,2 @@
>  bool
>  UDPSocketParent::Init(const IPC::Principal& aPrincipal,

I would assert here |MOZ_ASSERT_IF(mBackgroundManager, !aPrincipal)| because if you ever do start sending a principal in suddenly you will be touching main-thread-only things (like permission manager) off the main thread.

@@ +108,5 @@
> +        return false;
> +      }
> +    } else {
> +      // Verified AssertAppPrincipal in BackgroundParentImpl.cpp before calling this
> +      // via a callback.

This comment isn't really true... And since aPrincipal is always null you shouldn't ever get here anyway.

::: dom/network/UDPSocketParent.h
@@ +13,5 @@
>  #include "nsIUDPSocketFilter.h"
>  #include "mozilla/net/OfflineObserver.h"
>  #include "mozilla/dom/PermissionMessageUtils.h"
>  
> +class PNeckoParent;

This needs to be wrapped in |namespace mozilla { namespace net {|. You're forward-declaring a different class.

@@ +27,5 @@
>    NS_DECL_THREADSAFE_ISUPPORTS
>    NS_DECL_NSIUDPSOCKETLISTENER
>  
> +  MOZ_IMPLICIT UDPSocketParent(PBackgroundParent* aManager);
> +  MOZ_IMPLICIT UDPSocketParent(PNeckoParent* aManager);

These shouldn't be implicit I don't think... When do you ever pass a |PBackgroundParent*| and expect it to magically become a |UDPSocketParent|?

I think you should just mark them as explicit.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +20,5 @@
>  #include "nsID.h"
>  #include "nsTraceRefcnt.h"
>  
> +using mozilla::dom::UDPSocketChild;
> +using mozilla::net::PUDPSocketChild;

Nit: Move this below into the block with the other using declarations to prevent leaking into other UNIFIED_SOURCES.

@@ +222,5 @@
> +BackgroundChildImpl::AllocPUDPSocketChild(const OptionalPrincipalInfo& aPrincipalInfo,
> +                                          const nsCString& aFilter)
> +{
> +  MOZ_CRASH("AllocPUDPSocket should not be called");
> +  return nullptr;

Nit: This shouldn't be needed, MOZ_CRASH is marked as noreturn

::: ipc/glue/BackgroundParentImpl.cpp
@@ +8,5 @@
>  #include "FileDescriptorSetParent.h"
>  #include "mozilla/media/MediaParent.h"
>  #include "mozilla/AppProcessChecker.h"
>  #include "mozilla/Assertions.h"
> +#include "mozilla/unused.h"

I don't think you use this here, do you?

@@ +256,5 @@
>  }
>  
> +namespace {
> +
> +class CheckPrincipalCallback : public nsRunnable

Since you've only got one class that is derived from this why not just combine the classes?

@@ +261,5 @@
> +{
> +public:
> +  CheckPrincipalCallback() {}
> +
> +  void SetPrincipal(nsIPrincipal* aPrincipal)

This is dead code.

@@ +266,5 @@
> +  {
> +    mPrincipal = aPrincipal;
> +  }
> +
> +protected:

Nit: Need a protected virtual destructor here if you keep this as a base class.

@@ +270,5 @@
> +protected:
> +  nsCOMPtr<nsIPrincipal> mPrincipal;
> +};
> +
> +class CheckPrincipalWithCallbackRunnable final : public nsRunnable

Can you revert these changes (I think you just moved the class)? We shouldn't change blame here.

@@ +318,5 @@
> +};
> +
> +} // anonymous namespace
> +
> +class InitUDPSocketParentCallback final : public CheckPrincipalCallback

Nit: This should live in the anonymous namespace too.

@@ +321,5 @@
> +
> +class InitUDPSocketParentCallback final : public CheckPrincipalCallback
> +{
> +public:
> +  explicit InitUDPSocketParentCallback(UDPSocketParent* aActor,

Nit: No need for explicit on constructors that take more than one arg.

@@ +335,5 @@
> +  Run()
> +  {
> +    AssertIsInMainProcess();
> +
> +    IPC::Principal principal(mPrincipal);;

Nit: Just pass a temporary IPC::Principal? It defaults to having a null value, and you know mPrincipal is always null anyway...

@@ +336,5 @@
> +  {
> +    AssertIsInMainProcess();
> +
> +    IPC::Principal principal(mPrincipal);;
> +    mActor->Init(principal, mFilter); // result?

Hrm, how do you intend to return the return value from Init() to the caller? If Init() fails then you will have a half-baked actor that will probably crash later on...

You pass a null principal now, and you know mFilter is a value that we support, so that might be ok for now, but you should mark it as MOZ_ALWAYS_TRUE at least. Maybe even explicitly MOZ_CRASH if that ever returns false, because you have no way to handle that here.

@@ +340,5 @@
> +    mActor->Init(principal, mFilter); // result?
> +    return NS_OK;
> +  }
> +
> +private:

Nit: Please give this a private destructor since it is reference-counted.

@@ +371,5 @@
> +  if (aOptionalPrincipal.type() == OptionalPrincipalInfo::TPrincipalInfo) {
> +    // Support for checking principals (for non-mtransport use) will be handled in
> +    // bug 1167039
> +    return false;
> +  } else {

Nit: no else after return statements.

@@ +383,5 @@
> +    // they aren't STUN packets until a STUN response is seen.
> +    if (!aFilter.EqualsASCII("stun")) {
> +      return false;
> +    }
> +    callback->Run();

Seems to me that it would be far simpler (and better for error handling) to just do away with the InitUDPSocketParentCallback class entirely and just do:

  return aActor->Init(principal, mFilter);
Attachment #8608773 - Flags: review?(bent.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: