Label runnables in netwerk/sctp/datachannel/

RESOLVED FIXED in Firefox 56

Status

()

Core
WebRTC: Networking
P2
normal
Rank:
29
RESOLVED FIXED
11 months ago
7 months ago

People

(Reporter: kershaw, Assigned: kershaw)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox56 fixed)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

11 months ago
It seems we have a lot of runnables need to be labeled in DataChannel.cpp.


netwerk/sctp/datachannel/DataChannel.cpp

NS_DispatchToMainThread(WrapRunnable(nsCOMPtr<nsIThread>(mInternalIOThread), // found in mozilla::DataChannelConnection::~DataChannelConnection

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::CompleteConnect

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleOpenRequestMessage

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleAssociationChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleAssociationChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleAssociationChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleStreamChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::OpenFinish

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::OpenFinish

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::OpenFinish

NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL); // found in mozilla::DataChannelConnection::ReadBlob

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannel::StreamClosedLocked

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannel::AppReady

NS_DispatchToMainThread(runnable); // found in mozilla::DataChannel::AppReady

NS_DispatchToMainThread(aMessage); // found in mozilla::DataChannel::SendOrQueue

Updated

11 months ago
Rank: 29
Priority: -- → P2
Whiteboard: [necko-next] → [necko-next][QDL][TDC-MVP][NECKO]
(Assignee)

Comment 1

7 months ago
Created attachment 8877463 [details] [diff] [review]
Label runnables in DataChannelConnection

Summary:
 - Make DataChannelConnection inherit from NeckoTargetHolder
 - Initialize main thread event in DataChannelConnection's constructor
 - Get an event target by calling GetNeckoTarget and use it to dispatch runnables in DataChannelConnection.cpp.

I am not quite sure who should I ask to review.
:jesup, could you perhaps take a look? Thanks.
Assignee: nobody → kechang
Status: NEW → ASSIGNED
Attachment #8877463 - Flags: review?(rjesup)
(Assignee)

Updated

7 months ago
Whiteboard: [necko-next][QDL][TDC-MVP][NECKO] → [necko-active][QDL][TDC-MVP][NECKO]
Comment on attachment 8877463 [details] [diff] [review]
Label runnables in DataChannelConnection

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

r+ with nits... NeckoTargetHolder does end up doing an extra refcount cycle for each use (and adds a fair bit of duplicate code), compared to if we just stored an nsCOMPtr<> directly and used it with a "DispatchToTarget(mVirtualMainThread, message)" method that does the "if null, dispatch to mainthread" bit.   But it will work.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +1485,5 @@
>      } else {
>        LOG(("Unexpected state: %d", mState));
>      }
>      break;
> +  case SCTP_COMM_LOST: {

brace on it's own line (this isn't an if; grey area in the style guidelines).  Better yet, don't brace; comptr will die at end of switch (but perhaps conflicts with later local target, if so can keep the braces)

@@ +1492,5 @@
> +    nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();
> +    target->Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
> +                       DataChannelOnMessageAvailable::ON_DISCONNECTED,
> +                       this)));
> +    }

Indent braced code

@@ +1503,5 @@
> +    nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();
> +    target->Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
> +                       DataChannelOnMessageAvailable::ON_DISCONNECTED,
> +                       this)));
> +    }

ditto on braces

