Closed
Bug 1343755
Opened 8 years ago
Closed 7 years ago
Label runnables in netwerk/sctp/datachannel/
Categories
(Core :: WebRTC: Networking, defect, P2)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: kershaw, Assigned: kershaw)
References
Details
(Whiteboard: [necko-active][QDL][TDC-MVP][NECKO])
Attachments
(2 files, 3 obsolete files)
2.15 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
23.25 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Rank: 29
Priority: -- → P2
Updated•8 years ago
|
Whiteboard: [necko-next] → [necko-next][QDL][TDC-MVP][NECKO]
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Whiteboard: [necko-next][QDL][TDC-MVP][NECKO] → [necko-active][QDL][TDC-MVP][NECKO]
Comment 2•7 years ago
|
||
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 years 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 years ago
|
||
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 5•7 years ago
|
||
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+
![]() |
||
Comment 6•7 years 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 also, where is this used?
Comment 7•7 years ago
|
||
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 years 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 years 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 years ago
|
||
Carry r+ from reviewers.
Attachment #8877463 -
Attachment is obsolete: true
Attachment #8877959 -
Attachment is obsolete: true
Attachment #8879470 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ad90918ac00811cd70733b11c0e2325c3c0a55
Keywords: checkin-needed
Comment 13•7 years ago
|
||
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 years 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 years ago
|
||
Rebased.
Attachment #8879471 -
Attachment is obsolete: true
Attachment #8881429 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 16•7 years 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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77e551ec0d5f https://hg.mozilla.org/mozilla-central/rev/9b23b5dce54f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•