Label IPC actors in necko

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking
P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: kershaw, Assigned: kershaw)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [necko-active][QDL][TDC-MVP][NECKO])

Attachments

(7 attachments, 10 obsolete attachments)

43.70 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
11.18 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
9.00 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
2.28 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
5.65 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
2.72 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
4.58 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
Like the modification of HttpChannelChild in bug 1318506, this bug should label other IPC actors [1].

[1] http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/netwerk/ipc/PNecko.ipdl#47-61
Whiteboard: [necko-next] → [necko-next][QDL][TDC-MVP][NECKO]

Updated

11 months ago
Priority: -- → P1
(Assignee)

Comment 1

10 months ago
Created attachment 8867067 [details] [diff] [review]
Part1: Refactoring for mNeckoTarget and  ChannelEvent

Summary:
 - Add new class NeckoTargetHolder to hold mNeckoTarget.
 - Add new class NeckoTargetChannelEvent.
 - Remove some repeated code in HttpChannelChild and FtpChannelChild
Attachment #8867067 - Flags: review?(honzab.moz)
(Assignee)

Comment 2

10 months ago
Created attachment 8867069 [details] [diff] [review]
Part2: Set event target for WyciwygChannelChild

Summary:
 - Modify WyciwygChannelChild's constructor to receive an event target
 - Use NeckoTargetChannelEvent in WyciwygChannelChild
Assignee: nobody → kechang
Status: NEW → ASSIGNED
Attachment #8867069 - Flags: review?(honzab.moz)
(Assignee)

Updated

10 months ago
Whiteboard: [necko-next][QDL][TDC-MVP][NECKO] → [necko-active][QDL][TDC-MVP][NECKO]
(Assignee)

Comment 3

10 months ago
Created attachment 8867085 [details] [diff] [review]
Part3: Set event target for TCPSocketChild  and TCPServerSocketChild

Summary:
 - Add an event target parameter in TCPSocketChild's and TCPServerSocketChild's constructor
 - Call gNeckoChild->SetEventTargetForActor before sending IPC constructor message

Thanks.
Attachment #8867085 - Flags: review?(josh)

Comment 4

10 months ago
Comment on attachment 8867085 [details] [diff] [review]
Part3: Set event target for TCPSocketChild  and TCPServerSocketChild

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

::: dom/network/TCPServerSocket.cpp
@@ +63,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    nsCOMPtr<nsIEventTarget> target = nullptr;

No need for the `= nullptr`.

::: dom/network/TCPSocket.cpp
@@ +270,5 @@
>  
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>      mReadyState = TCPReadyState::Connecting;
> +
> +    nsCOMPtr<nsIEventTarget> target = nullptr;

No need for `= nullptr;`

::: media/mtransport/nr_socket_prsock.cpp
@@ +1973,5 @@
>  
> +  dom::TCPSocketChild* child =
> +    new dom::TCPSocketChild(NS_ConvertUTF8toUTF16(remote_addr),
> +                            remote_port,
> +                            nullptr);

You may want to check whether this makes sense with the webrtc folks, since this code is used for the e10s webrtc implementation.
Attachment #8867085 - Flags: review?(josh) → review+
Comment on attachment 8867067 [details] [diff] [review]
Part1: Refactoring for mNeckoTarget and  ChannelEvent

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

automatic r- for missing comments on new classes

::: netwerk/ipc/ChannelEventQueue.h
@@ +45,5 @@
>    }
>  };
>  
> +template<typename T>
> +class NeckoTargetChannelEvent : public ChannelEvent

I really can't tolerate total omission of any comments on new classes, sorry.

@@ +61,5 @@
> +  {
> +    MOZ_ASSERT(mChild);
> +
> +    nsCOMPtr<nsIEventTarget> target = mChild->GetNeckoTarget();
> +    return target.forget();

nit: can't return directly mChild->GetNeckoTarget();?

@@ +65,5 @@
> +    return target.forget();
> +  }
> +
> +protected:
> +  T *mChild;

how is the lifetime preserved?

::: netwerk/ipc/NeckoTargetHolder.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class NeckoTargetHolder

again, no comments.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +176,5 @@
>    AsyncCall(void (HttpChannelChild::*funcPtr)(),
>              nsRunnableMethod<HttpChannelChild> **retval = nullptr) override;
>  
> +  // Get event target for processing network events.
> +  already_AddRefed<nsIEventTarget> GetNeckoTarget() override;