@@ +2142,5 @@
>        }
>        if (channel->mFlags & DATA_CHANNEL_FLAGS_FINISH_OPEN) {
>          // We already returned the channel to the app.
>          NS_ERROR("Failed to send open request");
> +        nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();

don't we already have target as a local here?
Attachment #8877463 - Flags: review?(rjesup) → review+
(Assignee)

Comment 3

7 months ago
(In reply to Randell Jesup [:jesup] from comment #2)
> Comment on attachment 8877463 [details] [diff] [review]
> Label runnables in DataChannelConnection
> 
> Review of attachment 8877463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits... NeckoTargetHolder does end up doing an extra refcount cycle
> for each use (and adds a fair bit of duplicate code), compared to if we just
> stored an nsCOMPtr<> directly and used it with a
> "DispatchToTarget(mVirtualMainThread, message)" method that does the "if
> null, dispatch to mainthread" bit.   But it will work.
> 

Thanks for the review.

To save an extra refcount cycle, I think I can add a function "DispatchToNeckoTargetOrMainThread" in NeckoTargetHolder.
By doing this, we can get rid off some unnecessary local variables and annoying braces.
(Assignee)

Comment 4

7 months ago
Created attachment 8877959 [details] [diff] [review]
Add a helper function in NeckoTargetHolder for dispatching runnables directly

Please take a look at comment#2.

The way of using mNeckoTarget to dispatch is kind of annoying. We have to get a target by calling GetNeckoTarget() first, and then use it to dispatch.

I think we can add a helper function to dispatch runnables directly.

What do you think, Honza?
Attachment #8877959 - Flags: review?(honzab.moz)
Comment on attachment 8877959 [details] [diff] [review]
Add a helper function in NeckoTargetHolder for dispatching runnables directly

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

r+ with comments addressed

::: netwerk/ipc/NeckoTargetHolder.cpp
@@ +32,5 @@
> +  if (mNeckoTarget) {
> +    return mNeckoTarget->Dispatch(Move(aRunnable), aDispatchFlags);
> +  }
> +
> +  return  NS_DispatchToMainThread(Move(aRunnable), aDispatchFlags);

Before landing please consult bug 1361164 and bug 1365096 (:billm), we are replacing NS_GetMainThread with something smarter, not sure if NS_DispatchToMainThread is being replaced too or not.

nit: two spaces after return.
Attachment #8877959 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8877959 [details] [diff] [review]
> Add a helper function in NeckoTargetHolder for dispatching runnables directly

also, where is this used?
Comment on attachment 8877959 [details] [diff] [review]
Add a helper function in NeckoTargetHolder for dispatching runnables directly

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

I wasn't asked, but r+.

Honza: This would replace (lots of) instances of :
{
 nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();
 target->Dispatch(some form of runnable);
}

with:

Dispatch(runnable);

If Dispatch() might create conflicts (I don't think it will), you could use DispatchToTarget() or DispatchToNeckoTarget()

::: netwerk/ipc/NeckoTargetHolder.h
@@ +28,5 @@
>    // Get event target for processing network events.
>    virtual already_AddRefed<nsIEventTarget> GetNeckoTarget();
> +  // When |mNeckoTarget| is not null, use it to dispatch the runnable.
> +  // Otherwise, dispatch the runnable to the main thread.
> +  nsresult DispatchToNeckoTargetOrMainThread(

I suggest Dispatch() -- what it dispatches to is up to the NeckoTargetHolder to figure out; the algorithm doesn't need to be in the method name. :-)
Attachment #8877959 - Flags: review+
(Assignee)

Comment 8

7 months ago
> ::: netwerk/ipc/NeckoTargetHolder.h
> @@ +28,5 @@
> >    // Get event target for processing network events.
> >    virtual already_AddRefed<nsIEventTarget> GetNeckoTarget();
> > +  // When |mNeckoTarget| is not null, use it to dispatch the runnable.
> > +  // Otherwise, dispatch the runnable to the main thread.
> > +  nsresult DispatchToNeckoTargetOrMainThread(
> 
> I suggest Dispatch() -- what it dispatches to is up to the NeckoTargetHolder
> to figure out; the algorithm doesn't need to be in the method name. :-)

Thanks for this suggestion. I should've also asked your opinion :)
(Assignee)

Comment 9

7 months ago
(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8877959 [details] [diff] [review]
> Add a helper function in NeckoTargetHolder for dispatching runnables directly
> 
> Review of attachment 8877959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed
> 
> ::: netwerk/ipc/NeckoTargetHolder.cpp
> @@ +32,5 @@
> > +  if (mNeckoTarget) {
> > +    return mNeckoTarget->Dispatch(Move(aRunnable), aDispatchFlags);
> > +  }
> > +
> > +  return  NS_DispatchToMainThread(Move(aRunnable), aDispatchFlags);
> 
> Before landing please consult bug 1361164 and bug 1365096 (:billm), we are
> replacing NS_GetMainThread with something smarter, not sure if
> NS_DispatchToMainThread is being replaced too or not.
> 

Thanks for pointing this out.
This should be changed to something like:

 nsCOMPtr<nsIEventTarget> mainTarget = GetMainThreadEventTarget();
 return mainTarget->Dispatch(Move(aRunnable), nsIEventTarget::DISPATCH_NORMAL);

I'll update the patch.
(Assignee)

Comment 10

7 months ago
Created attachment 8879470 [details] [diff] [review]
Add a helper function in NeckoTargetHolder for dispatching runnables directly, r=mayhemer, jesup

Carry r+ from reviewers.
Attachment #8877463 - Attachment is obsolete: true
Attachment #8877959 - Attachment is obsolete: true
Attachment #8879470 - Flags: review+
(Assignee)

Comment 11

7 months ago
Created attachment 8879471 [details] [diff] [review]
Label runnables in DataChannelConnection, r=jesup

As said in comment#7, this patch uses NeckoTargetHolder::Dispatch to replace NS_DispatchToMainThread() in DataChannel.cpp.

I think we don't need another review round since changes in this patch are quite straightforward.
Attachment #8879471 - Flags: review+
seems there are conflicts with the 2nd patch like 

patching file netwerk/sctp/datachannel/DataChannel.cpp
Hunk #2 FAILED at 237
Hunk #12 FAILED at 2410
2 out of 15 hunks FAILED -- saving rejects to file netwerk/sctp/datachannel/DataChannel.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0002-Bug-1343755-Label-runnables-in-DataChannelConnection.patch

could you take a look, thanks!
Flags: needinfo?(kechang)
Keywords: checkin-needed
(Assignee)

Comment 14

7 months ago
(In reply to Carsten Book [:Tomcat] from comment #13)
> seems there are conflicts with the 2nd patch like 
> 
> patching file netwerk/sctp/datachannel/DataChannel.cpp
> Hunk #2 FAILED at 237
> Hunk #12 FAILED at 2410
> 2 out of 15 hunks FAILED -- saving rejects to file
> netwerk/sctp/datachannel/DataChannel.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> 0002-Bug-1343755-Label-runnables-in-DataChannelConnection.patch
> 
> could you take a look, thanks!

Thanks.

I'll rebase this until bug 1372405 is landed.
Flags: needinfo?(kechang)
(Assignee)

Comment 15

7 months ago
Created attachment 8881429 [details] [diff] [review]
Label runnables in DataChannelConnection, r=jesup

Rebased.
Attachment #8879471 - Attachment is obsolete: true
Attachment #8881429 - Flags: review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 16

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77e551ec0d5f
Part 1: Add a helper function in NeckoTargetHolder for dispatching runnable. r=mayhemer, r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b23b5dce54f
Label runnables in DataChannelConnection. r=jesup
Keywords: checkin-needed

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77e551ec0d5f
https://hg.mozilla.org/mozilla-central/rev/9b23b5dce54f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

7 months ago
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.