GCC is getting smarter. This looks bad: ../../Source/WebKit/WebProcess/Network/webrtc/LibWebRTCSocket.cpp:57:44: warning: ‘memset’ used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size] memset(&m_options, 1, MAX_SOCKET_OPTION); ^ I assume the intent of the code was to set all of m_options to 1, but it's only setting the first quarter of m_options (assuming the usual sizeof(int) == 4), right?
Created attachment 340210 [details] Patch
Comment on attachment 340210 [details] Patch Clearing flags on attachment: 340210 Committed r231755: <https://trac.webkit.org/changeset/231755>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40217250>
Comment on attachment 340210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340210&action=review > Source/WebKit/WebProcess/Network/webrtc/LibWebRTCSocket.cpp:57 > - memset(&m_options, 1, MAX_SOCKET_OPTION); > + memset(&m_options, 1, MAX_SOCKET_OPTION * sizeof(m_options[0])); This code is extremely peculiar and I don’t think our change is good. Why do we want to set each byte of all the integers in the m_options array to 1? That means the integers will be set to values like 0x01010101 = 16843009. If we want to set the options integers to 1 we need to use std::fill_n instead of std::memset.
I suggest: std::fill_n(&m_options[0], MAX_SOCKET_OPTION, 1);
Reverted r231755 for reason: Change is not correct Committed r231781: <https://trac.webkit.org/changeset/231781>
You're right, my patch was nonsense. Not good. I'm not sure if your suggestion is right either, though. I looked around to figure out what m_options is supposed to be, and found this in Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/socket.h: enum Option { OPT_DONTFRAGMENT, OPT_RCVBUF, // receive buffer size OPT_SNDBUF, // send buffer size OPT_NODELAY, // whether Nagle algorithm is enabled OPT_IPV6_V6ONLY, // Whether the socket is IPv6 only. OPT_DSCP, // DSCP code OPT_RTP_SENDTIME_EXTN_ID, // This is a non-traditional socket option param. // This is specific to libjingle and will be used // if SendTime option is needed at socket level. }; virtual int GetOption(Option opt, int* value) = 0; virtual int SetOption(Option opt, int value) = 0; m_options is indexed by Option, i.e. the first element of m_options is OPT_DONTFRAGMENT, the second element is OPT_RCVBUF, etc. So there are seven options/elements total in m_options, and each can be set to arbitrary integer values. So if we set each array element to 1, that would be equivalent to setting the value of all those options to 1 (DONTFRAGMENT = 1, receive buffer size = 1, send buffer size = 1, Nagle algorithm enabled = 1, IPv6 only = 1...). That seems like odd default behavior for a newly-constructed LibWebRTCSocket, so I'll guess that's probably not right. Youenn, any idea how it's supposed to work? P.S. The declaration of MAX_SOCKET_OPTION in LibWebRTCSocket.h will break in the future the next time a new Option is added, which is not great either.
Looks like the GetOption function treats -1 as a special value meaning not set. So maybe the intent was to initialize to -1? If so, then if we want these to be optional values that might not be set, std::optional is a clearer way to do it rather than using a magic value of -1. If we use std::optional we don’t need to use std::fill or std::memset since the constructor will take care of everything. But I’m sure Youenn has a better understanding of what we’re really after.
(In reply to Darin Adler from comment #9) > Looks like the GetOption function treats -1 as a special value meaning not > set. So maybe the intent was to initialize to -1? > > If so, then if we want these to be optional values that might not be set, > std::optional is a clearer way to do it rather than using a magic value of > -1. If we use std::optional we don’t need to use std::fill or std::memset > since the constructor will take care of everything. > > But I’m sure Youenn has a better understanding of what we’re really after. GetOption is not used in practice, SetOption is the one that is used. GetOption is indeed currently very broken :( We should probably do something like: - Use std::optional<> as suggested by Darin - If the std::optional<> value is not set, return -1. Ideally, we should be getting the value from NetworkProcess but I do not think this is worth it. Michael, I can go ahead if that is ok with you.
Sounds good.
Created attachment 340651 [details] Patch
Comment on attachment 340651 [details] Patch Wouldn't it be better to remove GetOption instead of returning potentially-stale values...?
(In reply to Michael Catanzaro from comment #13) > Comment on attachment 340651 [details] > Patch > > Wouldn't it be better to remove GetOption instead of returning > potentially-stale values...? We need to implement it to instantiate LibWebRTCSocket. GetOption is used in some unit tests, maybe logging as well. I doubt this could be upstreamed very easily. We could make the implementation even simpler by always returning -1 but this would increase breakage risk if some libwebrtc code starts to use GetOption for real. We could make it smarter, given the number of actually used options is pretty limited Mostly OPT_RCVBUF and OPT_SNDBUF are useful. OPT_NODELAY should be set for all TCP sockets. Or we could make it fully functional by doing IPC to the NetworkProcess. Given all these calls should be done in a background thread, we would not need to resort on sync IPC. This seems a bit like overkill to me.
FWIW, Chrome implementation is close to this lazy implementation. The main differences: - It has a static list of the options that Chrome supports. GetOption return -1 if not supported - If the value is not previously set, GetOption return 0 and the out value parameter is set to -1
Comment on attachment 340651 [details] Patch Attachment 340651 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7717459 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html http/tests/security/local-video-source-from-remote.html
Created attachment 340671 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340651 [details] Patch Clearing flags on attachment: 340651 Committed r231956: <https://trac.webkit.org/changeset/231956>