where is this overridden?
Attachment #8867067 - Flags: review?(honzab.moz) → review-
Attachment #8867069 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 6

9 months ago
(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8867067 [details] [diff] [review]
> Part1: Refactoring for mNeckoTarget and  ChannelEvent
> 
> Review of attachment 8867067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> automatic r- for missing comments on new classes
> 
> ::: netwerk/ipc/ChannelEventQueue.h
> @@ +45,5 @@
> >    }
> >  };
> >  
> > +template<typename T>
> > +class NeckoTargetChannelEvent : public ChannelEvent
> 
> I really can't tolerate total omission of any comments on new classes, sorry.
> 

Sorry, I'll add comments in next version.

> @@ +61,5 @@
> > +  {
> > +    MOZ_ASSERT(mChild);
> > +
> > +    nsCOMPtr<nsIEventTarget> target = mChild->GetNeckoTarget();
> > +    return target.forget();
> 
> nit: can't return directly mChild->GetNeckoTarget();?
> 

Yes, I think we can. Thanks.

> @@ +65,5 @@
> > +    return target.forget();
> > +  }
> > +
> > +protected:
> > +  T *mChild;
> 
> how is the lifetime preserved?
> 

We can only use raw ptr here to avoid ref count cycle.
If I am not wrong, child channels should outlive all channel events, so we don't have to worry about the lifetime here?

