Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2378763003:
Add ability to reuse address in chrome.sockets.udp
DescriptionAdd ability to reuse address in chrome.sockets.udp
This commit adds the ability to flag that a socket allows
address reuse. Reusing an address is permitted by UDP, and
exposing this functionality allows Apps to do things like
implement an mDNS server. The mDNS address is bound by Chrome
by default, and despite the fact that when binding Chrome
flags the address as reusable, before this change Apps are
unable to indicate that they too permit address reuse. This
is only available in the newer chrome.sockets.udp API, and
thus is only implemented in ResumableUDPSocket.
BUG=497875
R=reillyg@chromium.org
R=rockot@chromium.org
Patch Set 1 #
Total comments: 4
Patch Set 2 : Respond to code review #
Total comments: 6
Patch Set 3 : Respond to code reviews, clean up tests #
Messages
Total messages: 18 (8 generated)
The CQ bit was checked by srsudar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/09/28 23:10:39, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) A similar CL was submitted a few years ago: https://codereview.chromium.org/15039012/ Amusingly this time it fails on Windows, whereas that time it failed on Mac.
This looks like it's on the right track. Since this is adding a new API feature with possible security implications please follow the process documented here to send a proposal to apps-dev@ and security-enamel@: https://www.chromium.org/developers/design-documents/extensions/proposed-chan... The proposal will be simpler that normal since you are extending an existing API. https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... File extensions/browser/api/socket/udp_socket.h (right): https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... extensions/browser/api/socket/udp_socket.h:123: bool allow_address_reuse_; Instead of adding this field to ResumableUDPSocket add it to UDPSocket and then you don't need an IsReusable() method. https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... File extensions/test/data/sockets_udp/api/background.js (right): https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... extensions/test/data/sockets_udp/api/background.js:110: 'Not all sockets created during test'); This assert is vacuously true. I assume you meant to test that numSocketsCreated == numSocketsExpected. numSocketsCreated should probably be incremented in onGetInfoAfterBind() instead of createAndBindToSharedAddress().
On 2016/09/29 06:55:55, Reilly Grant wrote: > This looks like it's on the right track. Since this is adding a new API feature > with possible security implications please follow the process documented here to > send a proposal to apps-dev@ and security-enamel@: > > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > The proposal will be simpler that normal since you are extending an existing > API. > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > File extensions/browser/api/socket/udp_socket.h (right): > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > extensions/browser/api/socket/udp_socket.h:123: bool allow_address_reuse_; > Instead of adding this field to ResumableUDPSocket add it to UDPSocket and then > you don't need an IsReusable() method. > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > File extensions/test/data/sockets_udp/api/background.js (right): > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > extensions/test/data/sockets_udp/api/background.js:110: 'Not all sockets created > during test'); > This assert is vacuously true. I assume you meant to test that numSocketsCreated > == numSocketsExpected. numSocketsCreated should probably be incremented in > onGetInfoAfterBind() instead of createAndBindToSharedAddress(). Will do. Any idea as to why AllowAddressReuse() doesn't seem to be working on the Windows tests? I believe error 147 is address in use, or at least it was the same on my linux workstation after adding the test but before implementing support. Strangely in the previous CL a few years ago it was the Mac tests that were failing. I don't see any changes in the log for udp_socket_win.cc that look relevant to me, but I could be missing them.
https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... File extensions/browser/api/socket/udp_socket.h (right): https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... extensions/browser/api/socket/udp_socket.h:123: bool allow_address_reuse_; On 2016/09/29 06:55:55, Reilly Grant wrote: > Instead of adding this field to ResumableUDPSocket add it to UDPSocket and then > you don't need an IsReusable() method. Done. https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... File extensions/test/data/sockets_udp/api/background.js (right): https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... extensions/test/data/sockets_udp/api/background.js:110: 'Not all sockets created during test'); On 2016/09/29 06:55:55, Reilly Grant wrote: > This assert is vacuously true. I assume you meant to test that numSocketsCreated > == numSocketsExpected. numSocketsCreated should probably be incremented in > onGetInfoAfterBind() instead of createAndBindToSharedAddress(). Done.
This is not an area of the codebase I know so my comments are more generic. https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... File extensions/test/data/sockets_udp/api/background.js (right): https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... extensions/test/data/sockets_udp/api/background.js:89: function onGetInfoBeforeBind(info) { nit: it would be clearer to me if these 3 methods were defined in their logical order: onGetInfoBeforeBind, onBind and then onGetInfoAfterBind. https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... extensions/test/data/sockets_udp/api/background.js:111: 'Not all sockets created during test'); Is this trying to check if too many sockets were created? I am assuming that is true because this code would not be reached if they were too few. If this is indeed the case than the error message is not clear. https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... extensions/test/data/sockets_udp/api/background.js:114: chrome.test.succeed(); IIUC you created numSocketsExpected to store the amount of times you would like to reuse the socket in this test. But here it seems you are hard coding that same information. You should choose either way but not mix both.
On 2016/09/29 at 15:53:31, srsudar wrote: > On 2016/09/29 06:55:55, Reilly Grant wrote: > > This looks like it's on the right track. Since this is adding a new API feature > > with possible security implications please follow the process documented here to > > send a proposal to apps-dev@ and security-enamel@: > > > > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > > > The proposal will be simpler that normal since you are extending an existing > > API. > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > > File extensions/browser/api/socket/udp_socket.h (right): > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > > extensions/browser/api/socket/udp_socket.h:123: bool allow_address_reuse_; > > Instead of adding this field to ResumableUDPSocket add it to UDPSocket and then > > you don't need an IsReusable() method. > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > > File extensions/test/data/sockets_udp/api/background.js (right): > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > > extensions/test/data/sockets_udp/api/background.js:110: 'Not all sockets created > > during test'); > > This assert is vacuously true. I assume you meant to test that numSocketsCreated > > == numSocketsExpected. numSocketsCreated should probably be incremented in > > onGetInfoAfterBind() instead of createAndBindToSharedAddress(). > > Will do. Any idea as to why AllowAddressReuse() doesn't seem to be working on the Windows tests? I believe error 147 is address in use, or at least it was the same on my linux workstation after adding the test but before implementing support. Strangely in the previous CL a few years ago it was the Mac tests that were failing. I don't see any changes in the log for udp_socket_win.cc that look relevant to me, but I could be missing them. IIRC the address reuse flags mean something different in Windows vs. POSIX.
https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... File extensions/test/data/sockets_udp/api/background.js (right): https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... extensions/test/data/sockets_udp/api/background.js:89: function onGetInfoBeforeBind(info) { On 2016/09/30 02:16:24, carlosk wrote: > nit: it would be clearer to me if these 3 methods were defined in their logical > order: onGetInfoBeforeBind, onBind and then onGetInfoAfterBind. Done. https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... extensions/test/data/sockets_udp/api/background.js:111: 'Not all sockets created during test'); On 2016/09/30 02:16:24, carlosk wrote: > Is this trying to check if too many sockets were created? I am assuming that is > true because this code would not be reached if they were too few. > > If this is indeed the case than the error message is not clear. Good point. I was trying to use this call to ensure that I wouldn't get an index out of bounds with the array access on the next line, but you're right that this isn't necessary given the enclosing if/else. Removing. https://codereview.chromium.org/2378763003/diff/20001/extensions/test/data/so... extensions/test/data/sockets_udp/api/background.js:114: chrome.test.succeed(); On 2016/09/30 02:16:24, carlosk wrote: > IIUC you created numSocketsExpected to store the amount of times you would like > to reuse the socket in this test. But here it seems you are hard coding that > same information. You should choose either way but not mix both. Done.
Non-owner LGTM.
On 2016/09/30 07:25:30, Reilly Grant wrote: > On 2016/09/29 at 15:53:31, srsudar wrote: > > On 2016/09/29 06:55:55, Reilly Grant wrote: > > > This looks like it's on the right track. Since this is adding a new API > feature > > > with possible security implications please follow the process documented > here to > > > send a proposal to apps-dev@ and security-enamel@: > > > > > > > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > > > > > The proposal will be simpler that normal since you are extending an existing > > > API. > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > > > File extensions/browser/api/socket/udp_socket.h (right): > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > > > extensions/browser/api/socket/udp_socket.h:123: bool allow_address_reuse_; > > > Instead of adding this field to ResumableUDPSocket add it to UDPSocket and > then > > > you don't need an IsReusable() method. > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > > > File extensions/test/data/sockets_udp/api/background.js (right): > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > > > extensions/test/data/sockets_udp/api/background.js:110: 'Not all sockets > created > > > during test'); > > > This assert is vacuously true. I assume you meant to test that > numSocketsCreated > > > == numSocketsExpected. numSocketsCreated should probably be incremented in > > > onGetInfoAfterBind() instead of createAndBindToSharedAddress(). > > > > Will do. Any idea as to why AllowAddressReuse() doesn't seem to be working on > the Windows tests? I believe error 147 is address in use, or at least it was the > same on my linux workstation after adding the test but before implementing > support. Strangely in the previous CL a few years ago it was the Mac tests that > were failing. I don't see any changes in the log for udp_socket_win.cc that look > relevant to me, but I could be missing them. > > IIRC the address reuse flags mean something different in Windows vs. POSIX. Based on the interface and comments in udp_socket_win.h, I'd expect vagaries of flags across platforms to be handled by the AllowAddressReuse() function. I'm not dealing with any of the flags myself but instead relying on that function. Of course, the failing tests might suggest this is the wrong approach. Here is the relevant line: https://cs.chromium.org/chromium/src/net/udp/udp_socket_win.h?q=udp_socket_wi... Is it worth emailing the OWNERS of that file for their take? This might be exactly the thing that killed the previous CL to add this functionality, but I'd love to be able to leave a bit more understanding for posterity at the least.
On 2016/09/30 at 20:04:33, srsudar wrote: > On 2016/09/30 07:25:30, Reilly Grant wrote: > > On 2016/09/29 at 15:53:31, srsudar wrote: > > > On 2016/09/29 06:55:55, Reilly Grant wrote: > > > > This looks like it's on the right track. Since this is adding a new API > > feature > > > > with possible security implications please follow the process documented > > here to > > > > send a proposal to apps-dev@ and security-enamel@: > > > > > > > > > > https://www.chromium.org/developers/design-documents/extensions/proposed-chan... > > > > > > > > The proposal will be simpler that normal since you are extending an existing > > > > API. > > > > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > > > > File extensions/browser/api/socket/udp_socket.h (right): > > > > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/browser/api/sock... > > > > extensions/browser/api/socket/udp_socket.h:123: bool allow_address_reuse_; > > > > Instead of adding this field to ResumableUDPSocket add it to UDPSocket and > > then > > > > you don't need an IsReusable() method. > > > > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > > > > File extensions/test/data/sockets_udp/api/background.js (right): > > > > > > > > > > https://codereview.chromium.org/2378763003/diff/1/extensions/test/data/socket... > > > > extensions/test/data/sockets_udp/api/background.js:110: 'Not all sockets > > created > > > > during test'); > > > > This assert is vacuously true. I assume you meant to test that > > numSocketsCreated > > > > == numSocketsExpected. numSocketsCreated should probably be incremented in > > > > onGetInfoAfterBind() instead of createAndBindToSharedAddress(). > > > > > > Will do. Any idea as to why AllowAddressReuse() doesn't seem to be working on > > the Windows tests? I believe error 147 is address in use, or at least it was the > > same on my linux workstation after adding the test but before implementing > > support. Strangely in the previous CL a few years ago it was the Mac tests that > > were failing. I don't see any changes in the log for udp_socket_win.cc that look > > relevant to me, but I could be missing them. > > > > IIRC the address reuse flags mean something different in Windows vs. POSIX. > > Based on the interface and comments in udp_socket_win.h, I'd expect vagaries of flags across platforms to be handled by the AllowAddressReuse() function. I'm not dealing with any of the flags myself but instead relying on that function. Of course, the failing tests might suggest this is the wrong approach. Here is the relevant line: > > https://cs.chromium.org/chromium/src/net/udp/udp_socket_win.h?q=udp_socket_wi... > > Is it worth emailing the OWNERS of that file for their take? This might be exactly the thing that killed the previous CL to add this functionality, but I'd love to be able to leave a bit more understanding for posterity at the least. You should ask about this on net-dev@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
