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

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.

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?

(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.

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?

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.

(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. :-)

> ::: 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 :)

(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.

Carry r+ from reviewers.

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.

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!

(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.

Rebased.

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

https://hg.mozilla.org/mozilla-central/rev/77e551ec0d5f https://hg.mozilla.org/mozilla-central/rev/9b23b5dce54f