Issue 20001: Bug 869869.
DescriptionBug 869869. Patch Set 1 #
Total comments: 15
Patch Set 2 : Next revision from sc #
Total comments: 85
Patch Set 3 : REvised version from SC #
Total comments: 42
Patch Set 4 : REvised patch #
Total comments: 29
Patch Set 5 : Further Revisions #
Total comments: 8
Patch Set 6 : Another revision #
Total comments: 30
Patch Set 7 : Another revision #
Total comments: 29
Patch Set 8 : REvision 8.1 #
Total comments: 21
Patch Set 9 : New revision #
Total comments: 4
Created: 7 years, 5 months ago
MessagesTotal messages: 23
Here are some initial comments on this patch. LMK if you want to discuss. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:245: int nr_transport_addr_to_string(const nr_transport_addr *addr, This fucntion seems unnecessary. nr_transport_addrs have an .as_string value, which is the addra s a C-string. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:301: int nr_transport_addr_get_port(const nr_transport_addr *addr, This appears to already exist in transport_addr.c. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:520: NS_IMPL_THREADSAFE_ISUPPORTS1(NrSocketIpc, nsIUDPSocketInternal) Let's refactor this so that we have NrSocketIpc inherit from NrSocket or from a common base class. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:522: NrSocketIpc::NrSocketIpc() Please try to match the style of the surrounding code.... All this code is in Google style: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:537: MOZ_ASSERT(NS_SUCCEEDED(rv)); These seem like things that could actually fail. Suggest moving them to ::create so you can return an error. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:557: NrSocketIpc::CallListenerArrayBuffer(const nsAString & type, const nsAString & host, uint16_t port, uint8_t *data, uint32_t dataLength) Which thread is this called on? I am worried about thread locking on mErr. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:563: nr_udp_message *msg = (nr_udp_message*)moz_malloc(sizeof(nr_udp_message)); C++ style casts, please. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:577: mSTSThread->Dispatch(new NrSocketIpcRecvCallbackRunnable(this, NR_ASYNC_WAIT_READ, msg), Use WrapRunnable. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:755: r=R_IO_ERROR; Is this correct? Or do we want R_WOULDBLOCK? Or R_EOD? https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:941: sock->Wait(1000); This seems problematic. We can't take a 1second wait here. If we are going to block, it should be at least for the minimum time. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:968: sock->Wait(1000); This seems like we definitely could do it in nonblocking mode, right? https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.cpp:980: return sock->result(); I think it's actually OK to have this just ignore erorrs. https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.h:64: #include "../webrtc/trunk/webrtc/system_wrappers/interface/event_wrapper.h" Let's not use WebRTC internal constructs for inter-thread communication. Prefer Dispatch(). https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/nr_socke... media/mtransport/nr_socket_prsock.h:208: class NrSocketIpcCreateRunnable : public nsRunnable Suggest replacing all these runnables with WrapRunnable https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/third_pa... File media/mtransport/third_party/nICEr/src/net/nr_socket_local.h (right): https://firefox-codereview.appspot.com/20001/diff/1/media/mtransport/third_pa... media/mtransport/third_party/nICEr/src/net/nr_socket_local.h:38: int nr_socket_create(nr_transport_addr *addr, nr_socket **sockp); Why the rename? Local in this setting means "on my own machine"
Sign in to reply to this message.
http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:139: addPollFlags(flag); Why not just directly frob mPollFlags http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:445: NrSocketIpc::NrSocketIpc() : mPollFlags(0), udp_buffer(nullptr), mErr(false), initializers go on the next line with a four-stop indent. See the Google style guide. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:451: MOZ_ASSERT(false, "Failed to create UDP socket in content process."); Since this stuff can fail, let's move it to the create function so it can return error. Then abck these asserts up with an error return. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:465: uint32_t lineNumber, convention for arguments is underscore, no camel case. Here and below. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:467: r_log_e(LOG_GENERIC,LOG_ERR, NS_ConvertUTF16toUTF8(message).get()); Please do not call r_log_e() with random strings. Do: r_log_e(..., "%s", ...) http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:472: NS_IMETHODIMP NrSocketIpc::CallListenerArrayBuffer(const nsAString &type, What thread is this called on? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:476: if (mState != NR_CONNECTED) { How can this happen? Does it need an assert? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:481: moz_malloc(sizeof(nr_udp_message))); Seems like new would be preferred in C++ code, no? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:483: PR_StringToNetAddr(NS_ConvertUTF16toUTF8(host).get(), &msg->from); This can fail. Check return value http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:484: PR_SetNetAddr(PR_IpAddrNull, msg->from.raw.family, port, &msg->from); And here. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:486: FallibleTArray<uint8_t> fallibleArray; Please use either RefPtr<DataBuffer> or nsAutoPtr<DataBuffer> http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:494: mSTSThread->Dispatch(mozilla::WrapRunnable(this, &NrSocketIpc::recv_callback, msg), Let's use a reference counted object here to avoid the need for delete. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:500: ReentrantMonitorAutoEnter mon(mMonitor); Would a condition variable be simpler here? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:505: mSocketChild->GetLocalAddress(address); Can any of these fail? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:508: PR_StringToNetAddr(NS_ConvertUTF16toUTF8(address).get(), &praddr); Check for failure please on both these. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:530: } Please check for other values of readyState than the expected ones. A MOZ_ASSERT() is probably OK. What happens if we go to closed here? Would it make more sense to invert the control discipline and look at the readyState as the outer loop and then the mState inside that? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:538: break; indent. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:551: default: Maybe MOZ_CRASH()? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:561: // sock operation will perform on main thread. Please assert the thread. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:565: mMonitor.Wait(); Please check the state after this, since it might fail. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:576: NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::sendto_int, Is this going to work right in unit test? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:583: NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::close_int)); Samehere. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:590: if (mState != NR_CONNECTED) { MOZ_ASSERT? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:590: if (mState != NR_CONNECTED) { Is there a race condition here between the state being updated. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:595: ABORT(R_IO_ERROR); //XXX other error code? How could this hapen? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:595: ABORT(R_IO_ERROR); //XXX other error code? Is this a "can't happen" http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:597: if ((r=nr_praddr_to_transport_addr(&udp_buffer->from,from,0))) { space after , http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:601: memcpy(buf, udp_buffer->data->Elements(), udp_buffer->data->Length()); You need to check maxlen here. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:606: udp_buffer = nullptr; If you use DataBuffer and smart pointers, this will be simpler. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:622: int r, _status, port; Please put r, _status on their own lines. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:624: char addr_string[64]; Please move these closer to the use site? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:627: ABORT(R_INTERNAL); Is this a can't happen? If so, please add a MOZ_ASSERT http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:630: if ((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) { can't you just use .as_string? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:646: nr_transport_addr_copy(&my_addr_,addr); space after , http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:671: if ((r=nr_transport_addr_get_port(to, &port))) { Can you refactor the addr handling so it's not duplicated? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:679: (uint16_t)port, (uint8_t*)msg, len))) { C++-style casts, please. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:694: mSocketChild->Close(); I note that this is a smart pointer. Does the destructor automatically close? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:699: void NrSocketIpc::recv_callback(nr_udp_message *msg) { Have this take a smart pointer http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:705: } else { This seems to lose packets if you're not waiting for read. Isn't the right answer to buffer up the data, not discard it. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:742: // create IPC bridge for content process Should we make this conditional on being Gonk? I guess we're going to add E10S for desktop, but maybe until then? Thoughts? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:745: } else { Extra space between else { http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... File media/mtransport/nr_socket_prsock.h (right): http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:71: class NrSocketBase { Why not have this inherit from nsASocketHandler? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:97: NS_IMETHOD_(nsrefcnt) Release(void) = 0; Not needed if you inherit from nsASocketHandler, right? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:134: virtual void addPollFlags(uint32_t flags) { I tend to just directly manipulate these, but regardless they should be in the base class. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:193: void close_int(); Our style has changed here. Do _m (for main) instead of _int http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:195: void recv_callback(nr_udp_message *msg); recv_callback_m http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:196: private: Whitespace. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:200: uint32_t mPollFlags; Please use style that matches the existing code. Members are foo_, not mFoo. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:200: uint32_t mPollFlags; This shadows stuff that's in the base class.
Sign in to reply to this message.
feedback is updated. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:139: addPollFlags(flag); On 2013/06/20 14:38:33, ekr-webrtc wrote: > Why not just directly frob mPollFlags NrSocket inherit mPollFlags from ASocketHandler. Introducing another mPollFlags in NrSocketBase will cause naming conflict in NrSocket. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:472: NS_IMETHODIMP NrSocketIpc::CallListenerArrayBuffer(const nsAString &type, On 2013/06/20 14:38:33, ekr-webrtc wrote: > What thread is this called on? All the IPC callbacks are called on main thread. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:476: if (mState != NR_CONNECTED) { On 2013/06/20 14:38:33, ekr-webrtc wrote: > How can this happen? Does it need an assert? This can happen at a race condition, e.g. content thread invoke close, in the meanwhile, b2g thread receive a packet. So we need to silently drop the received packet without causing any error. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:481: moz_malloc(sizeof(nr_udp_message))); On 2013/06/20 14:38:33, ekr-webrtc wrote: > Seems like new would be preferred in C++ code, no? I can make nr_udp_message a regular C++ class and use new for allocation. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:481: moz_malloc(sizeof(nr_udp_message))); On 2013/06/20 14:38:33, ekr-webrtc wrote: > Seems like new would be preferred in C++ code, no? I can make nr_udp_message a regular C++ class and use new for allocation. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:500: ReentrantMonitorAutoEnter mon(mMonitor); On 2013/06/20 14:38:33, ekr-webrtc wrote: > Would a condition variable be simpler here? Monitor is simpler to use in Gecko. See http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/CondVar.h#l21 http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:505: mSocketChild->GetLocalAddress(address); On 2013/06/20 14:38:33, ekr-webrtc wrote: > Can any of these fail? Basically it will not fail. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:530: } On 2013/06/20 14:38:33, ekr-webrtc wrote: > Please check for other values of readyState than the expected ones. A > MOZ_ASSERT() is probably OK. What happens if we go to closed here? > In the INIT state, socket haven't been created, so it should not get "closed" readyState. I'll put some more ASSERT in this function. > Would it make more sense to invert the control discipline and look at the > readyState as the outer loop and then the mState inside that? I'll try comparing these two kinds of coding style. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:551: default: On 2013/06/20 14:38:33, ekr-webrtc wrote: > Maybe MOZ_CRASH()? I see MOZ_NOT_REACHED() is widely used in default switch case which is unexpected. Is there any reason you rather use MOZ_CRASH() in this case? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:576: NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::sendto_int, On 2013/06/20 14:38:33, ekr-webrtc wrote: > Is this going to work right in unit test? I haven't run unit test yet, but I suppose unit test is executed under non-OOP mode, which means it will use NrSocket instead of NrSocketIpc. I'll need to add some B2G marionette test cases to extend the test coverage. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:590: if (mState != NR_CONNECTED) { On 2013/06/20 14:38:33, ekr-webrtc wrote: > Is there a race condition here between the state being updated. Yes, race condition with close and we should not raise any error/warning under this case. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:595: ABORT(R_IO_ERROR); //XXX other error code? On 2013/06/20 14:38:33, ekr-webrtc wrote: > Is this a "can't happen" It'll only happened if recvfrom is been invoked outside the read callback. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:624: char addr_string[64]; On 2013/06/20 14:38:33, ekr-webrtc wrote: > Please move these closer to the use site? Declaring variable after a goto statement will produce a compile error, so I cannot put this statement below ABORT(…). http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:630: if ((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) { On 2013/06/20 14:38:33, ekr-webrtc wrote: > can't you just use .as_string? .as_string contains more than ip address, e.g. "IP4:127.0.0.1:1234". You'll still need to parse the string to get ip address. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:671: if ((r=nr_transport_addr_get_port(to, &port))) { On 2013/06/20 14:38:33, ekr-webrtc wrote: > Can you refactor the addr handling so it's not duplicated? I'm not sure what you mean here, can you point out the specific code that show the duplication? http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:694: mSocketChild->Close(); On 2013/06/20 14:38:33, ekr-webrtc wrote: > I note that this is a smart pointer. Does the destructor automatically close? No, the current design os UDPSocketChild is not doing automatically close. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:705: } else { On 2013/06/20 14:38:33, ekr-webrtc wrote: > This seems to lose packets if you're not waiting for read. Isn't the right > answer to buffer up the data, not discard it. I could add a simple version of ring buffer for it. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.cpp:742: // create IPC bridge for content process On 2013/06/20 14:38:33, ekr-webrtc wrote: > Should we make this conditional on being Gonk? > > I guess we're going to add E10S for desktop, but maybe until then? Thoughts? Even in Gonk, apps might be running in non-OOP mode, so I prefer to keep the condition in general. http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... File media/mtransport/nr_socket_prsock.h (right): http://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_soc... media/mtransport/nr_socket_prsock.h:71: class NrSocketBase { On 2013/06/20 14:38:33, ekr-webrtc wrote: > Why not have this inherit from nsASocketHandler? NrSocketIpc doesn't need to be inherit from nsASocketHandler, otherwise we'll end up with a bunch of empty nsASocketHandler virtual function in the implementation of NrSocketIpc.
Sign in to reply to this message.
Responses below. I think the mPollFlags thing will be a lot simpler when you change mPolllFlags to be poll_flags_ or the like https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:139: addPollFlags(flag); On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Why not just directly frob mPollFlags > NrSocket inherit mPollFlags from ASocketHandler. Introducing another mPollFlags > in NrSocketBase will cause naming conflict in NrSocket. Oh, I see what you are doing here. OK. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:472: NS_IMETHODIMP NrSocketIpc::CallListenerArrayBuffer(const nsAString &type, On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > What thread is this called on? > > All the IPC callbacks are called on main thread. Please add a thread assert, then. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:476: if (mState != NR_CONNECTED) { On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > How can this happen? Does it need an assert? > > This can happen at a race condition, e.g. content thread invoke close, in the > meanwhile, b2g thread receive a packet. So we need to silently drop the received > packet without causing any error. OK. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:481: moz_malloc(sizeof(nr_udp_message))); On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Seems like new would be preferred in C++ code, no? > > I can make nr_udp_message a regular C++ class and use new for allocation. You can use new even with C structs. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:500: ReentrantMonitorAutoEnter mon(mMonitor); On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Would a condition variable be simpler here? > > Monitor is simpler to use in Gecko. > See http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/CondVar.h#l21 OK. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:551: default: On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Maybe MOZ_CRASH()? > > I see MOZ_NOT_REACHED() is widely used in default switch case which is > unexpected. Is there any reason you rather use MOZ_CRASH() in this case? This seems OK as-is. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:576: NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::sendto_int, On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Is this going to work right in unit test? > > I haven't run unit test yet, but I suppose unit test is executed under non-OOP > mode, which means it will use NrSocket instead of NrSocketIpc. I'll need to add > some B2G marionette test cases to extend the test coverage. Sure would be nice to have the unit tests work with NrSocketIpc, but I guess we can live with that. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:590: if (mState != NR_CONNECTED) { On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Is there a race condition here between the state being updated. > > Yes, race condition with close and we should not raise any error/warning under > this case. Is it thread-safe, though? I worry it is being frobbed in two threads. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:595: ABORT(R_IO_ERROR); //XXX other error code? On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Is this a "can't happen" > > It'll only happened if recvfrom is been invoked outside the read callback. Well ,that's a totally legit thing to do, so it needs to work. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:624: char addr_string[64]; On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Please move these closer to the use site? > > Declaring variable after a goto statement will produce a compile error, so I > cannot put this statement below ABORT(…). OK https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:630: if ((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) { On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > can't you just use .as_string? > > .as_string contains more than ip address, e.g. "IP4:127.0.0.1:1234". You'll > still need to parse the string to get ip address. OK. add a comment to this effect please. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:671: if ((r=nr_transport_addr_get_port(to, &port))) { On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Can you refactor the addr handling so it's not duplicated? > > I'm not sure what you mean here, can you point out the specific code that show > the duplication? sendto_int() and create_int() seem to duplicate most of the lines from 668-678 https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:694: mSocketChild->Close(); On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > I note that this is a smart pointer. Does the destructor automatically close? > > No, the current design os UDPSocketChild is not doing automatically close. Should we change that? https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:705: } else { On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > This seems to lose packets if you're not waiting for read. Isn't the right > > answer to buffer up the data, not discard it. > I suggest std::queue<udp_buffer> > I could add a simple version of ring buffer for it. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:742: // create IPC bridge for content process On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Should we make this conditional on being Gonk? > > > > I guess we're going to add E10S for desktop, but maybe until then? Thoughts? > > Even in Gonk, apps might be running in non-OOP mode, so I prefer to keep the > condition in general. OK https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:742: // create IPC bridge for content process On 2013/06/21 13:49:55, schien wrote: > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > Should we make this conditional on being Gonk? > > > > I guess we're going to add E10S for desktop, but maybe until then? Thoughts? > > Even in Gonk, apps might be running in non-OOP mode, so I prefer to keep the > condition in general. OK
Sign in to reply to this message.
SC: more comments below. https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/4001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:630: if ((r=nr_transport_addr_get_addrstring(addr, addr_string, sizeof(addr_string)))) { On 2013/06/21 16:40:13, ekr-webrtc wrote: > On 2013/06/21 13:49:55, schien wrote: > > On 2013/06/20 14:38:33, ekr-webrtc wrote: > > > can't you just use .as_string? > > > > .as_string contains more than ip address, e.g. "IP4:127.0.0.1:1234". You'll > > still need to parse the string to get ip address. > > OK. add a comment to this effect please. Still don't see a comment. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:202: mPollFlags = poll_flags(); Should you be setting these if r != 0, here and below. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:321: nsAString &host, int32_t &port) { Intent to line up with ( https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:321: nsAString &host, int32_t &port) { Out parameters in this code should use: T *foo, not T & foo. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:480: NrSocketIpc::NrSocketIpc() : err_(false), state_(NR_INIT), Please fix style. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:489: close(); This has a lifetime problem: close() dispatches asynchronously to the main thread, which means that the NrSocketIpc can have been destroyed when close_m runs. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:499: MOZ_ASSERT(NS_IsMainThread()); The mtransport convention is to do ASSERT_ON_THREAD(main_thread_) or some other variable that has it. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:520: MOZ_ASSERT(status == PR_SUCCESS); Is it really not possible that these can fail in ordinary operation. If it is possible, you should be handling the error. Generally, MOZ_ASSERTS should be backed up by real error handling and should return status. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:528: nsIEventTarget::DISPATCH_NORMAL); I don't think you need nsIEventTarget:: here. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:537: r_log_e(LOG_GENERIC,LOG_ERR, "Failed to get local port"); Don't you need to return after this error here and below? https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:548: MOZ_ASSERT(status == PR_SUCCESS); Above comments about MOZ_ASSERT https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:553: ReentrantMonitorAutoEnter mon(monitor_); Why do you need to lock this monitor before frobbing my_addr_? https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:565: if (NS_FAILED(static_cast<nsresult>(value))) { Do you need to grab the monitor before modifying err_. I see you are grabbing it elsewhere. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:575: state_ = NR_CLOSED; Is there a transition from NR_CONNECTING to NR_CLOSED? https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:618: NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::create_m, This is async, so I think you have a lifetime issue. If you pass an RefPtr<this> then the problem should go away. Also check the other _m dispatches. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:636: nr_transport_addr *to) { Indent one space to line up. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:684: nr_udp_message &msg = received_msgs_.front(); Space before the conditional. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:709: } In general, partial UDP reads should just drop the remaining data. If you don't read the whole packet' it's not generally possible to do anything with the rest. That will also make this code simpler. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:798: } else { Fix indent. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:164: uint32_t begin; ctors before member variables. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message &other) { Hmm... I have a couple of concerns here: Because you are using nsAutoPtr here, when you copy-construct this, you are destroying the source, which is disturbing when you are putting these in a queue. Suggest that we replace this with RefPtr<>. Alternately, have a std::queue<nr_udp_message *> and ditch the copy ctor.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:202: mPollFlags = poll_flags(); On 2013/07/01 04:55:33, ekr-webrtc wrote: > Should you be setting these if r != 0, here and below. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:321: nsAString &host, int32_t &port) { On 2013/07/01 04:55:33, ekr-webrtc wrote: > Intent to line up with ( Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:321: nsAString &host, int32_t &port) { On 2013/07/01 04:55:33, ekr-webrtc wrote: > Out parameters in this code should use: T *foo, not T & foo. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:480: NrSocketIpc::NrSocketIpc() : err_(false), state_(NR_INIT), On 2013/07/01 04:55:33, ekr-webrtc wrote: > Please fix style. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:489: close(); On 2013/07/01 04:55:33, ekr-webrtc wrote: > This has a lifetime problem: close() dispatches asynchronously to the main > thread, which means that the NrSocketIpc can have been destroyed when close_m > runs. I'll make UDPSocketChild to be auto closed after destroy, which will make the dtor easiler. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:499: MOZ_ASSERT(NS_IsMainThread()); On 2013/07/01 04:55:33, ekr-webrtc wrote: > The mtransport convention is to do > > ASSERT_ON_THREAD(main_thread_) or some other variable that has it. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:520: MOZ_ASSERT(status == PR_SUCCESS); On 2013/07/01 04:55:33, ekr-webrtc wrote: > Is it really not possible that these can fail in ordinary operation. If it is > possible, you should be handling the error. > > Generally, MOZ_ASSERTS should be backed up by real error handling and should > return status. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:528: nsIEventTarget::DISPATCH_NORMAL); On 2013/07/01 04:55:33, ekr-webrtc wrote: > I don't think you need nsIEventTarget:: here. Will change to NS_DISPATCH_NORMAL. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:537: r_log_e(LOG_GENERIC,LOG_ERR, "Failed to get local port"); On 2013/07/01 04:55:33, ekr-webrtc wrote: > Don't you need to return after this error here and below? I'll make it return after error. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:548: MOZ_ASSERT(status == PR_SUCCESS); On 2013/07/01 04:55:33, ekr-webrtc wrote: > Above comments about MOZ_ASSERT Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:553: ReentrantMonitorAutoEnter mon(monitor_); On 2013/07/01 04:55:33, ekr-webrtc wrote: > Why do you need to lock this monitor before frobbing my_addr_? I'm putting lock around all the attributes in NrSocketIpc that will be accessed by multiple threads. I'll also add lock in NrSocketIpc::getaddr. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:565: if (NS_FAILED(static_cast<nsresult>(value))) { On 2013/07/01 04:55:33, ekr-webrtc wrote: > Do you need to grab the monitor before modifying err_. I see you are grabbing it > elsewhere. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:575: state_ = NR_CLOSED; On 2013/07/01 04:55:33, ekr-webrtc wrote: > Is there a transition from NR_CONNECTING to NR_CLOSED? I'll lock the monitor before modifying state_, it'll prevent close-during-create scenario. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:618: NS_DispatchToMainThread(mozilla::WrapRunnable(this, &NrSocketIpc::create_m, On 2013/07/01 04:55:33, ekr-webrtc wrote: > This is async, so I think you have a lifetime issue. If you pass an RefPtr<this> > then the problem should go away. Also check the other _m dispatches. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:636: nr_transport_addr *to) { On 2013/07/01 04:55:33, ekr-webrtc wrote: > Indent one space to line up. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:684: nr_udp_message &msg = received_msgs_.front(); On 2013/07/01 04:55:33, ekr-webrtc wrote: > Space before the conditional. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:709: } On 2013/07/01 04:55:33, ekr-webrtc wrote: > In general, partial UDP reads should just drop the remaining data. If you don't > read the whole packet' it's not generally possible to do anything with the rest. > > That will also make this code simpler. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:798: } else { On 2013/07/01 04:55:33, ekr-webrtc wrote: > Fix indent. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:164: uint32_t begin; On 2013/07/01 04:55:33, ekr-webrtc wrote: > ctors before member variables. Done. https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message &other) { On 2013/07/01 04:55:33, ekr-webrtc wrote: > Hmm... I have a couple of concerns here: > Because you are using nsAutoPtr here, when you copy-construct this, you are > destroying the source, which is disturbing when you are putting these in a > queue. Suggest that we replace this with RefPtr<>. > > Alternately, have a std::queue<nr_udp_message *> and ditch the copy ctor. Using RefPtr<> looks good to me, should I also replace all the other nsAutoPtr<nr_udp_message> with RefPtr<nr_udp_message> in NrSocketIpc?
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message &other) { On 2013/07/07 15:44:59, schien wrote: > On 2013/07/01 04:55:33, ekr-webrtc wrote: > > Hmm... I have a couple of concerns here: > > Because you are using nsAutoPtr here, when you copy-construct this, you are > > destroying the source, which is disturbing when you are putting these in a > > queue. Suggest that we replace this with RefPtr<>. > > > > Alternately, have a std::queue<nr_udp_message *> and ditch the copy ctor. > > Using RefPtr<> looks good to me, should I also replace all the other > nsAutoPtr<nr_udp_message> with RefPtr<nr_udp_message> in NrSocketIpc? It's not the nr_udp_message I am concerned about so much as the DataBuffer. The semantics of nsAutoPtr<T> is that only one copy is valid at a time. So, if we have, for instance: nsAutoPtr<T> t1(new T()); nsAutoPtr<T> t2 = t1; t1 now becomes invalid. So the problem here is that if you have: nr_udp_message u1; u1.data = new DataBuffer(); nr_udp_message u2(u1); u1 now has an invalid data buffer, which seems suboptimal, because it's otherwise a valid object. The two primary ways to address this are: 1. Pass around nr_udp_message *s (or a smart pointer) and then you never have to copy-construct nr_udp_messages. (In this case I would DISALLOW_COPY_ASSIGN(nr_udp_message)). 2. Have nr_udp_message hold a RefPtr<DataBuffer> so that you can do a copy construct and still have the origin object be safe. My suggestion would actually be the former, but you'll need to be a little careful about the ownership of the nr_udp_message *s to make sure that things get freed when expected.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/10001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:167: nr_udp_message(const nr_udp_message &other) { On 2013/07/07 16:19:16, ekr-webrtc wrote: > On 2013/07/07 15:44:59, schien wrote: > > On 2013/07/01 04:55:33, ekr-webrtc wrote: > > > Hmm... I have a couple of concerns here: > > > Because you are using nsAutoPtr here, when you copy-construct this, you are > > > destroying the source, which is disturbing when you are putting these in a > > > queue. Suggest that we replace this with RefPtr<>. > > > > > > Alternately, have a std::queue<nr_udp_message *> and ditch the copy ctor. > > > > Using RefPtr<> looks good to me, should I also replace all the other > > nsAutoPtr<nr_udp_message> with RefPtr<nr_udp_message> in NrSocketIpc? > > It's not the nr_udp_message I am concerned about so much as the DataBuffer. > > The semantics of nsAutoPtr<T> is that only one copy is valid at a time. So, if > we have, for instance: > > nsAutoPtr<T> t1(new T()); > nsAutoPtr<T> t2 = t1; > > t1 now becomes invalid. > > So the problem here is that if you have: > > nr_udp_message u1; > u1.data = new DataBuffer(); > > nr_udp_message u2(u1); > > u1 now has an invalid data buffer, which seems suboptimal, because it's > otherwise a valid object. > > The two primary ways to address this are: > > 1. Pass around nr_udp_message *s (or a smart pointer) and then you never have to > copy-construct nr_udp_messages. (In this case I would > DISALLOW_COPY_ASSIGN(nr_udp_message)). > > 2. Have nr_udp_message hold a RefPtr<DataBuffer> so that you can do a copy > construct and still have the origin object be safe. > > > My suggestion would actually be the former, but you'll need to be a little > careful about the ownership of the nr_udp_message *s to make sure that things > get freed when expected. Will using queue< RefPtr<nr_udp_message> > make memory management easier in the first approach?
Sign in to reply to this message.
This is looking pretty good. I'm still a little concerned about how you're handling errors. Shouldn't a lot of these have some negative side effect.... https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:269: static int nr_transport_addr_to_netaddr(nr_transport_addr *addr, Use PRNetAddrToNetAddr here instead of duplicating the code. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:539: NrSocketIpc::NrSocketIpc(nsCOMPtr<nsIEventTarget> main_thread) const nsCOMPtr<nsIEventTarget>& https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:574: return NS_OK; Is NS_OK what we really want here? The assert suggests that this is a can't happen. Does that mean we want to return some error? Maybe set an error flag? https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:766: ReentrantMonitorAutoEnter mon(monitor_); Do this at the top. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:882: sock = new NrSocketIpc(main_thread.forget()); Why are you doing forget here? https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:162: struct nr_udp_message : public RefCounted<nr_udp_message> { I typically use the NS_INLINE_DECL_ macros https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:164: nsAutoPtr<DataBuffer> data; Use private:DISALLOW_COPY_ASSIGN so this can't be assigned or copy-constructed. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:174: NrSocketIpc(nsCOMPtr<nsIEventTarget> main_thread); const nsCOMPtr<nsIEventTarget>& https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:175: virtual ~NrSocketIpc() { }; {} https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:188: void create_m(nsACString &host, uint16_t port); Should this be const? https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:189: void sendto_m(net::NetAddr &addr, nsAutoPtr<DataBuffer> buf); Const? https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:193: private: Space before private: https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:199: std::queue< RefPtr<nr_udp_message> > received_msgs_; <Ref not < Ref
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:269: static int nr_transport_addr_to_netaddr(nr_transport_addr *addr, On 2013/07/17 10:19:03, ekr-webrtc wrote: > Use PRNetAddrToNetAddr here instead of duplicating the code. Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:539: NrSocketIpc::NrSocketIpc(nsCOMPtr<nsIEventTarget> main_thread) On 2013/07/17 10:19:03, ekr-webrtc wrote: > const nsCOMPtr<nsIEventTarget>& Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:574: return NS_OK; On 2013/07/17 10:19:03, ekr-webrtc wrote: > Is NS_OK what we really want here? The assert suggests that this is a can't > happen. Does that mean we want to return some error? Maybe set an error flag? I think the error inside the callback function should only be handled inside the callback. Return NS_OK here means the callback is invoked successfully. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:766: ReentrantMonitorAutoEnter mon(monitor_); On 2013/07/17 10:19:03, ekr-webrtc wrote: > Do this at the top. Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:882: sock = new NrSocketIpc(main_thread.forget()); On 2013/07/17 10:19:03, ekr-webrtc wrote: > Why are you doing forget here? nsCOMPtr<nsIThread> cannot be converted to nsCOMPtr<nsIEventTarget> directly. Using forget() will get a nsIThread* pointer, which can be converted to nsIEventTarget* and nsCOMPtr<nsIEventTarget>. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:162: struct nr_udp_message : public RefCounted<nr_udp_message> { On 2013/07/17 10:19:03, ekr-webrtc wrote: > I typically use the NS_INLINE_DECL_ macros Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:164: nsAutoPtr<DataBuffer> data; On 2013/07/17 10:19:03, ekr-webrtc wrote: > Use private:DISALLOW_COPY_ASSIGN so this can't be assigned or copy-constructed. Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:174: NrSocketIpc(nsCOMPtr<nsIEventTarget> main_thread); On 2013/07/17 10:19:03, ekr-webrtc wrote: > const nsCOMPtr<nsIEventTarget>& Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:175: virtual ~NrSocketIpc() { }; On 2013/07/17 10:19:03, ekr-webrtc wrote: > {} Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:188: void create_m(nsACString &host, uint16_t port); On 2013/07/17 10:19:03, ekr-webrtc wrote: > Should this be const? Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:189: void sendto_m(net::NetAddr &addr, nsAutoPtr<DataBuffer> buf); On 2013/07/17 10:19:03, ekr-webrtc wrote: > Const? Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:193: private: On 2013/07/17 10:19:03, ekr-webrtc wrote: > Space before private: Done. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:199: std::queue< RefPtr<nr_udp_message> > received_msgs_; On 2013/07/17 10:19:03, ekr-webrtc wrote: > <Ref not < Ref Done.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:574: return NS_OK; On 2013/07/18 07:09:13, schien wrote: > On 2013/07/17 10:19:03, ekr-webrtc wrote: > > Is NS_OK what we really want here? The assert suggests that this is a can't > > happen. Does that mean we want to return some error? Maybe set an error flag? > > I think the error inside the callback function should only be handled inside the > callback. Return NS_OK here means the callback is invoked successfully. Fair enough. Should we set an error flag? https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:882: sock = new NrSocketIpc(main_thread.forget()); On 2013/07/18 07:09:13, schien wrote: > On 2013/07/17 10:19:03, ekr-webrtc wrote: > > Why are you doing forget here? > > nsCOMPtr<nsIThread> cannot be converted to nsCOMPtr<nsIEventTarget> directly. > Using forget() will get a nsIThread* pointer, which can be converted to > nsIEventTarget* and nsCOMPtr<nsIEventTarget>. I would suggest doing .get() instead, just b/c we're not really doing an ownership xfer.
Sign in to reply to this message.
A few small nits. I think we should be good in the next cycle. https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:574: return NS_OK; On 2013/07/18 07:13:19, ekr-webrtc wrote: > On 2013/07/18 07:09:13, schien wrote: > > On 2013/07/17 10:19:03, ekr-webrtc wrote: > > > Is NS_OK what we really want here? The assert suggests that this is a can't > > > happen. Does that mean we want to return some error? Maybe set an error > flag? > > > > I think the error inside the callback function should only be handled inside > the > > callback. Return NS_OK here means the callback is invoked successfully. > > Fair enough. Should we set an error flag? I don't see a change here. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:269: static int nr_transport_addr_to_netaddr(nr_transport_addr *addr, Since you can't link to PRNetAddrToNetAddr, suggest you copy it here instead. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:539: NrSocketIpc::NrSocketIpc(const nsCOMPtr<nsIEventTarget> main_thread) You forgot the &. The point here is to remove the AddRef/Delete cycle when the arguments are copied in. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:597: err_ = true; Please confirm these are thread-safe. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:183: NrSocketIpc(const nsCOMPtr<nsIEventTarget> main_thread); & again/
Sign in to reply to this message.
On 2013/07/26 01:39:48, ekr wrote: > A few small nits. I think we should be good in the next cycle. > > https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... > File media/mtransport/nr_socket_prsock.cpp (right): > > https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_s... > media/mtransport/nr_socket_prsock.cpp:574: return NS_OK; > On 2013/07/18 07:13:19, ekr-webrtc wrote: > > On 2013/07/18 07:09:13, schien wrote: > > > On 2013/07/17 10:19:03, ekr-webrtc wrote: > > > > Is NS_OK what we really want here? The assert suggests that this is a > can't > > > > happen. Does that mean we want to return some error? Maybe set an error > > flag? > > > > > > I think the error inside the callback function should only be handled inside > > the > > > callback. Return NS_OK here means the callback is invoked successfully. > > > > Fair enough. Should we set an error flag? > > I don't see a change here. The error flag is set by |err_ = true;| in CallbackListenerVoid, do I misunderstand what you mean?
Sign in to reply to this message.
Isn't this in CAllListenerArrayBuffer? On Thu, Jul 25, 2013 at 6:51 PM, <schien@mozilla.com> wrote: > On 2013/07/26 01:39:48, ekr wrote: > >> A few small nits. I think we should be good in the next cycle. >> > > > https://firefox-codereview.**appspot.com/20001/diff/18001/** > media/mtransport/nr_socket_**prsock.cpp<https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp> > >> File media/mtransport/nr_socket_**prsock.cpp (right): >> > > > https://firefox-codereview.**appspot.com/20001/diff/18001/** > media/mtransport/nr_socket_**prsock.cpp#newcode574<https://firefox-codereview.appspot.com/20001/diff/18001/media/mtransport/nr_socket_prsock.cpp#newcode574> > >> media/mtransport/nr_socket_**prsock.cpp:574: return NS_OK; >> On 2013/07/18 07:13:19, ekr-webrtc wrote: >> > On 2013/07/18 07:09:13, schien wrote: >> > > On 2013/07/17 10:19:03, ekr-webrtc wrote: >> > > > Is NS_OK what we really want here? The assert suggests that this >> > is a > >> can't >> > > > happen. Does that mean we want to return some error? Maybe set >> > an error > >> > flag? >> > > >> > > I think the error inside the callback function should only be >> > handled inside > >> > the >> > > callback. Return NS_OK here means the callback is invoked >> > successfully. > >> > >> > Fair enough. Should we set an error flag? >> > > I don't see a change here. >> > The error flag is set by |err_ = true;| in CallbackListenerVoid, do I > misunderstand what you mean? > > https://firefox-codereview.**appspot.com/20001/<https://firefox-codereview.ap...> >
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:269: static int nr_transport_addr_to_netaddr(nr_transport_addr *addr, On 2013/07/26 01:39:48, ekr wrote: > Since you can't link to PRNetAddrToNetAddr, suggest you copy it here instead. Done. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:539: NrSocketIpc::NrSocketIpc(const nsCOMPtr<nsIEventTarget> main_thread) On 2013/07/26 01:39:48, ekr wrote: > You forgot the &. The point here is to remove the AddRef/Delete cycle when the > arguments are copied in. Done. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:597: err_ = true; On 2013/07/26 01:39:48, ekr wrote: > Please confirm these are thread-safe. Done. https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/23001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:183: NrSocketIpc(const nsCOMPtr<nsIEventTarget> main_thread); On 2013/07/26 01:39:48, ekr wrote: > & again/ Done.
Sign in to reply to this message.
I went through the whole bug and found a few more issues. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:283: } This needs to be able to fail if the family is something else. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:363: nsACString *host, int32_t *port) { Does this compile properly in the unit tests? I remember having string problems. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:542: r_log_e(LOG_GENERIC,LOG_ERR, "UDP socket error:%s", message.BeginReading()); Please change all the r_log_es in this file to r_log. We shouldn't be trying to read errno. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:569: if (PR_SUCCESS != PR_SetNetAddr(PR_IpAddrNull, addr.raw.family, port, &addr)) { I think a comment here about what Null does would be appropriate. I had to look it up. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:586: NS_IMETHODIMP NrSocketIpc::CallListenerVoid(const nsACString &type) { Maybe this could be renamed to better match what it does? CallListenerOnOpen? https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:762: mozilla::RefPtr<nr_udp_message> &msg = received_msgs_.front(); What happens if this is empty? I bet a crash. you should be checking for empty first. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:769: ABORT(R_IO_ERROR); //XXX other error code? Probably R_WOOULDBLOCK https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:788: ASSERT_ON_THREAD(sts_thread_); Check the state to make sure we have gotten the response. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:803: NS_ENSURE_SUCCESS_VOID(socket_child_->Bind(this, host, port)); How do we handle errors in these? Shouldn't we be setting err_ here and below? https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:867: static bool IsChromeProcess() { Is there a reason for this extra fxn since you only use it once? https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:116: memset(&my_addr_, 0, sizeof(my_addr_)); Can we put my_addr_ into the base? It seems to be common. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:162: }; Let's put this in NrSocketIpc https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:165: nr_udp_message() {} Either make this initialize sensibly or remove it. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:197: void create_m(const nsACString &host, const uint16_t port); Can these be private?
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:283: } On 2013/08/08 22:26:10, ekr-webrtc wrote: > This needs to be able to fail if the family is something else. Done. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:363: nsACString *host, int32_t *port) { On 2013/08/08 22:26:10, ekr-webrtc wrote: > Does this compile properly in the unit tests? I remember having string problems. No compile error been found on both my local machine and try server. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:542: r_log_e(LOG_GENERIC,LOG_ERR, "UDP socket error:%s", message.BeginReading()); On 2013/08/08 22:26:10, ekr-webrtc wrote: > Please change all the r_log_es in this file to r_log. We shouldn't be trying to > read errno. Done. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:569: if (PR_SUCCESS != PR_SetNetAddr(PR_IpAddrNull, addr.raw.family, port, &addr)) { On 2013/08/08 22:26:10, ekr-webrtc wrote: > I think a comment here about what Null does would be appropriate. I had to look > it up. Done. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:586: NS_IMETHODIMP NrSocketIpc::CallListenerVoid(const nsACString &type) { On 2013/08/08 22:26:10, ekr-webrtc wrote: > Maybe this could be renamed to better match what it does? > > CallListenerOnOpen? This is actually used as a callback from parent process without any parameter. "onopen" and "onclose" events are using this callback. I'd like to keep the name Void for identifying callback an event without attribute. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:762: mozilla::RefPtr<nr_udp_message> &msg = received_msgs_.front(); On 2013/08/08 22:26:10, ekr-webrtc wrote: > What happens if this is empty? I bet a crash. > > you should be checking for empty first. It's undefined behavior by spec. I'll check empty first. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:769: ABORT(R_IO_ERROR); //XXX other error code? On 2013/08/08 22:26:10, ekr-webrtc wrote: > Probably R_WOOULDBLOCK Done. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:788: ASSERT_ON_THREAD(sts_thread_); On 2013/08/08 22:26:10, ekr-webrtc wrote: > Check the state to make sure we have gotten the response. Done. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:803: NS_ENSURE_SUCCESS_VOID(socket_child_->Bind(this, host, port)); On 2013/08/08 22:26:10, ekr-webrtc wrote: > How do we handle errors in these? Shouldn't we be setting err_ here and below? I'll set err_ here. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:867: static bool IsChromeProcess() { On 2013/08/08 22:26:10, ekr-webrtc wrote: > Is there a reason for this extra fxn since you only use it once? In previous revision this function was used multiple times. I'll remove the function since it only is used in one place now. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:116: memset(&my_addr_, 0, sizeof(my_addr_)); On 2013/08/08 22:26:10, ekr-webrtc wrote: > Can we put my_addr_ into the base? It seems to be common. In google coding style data member should be private. Do you suggest to move my_addr_ to NrSocketBase as a protected member? https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:162: }; On 2013/08/08 22:26:10, ekr-webrtc wrote: > Let's put this in NrSocketIpc Done. https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:165: nr_udp_message() {} On 2013/08/08 22:26:10, ekr-webrtc wrote: > Either make this initialize sensibly or remove it. Need to explicitly declare a constructor because DISALLOW_COPY_ASSIGN will prevent a default constructor been generated by compiler. I'll change this dummy constructor to nr_udp_message(PRNetAddr&, nsAutoPtr&). https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:197: void create_m(const nsACString &host, const uint16_t port); On 2013/08/08 22:26:10, ekr-webrtc wrote: > Can these be private? Originally I thought RunnableWrapper can only be applied on public method, but it surprises me.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:586: NS_IMETHODIMP NrSocketIpc::CallListenerVoid(const nsACString &type) { On 2013/08/09 10:13:41, schien wrote: > On 2013/08/08 22:26:10, ekr-webrtc wrote: > > Maybe this could be renamed to better match what it does? > > > > CallListenerOnOpen? > > This is actually used as a callback from parent process without any parameter. > "onopen" and "onclose" events are using this callback. I'd like to keep the name > Void for identifying callback an event without attribute. I don't think the void is a problem, but i'd like to label it some way that explains what the purpose is https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/30001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:116: memset(&my_addr_, 0, sizeof(my_addr_)); On 2013/08/09 10:13:41, schien wrote: > On 2013/08/08 22:26:10, ekr-webrtc wrote: > > Can we put my_addr_ into the base? It seems to be common. > > In google coding style data member should be private. Do you suggest to move > my_addr_ to NrSocketBase as a protected member? That was what I had in mind, though I see it's banned by Google style. I think this is OK to deviate from. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:308: praddr_to_netaddr(&praddr, naddr); Check return value here.... https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:581: // Use PR_IpAddrNull to avoid address been reset to 0. s/been/being/ https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:589: nsAutoPtr<DataBuffer> buf(new DataBuffer(data, data_length)); Let's move the construction of the udp_message here so that that way we don't need to do a copy of the addr. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:601: if (type.EqualsLiteral("onopen")) { Can anything else besides onopen happen here? If so, what does it mean? https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:682: MOZ_ASSERT(false, "Failed to create STS thread"); s/create/get/ https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:691: if ((r=nr_transport_addr_copy(&my_addr_,addr))) { space here after comma. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:703: mon.Wait(); Add a comment here to the effect that this is actually waiting for open to succed. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:763: std::swap( received_msgs_, empty); no space after ( https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:783: { What is the block doing here? https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:786: if ((r=nr_praddr_to_transport_addr(&msg->from, from, 0))) { Should set err_ here? https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:795: received_msgs_.pop(); Pop right after .front() so that address errors don't leave us in a borked state. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:839: buf->data(), Check for error and maybe set err_ https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:113: , public nsASocketHandler { Convention here is to have the comma at the end of the previous line. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:157: nr_udp_message(const PRNetAddr &from, nsAutoPtr<DataBuffer> &data) { please us an initializer here. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:172: , public nsIUDPSocketInternal { ,s at end please.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.cpp (right): https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:308: praddr_to_netaddr(&praddr, naddr); On 2013/08/09 19:28:14, ekr-webrtc wrote: > Check return value here.... Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:581: // Use PR_IpAddrNull to avoid address been reset to 0. On 2013/08/09 19:28:14, ekr-webrtc wrote: > s/been/being/ Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:601: if (type.EqualsLiteral("onopen")) { On 2013/08/09 19:28:14, ekr-webrtc wrote: > Can anything else besides onopen happen here? If so, what does it mean? "onclose" can happen here and it means socket is closed on parent process accidentally. I handle the close event in UpdateReadyState since we modify state_ there. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:682: MOZ_ASSERT(false, "Failed to create STS thread"); On 2013/08/09 19:28:14, ekr-webrtc wrote: > s/create/get/ Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:691: if ((r=nr_transport_addr_copy(&my_addr_,addr))) { On 2013/08/09 19:28:14, ekr-webrtc wrote: > space here after comma. Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:703: mon.Wait(); On 2013/08/09 19:28:14, ekr-webrtc wrote: > Add a comment here to the effect that this is actually waiting for open to > succed. Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:763: std::swap( received_msgs_, empty); On 2013/08/09 19:28:14, ekr-webrtc wrote: > no space after ( Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:783: { On 2013/08/09 19:28:14, ekr-webrtc wrote: > What is the block doing here? Variable declaration after goto statement, i.e. ABORT(...), will get a compile error. Additional block will prevent |msg| being seen after |abort| label and can prevent the compile error. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:786: if ((r=nr_praddr_to_transport_addr(&msg->from, from, 0))) { On 2013/08/09 19:28:14, ekr-webrtc wrote: > Should set err_ here? I was thinking not to kill the entire socket by an error of a single incoming packet. Ok for me to set err_ here if we want to fail early. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:795: received_msgs_.pop(); On 2013/08/09 19:28:14, ekr-webrtc wrote: > Pop right after .front() so that address errors don't leave us in a borked > state. Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.cpp:839: buf->data(), On 2013/08/09 19:28:14, ekr-webrtc wrote: > Check for error and maybe set err_ Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... File media/mtransport/nr_socket_prsock.h (right): https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:113: , public nsASocketHandler { On 2013/08/09 19:28:14, ekr-webrtc wrote: > Convention here is to have the comma at the end of the previous line. Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:157: nr_udp_message(const PRNetAddr &from, nsAutoPtr<DataBuffer> &data) { On 2013/08/09 19:28:14, ekr-webrtc wrote: > please us an initializer here. Done. https://firefox-codereview.appspot.com/20001/diff/36001/media/mtransport/nr_s... media/mtransport/nr_socket_prsock.h:172: , public nsIUDPSocketInternal { On 2013/08/09 19:28:14, ekr-webrtc wrote: > ,s at end please. Done.
Sign in to reply to this message.
This is looking good. I don't see any tests, however. Can you tell me what you did to test this? http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:376: int nr_transport_addr_get_addrstring_and_port(nr_transport_addr *addr, Please add a comment about the form this comes out in. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:550: NS_IMETHODIMP NrSocketIpc::CallListenerError(const nsACString &type, Please add comments indicating what each of these fxns does. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:555: ASSERT_ON_THREAD(main_thread_); Please mark the unused values as unused. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:565: NS_IMETHODIMP NrSocketIpc::CallListenerReceivedData(const nsACString &type, What is type here and below. Please either check or mark unused. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:634: if (nr_praddr_to_transport_addr(&praddr, &my_addr_, 1)) { Check that the non-port part of my_addr_ is as expected. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:666: state_ = NR_CLOSED; Please also have checks/asserts for the other states. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:710: // Waiting until socket creation complete s/Waiting/Wait/ and . at end http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:805: consumed_len = std::min(maxlen, msg->data->len()); Please log if we discarded data. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.h (right): http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.h:96: NS_IMETHOD_(nsrefcnt) AddRef(void) = 0; Please add a comment here. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.h:159: data(data) { These can go on one line.
Sign in to reply to this message.
http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:376: int nr_transport_addr_get_addrstring_and_port(nr_transport_addr *addr, On 2013/08/12 19:50:07, ekr-webrtc wrote: > Please add a comment about the form this comes out in. Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:550: NS_IMETHODIMP NrSocketIpc::CallListenerError(const nsACString &type, On 2013/08/12 19:50:07, ekr-webrtc wrote: > Please add comments indicating what each of these fxns does. Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:555: ASSERT_ON_THREAD(main_thread_); On 2013/08/12 19:50:07, ekr-webrtc wrote: > Please mark the unused values as unused. I'll use all of the parameters in error log. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:565: NS_IMETHODIMP NrSocketIpc::CallListenerReceivedData(const nsACString &type, On 2013/08/12 19:50:07, ekr-webrtc wrote: > What is type here and below. Please either check or mark unused. Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:634: if (nr_praddr_to_transport_addr(&praddr, &my_addr_, 1)) { On 2013/08/12 19:50:07, ekr-webrtc wrote: > Check that the non-port part of my_addr_ is as expected. Do you mean we should compare the IP address of praddr with the IP address already stored in my_addr_, then update the port number if these two address are the same? http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:666: state_ = NR_CLOSED; On 2013/08/12 19:50:07, ekr-webrtc wrote: > Please also have checks/asserts for the other states. Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:710: // Waiting until socket creation complete On 2013/08/12 19:50:07, ekr-webrtc wrote: > s/Waiting/Wait/ and . at end Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:805: consumed_len = std::min(maxlen, msg->data->len()); On 2013/08/12 19:50:07, ekr-webrtc wrote: > Please log if we discarded data. Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.h (right): http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.h:96: NS_IMETHOD_(nsrefcnt) AddRef(void) = 0; On 2013/08/12 19:50:07, ekr-webrtc wrote: > Please add a comment here. Done. http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.h:159: data(data) { On 2013/08/12 19:50:07, ekr-webrtc wrote: > These can go on one line. Done.
Sign in to reply to this message.
This looks basically fine. You should be able to land it once it's tested http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/32004/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:634: if (nr_praddr_to_transport_addr(&praddr, &my_addr_, 1)) { On 2013/08/15 06:17:36, schien wrote: > On 2013/08/12 19:50:07, ekr-webrtc wrote: > > Check that the non-port part of my_addr_ is as expected. > > Do you mean we should compare the IP address of praddr with the IP address > already stored in my_addr_, then update the port number if these two address are > the same? Yes. This is what I was looking for. http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:646: nr_transport_addr_copy(&expected_addr, &my_addr_); Please check for errors here. http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:819: RefPtr<nr_udp_message> msg(received_msgs_.front()); Why did you change this?
Sign in to reply to this message.
http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so... File media/mtransport/nr_socket_prsock.cpp (right): http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:646: nr_transport_addr_copy(&expected_addr, &my_addr_); On 2013/08/15 21:06:54, ekr-webrtc wrote: > Please check for errors here. Done. http://firefox-codereview.appspot.com/20001/diff/41001/media/mtransport/nr_so... media/mtransport/nr_socket_prsock.cpp:819: RefPtr<nr_udp_message> msg(received_msgs_.front()); On 2013/08/15 21:06:54, ekr-webrtc wrote: > Why did you change this? Originally msg is a reference to RefPtr, which doesn't increase the reference count. Doing pop before use might cause SEGV, therefore, I change the type of msg for explicitly holding the reference. Furthermore, namespace "mozilla" is redundant so I simply remove it.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