> ::: netwerk/ipc/NeckoTargetHolder.h
> @@ +13,5 @@
> > +
> > +namespace mozilla {
> > +namespace net {
> > +
> > +class NeckoTargetHolder
> 
> again, no comments.
> 

I'll add in next version.

> ::: netwerk/protocol/http/HttpChannelChild.h
> @@ +176,5 @@
> >    AsyncCall(void (HttpChannelChild::*funcPtr)(),
> >              nsRunnableMethod<HttpChannelChild> **retval = nullptr) override;
> >  
> > +  // Get event target for processing network events.
> > +  already_AddRefed<nsIEventTarget> GetNeckoTarget() override;
> 
> where is this overridden?

Not quite understand your question. It's overridden in http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/netwerk/protocol/http/HttpChannelChild.cpp#2266.
(Assignee)

Comment 7

9 months ago
Created attachment 8868835 [details] [diff] [review]
Part1: Refactoring for mNeckoTarget and  ChannelEvent, v2

Summary:
 - Add comments for new classes.
 - Add an assertion in MainThreadChannelEvent::GetEventTarget() to make sure this should be only called in parent process.
Attachment #8867067 - Attachment is obsolete: true
Attachment #8868835 - Flags: review?(honzab.moz)
(Assignee)

Comment 8

9 months ago
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +1973,5 @@
> >  
> > +  dom::TCPSocketChild* child =
> > +    new dom::TCPSocketChild(NS_ConvertUTF8toUTF16(remote_addr),
> > +                            remote_port,
> > +                            nullptr);
> 
> You may want to check whether this makes sense with the webrtc folks, since
> this code is used for the e10s webrtc implementation.

I think we can use SystemGroup here because nsITCPSocketCallback implemented in nr_socket_prsock.cpp doesn't touch any content script.
(Assignee)

Comment 9

9 months ago
Created attachment 8870249 [details] [diff] [review]
Part 3-1: Use system group's event target  for TCPSocketChild's constructor

Just for safe, I'd like to have a webrtc expert to have a look.

:bwc, could you please take a look at this patch? Thanks.
Attachment #8870249 - Flags: review?(docfaraday)
(Assignee)

Comment 10

9 months ago
Hi Bill,

I have a question about do we really need to label all IPC actors even if some of them doing nothing in child process?

For example, PDataChannelChild [1] doesn't expect to receive any IPC message from parent process. Do we still have to call SetEventTargetForActor brfore sending the constructor message?

Thanks.

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/netwerk/ipc/PDataChannel.ipdl
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 11

9 months ago
Created attachment 8870376 [details] [diff] [review]
Part1: Refactoring for mNeckoTarget and  ChannelEvent, v3

Update the patch since that I didn't add "MOZ_COUNT_CTOR(NeckoTargetHolder)" in NeckoTargetHolder's constructor.
Attachment #8868835 - Attachment is obsolete: true
Attachment #8868835 - Flags: review?(honzab.moz)
Attachment #8870376 - Flags: review?(honzab.moz)

Comment 12

9 months ago
Comment on attachment 8870249 [details] [diff] [review]
Part 3-1: Use system group's event target  for TCPSocketChild's constructor

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

Is this going to change what thread calls the various nsITCPSocketCallback functions? Right now we assume that these callbacks happen on the same thread that the TCPSocketChild was created on.
Attachment #8870249 - Flags: review?(docfaraday)

Updated

9 months ago
Flags: needinfo?(kechang)
(Assignee)

Comment 13

9 months ago
(In reply to Byron Campen [:bwc] from comment #12)
> Comment on attachment 8870249 [details] [diff] [review]
> Part 3-1: Use system group's event target  for TCPSocketChild's constructor
> 
> Review of attachment 8870249 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this going to change what thread calls the various nsITCPSocketCallback
> functions? Right now we assume that these callbacks happen on the same
> thread that the TCPSocketChild was created on.

Yes, you are right. This could change what thread calls nsITCPSocketCallback functions.
SystemGroup should only be used when is created on main thread.

I'll update the patch and ask you to review again. Thanks.
Flags: needinfo?(kechang)
No, there's no need to label PDataChannel since no messages for it are sent to the content process.
Flags: needinfo?(wmccloskey)
(In reply to Kershaw Chang [:kershaw] from comment #6)
> > > +  T *mChild;
> > 
> > how is the lifetime preserved?
> > 
> 
> We can only use raw ptr here to avoid ref count cycle.
> If I am not wrong, child channels should outlive all channel events, so we
> don't have to worry about the lifetime here?

Yes.  This is all around the IPC code already, so we would already be crashing a lot.

> > > +  already_AddRefed<nsIEventTarget> GetNeckoTarget() override;
> > 
> > where is this overridden?
> 
> Not quite understand your question. It's overridden in
> http://searchfox.org/mozilla-central/rev/
> f55349994fdac101d121b11dac769f3f17fbec4b/netwerk/protocol/http/
> HttpChannelChild.cpp#2266.

Ah!!!  Got it.  This is turned by your patch to a virtual method, yes.  OK.
Comment on attachment 8870376 [details] [diff] [review]
Part1: Refactoring for mNeckoTarget and  ChannelEvent, v3

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

Nice work, thanks.

Sorry for the delay!

::: netwerk/ipc/ChannelEventQueue.h
@@ +51,5 @@
>  };
>  
> +// This event is designed to be only used for e10s child channels.
> +// The goal is to force the child channel to implement GetNeckoTarget()
> +// which should returns a labeled main thread event target so that this

should return

@@ +77,5 @@
> +  }
> +
> +protected:
> +  T *mChild;
> +

remove this blank line please.

::: netwerk/ipc/NeckoTargetHolder.h
@@ +21,5 @@
> +public:
> +  explicit NeckoTargetHolder(nsIEventTarget *aNeckoTarget)
> +    : mNeckoTarget(aNeckoTarget)
> +  {
> +    MOZ_COUNT_CTOR(NeckoTargetHolder);

to be honest, if this is always only derived from and all implementers are refcounted via nsisupports macros, then this is not needed.
Attachment #8870376 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 17

9 months ago
Comment on attachment 8870249 [details] [diff] [review]
Part 3-1: Use system group's event target  for TCPSocketChild's constructor

(In reply to Byron Campen [:bwc] from comment #12)
> Comment on attachment 8870249 [details] [diff] [review]
> Part 3-1: Use system group's event target  for TCPSocketChild's constructor
> 
> Review of attachment 8870249 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this going to change what thread calls the various nsITCPSocketCallback
> functions? Right now we assume that these callbacks happen on the same
> thread that the TCPSocketChild was created on.

According to [1], it looks like TCPSocketChild here is only created on main thread. That means this patch should be fine.

:bwc, could you take a look again? Thanks.

[1] http://searchfox.org/mozilla-central/rev/66d9eb3103e429127c85a7921e16c5a02458a127/media/mtransport/nr_socket_prsock.cpp#2169-2171
Attachment #8870249 - Flags: review?(docfaraday)

Comment 18

9 months ago
(In reply to Kershaw Chang [:kershaw] from comment #17)
> Comment on attachment 8870249 [details] [diff] [review]
> Part 3-1: Use system group's event target  for TCPSocketChild's constructor
> 
> (In reply to Byron Campen [:bwc] from comment #12)
> > Comment on attachment 8870249 [details] [diff] [review]
> > Part 3-1: Use system group's event target  for TCPSocketChild's constructor
> > 
> > Review of attachment 8870249 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Is this going to change what thread calls the various nsITCPSocketCallback
> > functions? Right now we assume that these callbacks happen on the same
> > thread that the TCPSocketChild was created on.
> 
> According to [1], it looks like TCPSocketChild here is only created on main
> thread. That means this patch should be fine.
> 
> :bwc, could you take a look again? Thanks.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 66d9eb3103e429127c85a7921e16c5a02458a127/media/mtransport/nr_socket_prsock.
> cpp#2169-2171

No, TCPSocketChild is created here: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp?q=nr_socket_prsock.cpp&redirect_type=direct#1974

Updated

9 months ago
Attachment #8870249 - Flags: review?(docfaraday)
(Assignee)

Comment 19

9 months ago
(In reply to Byron Campen [:bwc] from comment #18)
> (In reply to Kershaw Chang [:kershaw] from comment #17)
> > Comment on attachment 8870249 [details] [diff] [review]
> > Part 3-1: Use system group's event target  for TCPSocketChild's constructor
> > 
> > (In reply to Byron Campen [:bwc] from comment #12)
> > > Comment on attachment 8870249 [details] [diff] [review]
> > > Part 3-1: Use system group's event target  for TCPSocketChild's constructor
> > > 
> > > Review of attachment 8870249 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Is this going to change what thread calls the various nsITCPSocketCallback
> > > functions? Right now we assume that these callbacks happen on the same
> > > thread that the TCPSocketChild was created on.
> > 
> > According to [1], it looks like TCPSocketChild here is only created on main
> > thread. That means this patch should be fine.
> > 
> > :bwc, could you take a look again? Thanks.
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > 66d9eb3103e429127c85a7921e16c5a02458a127/media/mtransport/nr_socket_prsock.
> > cpp#2169-2171
> 
> No, TCPSocketChild is created here:
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.cpp?q=nr_socket_prsock.cpp&redirect_type=direct#1974

Sorry that I didn't explain clearly before.

Yes, TCPSocketChild is created in NrTcpSocketIpc::connect_i at [1]. But io_thread_ is initialized in NrTcpSocketIpc's constructor at [2]. As I said in comment#17, NrTcpSocketIpc is created at [3].
This means that io_thread_ should be main thread so that TCPSocketChild is also created on main thread.

Could you correct me if I missed something? Thanks.

[1] http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/media/mtransport/nr_socket_prsock.cpp#1974
[2] http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/media/mtransport/nr_socket_prsock.cpp#1690
[3] http://searchfox.org/mozilla-central/rev/66d9eb3103e429127c85a7921e16c5a02458a127/media/mtransport/nr_socket_prsock.cpp#2169-2171
Flags: needinfo?(docfaraday)

Comment 20

9 months ago
Hmm, that is not what I was expecting. In the UDP case io_thread_ is a different thread.
Flags: needinfo?(docfaraday)

Comment 21

9 months ago
Comment on attachment 8870249 [details] [diff] [review]
Part 3-1: Use system group's event target  for TCPSocketChild's constructor

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

::: media/mtransport/nr_socket_prsock.cpp
@@ +1968,5 @@
>                                 uint16_t remote_port,
>                                 const nsACString &local_addr,
>                                 uint16_t local_port,
>                                 const nsACString &tls_host) {
>    ASSERT_ON_THREAD(io_thread_);

Let's also assert we're on the main thread, and add a comment that this needs to be true because of how we're setting the event target below.
Attachment #8870249 - Flags: review+
(Assignee)

Comment 22

9 months ago
Created attachment 8874321 [details] [diff] [review]
Part4: Pass a labeled main thread event target to nsIUDPSocketChild::bind

Summary:
 - Add a new parameter (mainThreadTarget) in nsIUDPSocketChild::bind
 - In UDPSocket::InitRemote, get an event target and pass it via nsIUDPSocketChild::bind.
 - Note that in NrUdpSocketIpc, UdpSocketChild is created on sts thread, so it is ok to pass nullptr.
Attachment #8874321 - Flags: review?(josh)
(Assignee)

Comment 23

9 months ago
Created attachment 8874323 [details] [diff] [review]
Part5 : Label PAltDataOutputStreamChild

Summary:
 - Use a system group event target to label PAltDataOutputStreamChild

Thanks.
Attachment #8874323 - Flags: review?(honzab.moz)
(Assignee)

Comment 24

9 months ago
Created attachment 8874324 [details] [diff] [review]
Part6: Pass a labeled event target to StunAddrsRequestChild's constructor

Summary:
 - Add a new parameter (mainThreadTarget) in StunAddrsRequestChild's constructor
 - Get an labeled event target from mParent's window and use it.
Attachment #8874324 - Flags: review?(docfaraday)
Comment on attachment 8874323 [details] [diff] [review]
Part5 : Label PAltDataOutputStreamChild

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

::: netwerk/ipc/NeckoChild.cpp
@@ +124,5 @@
> +
> +  // Label PAltDataOutputStream actor's messages as SystemGroup.
> +  gNeckoChild->SetEventTargetForActor(
> +    stream,
> +    SystemGroup::EventTargetFor(TaskCategory::Other));

can we cast channel to HttpChannelChild and find the target there?  The sys group is definitely under-performing  here.

Comment 26

9 months ago
Comment on attachment 8874321 [details] [diff] [review]
Part4: Pass a labeled main thread event target to nsIUDPSocketChild::bind

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

Do we pass a non-null argument to bind in a later patch?
Attachment #8874321 - Flags: review?(josh) → review+
(Assignee)

Comment 27

9 months ago
(In reply to Honza Bambas (:mayhemer) from comment #25)
> Comment on attachment 8874323 [details] [diff] [review]
> Part5 : Label PAltDataOutputStreamChild
> 
> Review of attachment 8874323 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/ipc/NeckoChild.cpp
> @@ +124,5 @@
> > +
> > +  // Label PAltDataOutputStream actor's messages as SystemGroup.
> > +  gNeckoChild->SetEventTargetForActor(
> > +    stream,
> > +    SystemGroup::EventTargetFor(TaskCategory::Other));
> 
> can we cast channel to HttpChannelChild and find the target there?  The sys
> group is definitely under-performing  here.

I don't exactly understand "under-performing" you mean, sorry for my bad English.
But, I think I can move the code to [1]. I think that's what you'd like to see, right?

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/netwerk/protocol/http/HttpChannelChild.cpp#2765
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 28

9 months ago
(In reply to Josh Matthews [:jdm] from comment #26)
> Comment on attachment 8874321 [details] [diff] [review]
> Part4: Pass a labeled main thread event target to nsIUDPSocketChild::bind
> 
> Review of attachment 8874321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we pass a non-null argument to bind in a later patch?

Do you mean pass a non-null argument here [1]?
If yes, there is no need to do this because UDPSocketChild at [1] is not created on main thread. 

[1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/media/mtransport/nr_socket_prsock.cpp#1578
(In reply to Kershaw Chang [:kershaw] from comment #27)
> (In reply to Honza Bambas (:mayhemer) from comment #25)
> > Comment on attachment 8874323 [details] [diff] [review]
> > Part5 : Label PAltDataOutputStreamChild
> > 
> > Review of attachment 8874323 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/ipc/NeckoChild.cpp
> > @@ +124,5 @@
> > > +
> > > +  // Label PAltDataOutputStream actor's messages as SystemGroup.
> > > +  gNeckoChild->SetEventTargetForActor(
> > > +    stream,
> > > +    SystemGroup::EventTargetFor(TaskCategory::Other));
> > 
> > can we cast channel to HttpChannelChild and find the target there?  The sys
> > group is definitely under-performing  here.
> 
> I don't exactly understand "under-performing" you mean, sorry for my bad
> English.

Sorry, it was a shortcut - I meant using a system group for a content delivered as part of a request made by a tab would degrade the DOM scheduling.  Using the right target - bound to a tab - is preferred here.

> But, I think I can move the code to [1]. I think that's what you'd like to
> see, right?

Looks like the right place, yes!

Thanks.

> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 507743376d1ba753d14ab6b9305b7c6358570be8/netwerk/protocol/http/
> HttpChannelChild.cpp#2765
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 30

9 months ago
Created attachment 8874822 [details] [diff] [review]
Part5 : Label PAltDataOutputStreamChild

Summary:
 - Per our discussion, label PAltDataOutputStreamChild in HttpChannelChild by neckoTarget.
Attachment #8874323 - Attachment is obsolete: true
Attachment #8874323 - Flags: review?(honzab.moz)
Attachment #8874822 - Flags: review?(honzab.moz)
Comment on attachment 8874822 [details] [diff] [review]
Part5 : Label PAltDataOutputStreamChild

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

Thanks!
Attachment #8874822 - Flags: review?(honzab.moz) → review+

Comment 32

9 months ago
Comment on attachment 8874324 [details] [diff] [review]
Part6: Pass a labeled event target to StunAddrsRequestChild's constructor

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

Out of curiosity, in what cases are the window expected to be null?
Attachment #8874324 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 33

9 months ago
(In reply to Byron Campen [:bwc] from comment #32)
> Comment on attachment 8874324 [details] [diff] [review]
> Part6: Pass a labeled event target to StunAddrsRequestChild's constructor
> 
> Review of attachment 8874324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Out of curiosity, in what cases are the window expected to be null?

If we create PeerConnectionImpl from [1], the window is null. But I have no idea when or why this function will be called.


[1] http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#191
(Assignee)

Comment 34

9 months ago
Created attachment 8876594 [details] [diff] [review]
Part1: Refactoring for mNeckoTarget and  ChannelEvent, r=mayhemer

Carry r+ and rebase.
Attachment #8867069 - Attachment is obsolete: true
Attachment #8867085 - Attachment is obsolete: true
Attachment #8870249 - Attachment is obsolete: true
Attachment #8870376 - Attachment is obsolete: true
Attachment #8874321 - Attachment is obsolete: true
Attachment #8874324 - Attachment is obsolete: true
Attachment #8874822 - Attachment is obsolete: true
Attachment #8876594 - Flags: review+
(Assignee)

Comment 35

9 months ago
Created attachment 8876595 [details] [diff] [review]
Part2: Set event target for WyciwygChannelChild, r=mayhemer

Carry r+.
Attachment #8876595 - Flags: review+
(Assignee)

Comment 36

9 months ago
Created attachment 8876597 [details] [diff] [review]
Part3: Set event target for TCPSocketChild  and TCPServerSocketChild, r=jdm

Carry r+.
Attachment #8876597 - Flags: review+
(Assignee)

Comment 37

9 months ago
Created attachment 8876598 [details] [diff] [review]
Part 3-1: Use system group's event target  for TCPSocketChild's constructor, r=bwc

Carry r+.
Attachment #8876598 - Flags: review+
(Assignee)

Comment 38

9 months ago
Created attachment 8876600 [details] [diff] [review]
Part4: Pass a labeled main thread event target to nsIUDPSocketChild::bind, r=jdm

Carry r+.
Attachment #8876600 - Flags: review+
(Assignee)

Comment 39

9 months ago
Created attachment 8876601 [details] [diff] [review]
Part5 : Label PAltDataOutputStreamChild, r=mayhemer

Carry r+.
Attachment #8876601 - Flags: review+
(Assignee)

Comment 40

9 months ago
Created attachment 8876602 [details] [diff] [review]
Part6: Pass a labeled event target to StunAddrsRequestChild's constructor, r=bwc

Carry r+.
Attachment #8876602 - Flags: review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 42

9 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec77a61f0f2
Part1: Refactoring for mNeckoTarget and ChannelEvent, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a3c9dfefd3
Part2: Set event target for WyciwygChannelChild, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed01a9fc13c
Part3: Set event target for TCPSocketChild and TCPServerSocketChild, r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/df1028511939
Part3-1: Use system group's event target for TCPSocketChild's constructor, r=bwc
https://hg.mozilla.org/integration/mozilla-inbound/rev/750c2b2dbfb6
Part4: Pass a labeled main thread event target in nsIUDPSocketChild::bind, r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9519b41a5f
Part5: Label PAltDataOutputStreamChild, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c66041660e
Part6: Pass a labeled event target to StunAddrsRequestChild's constructor., r=bwc
Keywords: checkin-needed
Mark 54 won't fix as 54 was released.
status-firefox54: affected → wontfix
status-firefox55: --- → affected
status-firefox55: affected → wontfix
You need to log in before you can comment on or make changes to this bug.