Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127653002:
Add policy to control valid UDP port range in WebRTC (Closed)
|
Created:
4 years, 5 months ago by Guido Urdaneta Modified:
4 years, 4 months ago Reviewers:
tommi (sloooow) - chröme, sky, jam, Thiemo Nagel, Mike West, Peter Kasting, Marijn Kruisselbrink, Ilya Sherman, cschuet (SLOW) CC:
asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, tnagel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd policy to control valid UDP port range in WebRTC
BUG=342476
Committed: https://crrev.com/ca055a5420598ca1b7cae3eb825829248d841de8
Cr-Commit-Position: refs/heads/master@{#411300}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 7
Patch Set 3 : fix bugs #Patch Set 4 : rebase + some fixes #
Total comments: 4
Patch Set 5 : sky's comments and rebase #
Total comments: 10
Patch Set 6 : tnagel's comments #Patch Set 7 : tnagel's comments #Messages
Total messages: 68 (28 generated)
guidou@chromium.org changed reviewers: + tommi@chromium.org
Hi, PTAL
https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_preferences_util.cc (right): https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:37: // Parses a string |range| with a port range in the form "<min>-<max>". This code is largely duplicated from remoting/protocol/port_range.cc. Is there a way to share that code that is simpler than copying it here? I tried putting it in components, but that looked like overkill.
lgtm - nice! https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_preferences_util.cc (right): https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:37: // Parses a string |range| with a port range in the form "<min>-<max>". On 2016/07/06 08:56:55, Guido Urdaneta wrote: > This code is largely duplicated from remoting/protocol/port_range.cc. > Is there a way to share that code that is simpler than copying it here? > I tried putting it in components, but that looked like overkill. I don't think it's a huge deal. The TODO below is good enough for now. https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:64: max_port_uint > UINT16_MAX) {} (could run git cl format) https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:8755: 'example_value': '10400-10409', nit: as an example, we might want to have a wider range. Incidentally, have you tested what happens if the range is set to be very small and you run an app such as Hangouts?
https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_preferences_util.cc (right): https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... chrome/browser/renderer_preferences_util.cc:64: max_port_uint > UINT16_MAX) On 2016/07/06 10:46:43, tommi-chrömium wrote: > {} > > (could run git cl format) Done. https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:8755: 'example_value': '10400-10409', On 2016/07/06 10:46:43, tommi-chrömium wrote: > nit: as an example, we might want to have a wider range. Incidentally, have you > tested what happens if the range is set to be very small and you run an app such > as Hangouts? Actually, it is not working as we expected since IPCPacketSocketFactory currently ignores port limits. https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory.... We'll have to fix that before landing this CL.
On 2016/07/07 09:27:55, Guido Urdaneta wrote: > https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... > File chrome/browser/renderer_preferences_util.cc (right): > > https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... > chrome/browser/renderer_preferences_util.cc:64: max_port_uint > UINT16_MAX) > On 2016/07/06 10:46:43, tommi-chrömium wrote: > > {} > > > > (could run git cl format) > > Done. > > https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... > components/policy/resources/policy_templates.json:8755: 'example_value': > '10400-10409', > On 2016/07/06 10:46:43, tommi-chrömium wrote: > > nit: as an example, we might want to have a wider range. Incidentally, have > you > > tested what happens if the range is set to be very small and you run an app > such > > as Hangouts? > > Actually, it is not working as we expected since IPCPacketSocketFactory > currently ignores port limits. > https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory.... > > We'll have to fix that before landing this CL. ach... looks like this isn't implemented at all even at the UDPSocket level. Will you take a look at that?
On 2016/07/07 09:55:07, tommi-chrömium wrote: > On 2016/07/07 09:27:55, Guido Urdaneta wrote: > > > https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... > > File chrome/browser/renderer_preferences_util.cc (right): > > > > > https://codereview.chromium.org/2127653002/diff/20001/chrome/browser/renderer... > > chrome/browser/renderer_preferences_util.cc:64: max_port_uint > UINT16_MAX) > > On 2016/07/06 10:46:43, tommi-chrömium wrote: > > > {} > > > > > > (could run git cl format) > > > > Done. > > > > > https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... > > File components/policy/resources/policy_templates.json (right): > > > > > https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... > > components/policy/resources/policy_templates.json:8755: 'example_value': > > '10400-10409', > > On 2016/07/06 10:46:43, tommi-chrömium wrote: > > > nit: as an example, we might want to have a wider range. Incidentally, have > > you > > > tested what happens if the range is set to be very small and you run an app > > such > > > as Hangouts? > > > > Actually, it is not working as we expected since IPCPacketSocketFactory > > currently ignores port limits. > > > https://cs.chromium.org/chromium/src/content/renderer/p2p/ipc_socket_factory.... > > > > We'll have to fix that before landing this CL. > > ach... looks like this isn't implemented at all even at the UDPSocket level. > Will you take a look at that? I'm looking into it.
https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:8755: 'example_value': '10400-10409', On 2016/07/06 10:46:43, tommi-chrömium wrote: > nit: as an example, we might want to have a wider range. Incidentally, have you > tested what happens if the range is set to be very small and you run an app such > as Hangouts? I tested this on top of an updated IpcSocketFactory and once Hangouts runs out of UDP ports it starts using TCP.
Great. Thanks for the update. On Mon, Jul 11, 2016 at 7:18 PM <guidou@chromium.org> wrote: > > > https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... > File components/policy/resources/policy_templates.json (right): > > > https://codereview.chromium.org/2127653002/diff/20001/components/policy/resou... > components/policy/resources/policy_templates.json:8755: 'example_value': > '10400-10409', > On 2016/07/06 10:46:43, tommi-chrömium wrote: > > nit: as an example, we might want to have a wider range. > Incidentally, have you > > tested what happens if the range is set to be very small and you run > an app such > > as Hangouts? > > I tested this on top of an updated IpcSocketFactory and once Hangouts > runs out of UDP ports it starts using TCP. > > https://codereview.chromium.org/2127653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by guidou@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...
guidou@chromium.org changed reviewers: + jochen@chromium.org, mek@chromium.org, pkasting@chromium.org, tnagel@chromium.org
mek@chromium.org: Please review changes in chrome/browser/extensions/api mkwst@chromium.org: Please review changes in content/common/ (IPC) and tools/metrics/histograms tnagel@chromium.org: Please review changes in chrome/browser/policy pkasting@chromium.org: Please review changes in chrome/browser/ui jochen@chromium.org: Please review changes in chrome/browser and content/public
Description was changed from ========== Add policy to control valid UDP port range in WebRTC BUG=342476 ========== to ========== Add policy to control valid UDP port range in WebRTC BUG=342476 ==========
guidou@chromium.org changed reviewers: + mkwst@chromium.org
guidou@chromium.org changed reviewers: + isherman@chromium.org
+isherman@chromium.org: Please review histograms.xml +mkwst@chromium.org: Please review IPC (content/common/view_messages.h)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml lgtm
extensions lgtm
Adding these two items to the IPC LGTM.
is everything in this CL already covered?
On 2016/08/02 15:29:40, jochen wrote: > is everything in this CL already covered? We are still waiting for specific owner approval for changes in */policy/ and chrome/browser/ui
c/b/ui/ LGTM
I guess that's covered by tnagel
On 2016/08/03 15:29:08, jochen wrote: > I guess that's covered by tnagel Yes, the set of reviewers covers everything in this CL. I thought you meant approval by lower-level owners in your previous question.
guidou@chromium.org changed reviewers: + cschuet@chromium.org - tnagel@chromium.org
cschuet: Can you take a look at */policy/* ?
On 2016/08/08 09:56:08, Guido Urdaneta wrote: > cschuet: Can you take a look at */policy/* ? LGTM
guidou@chromium.org changed reviewers: + sky@chromium.org
guidou@chromium.org changed reviewers: + pfeldman@chromium.org
guidou@chromium.org changed reviewers: - jochen@chromium.org
pfeldman@chromium.org: please review content/public sky@chromium.org: please review chrome/browser
LGTM https://codereview.chromium.org/2127653002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2127653002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:3759: }; nit: private: DISALLOW... https://codereview.chromium.org/2127653002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:3772: EXPECT_EQ("10000-10100", port_range); Document where this string comes from.
The CQ bit was checked by guidou@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...
https://codereview.chromium.org/2127653002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2127653002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:3759: }; On 2016/08/08 15:20:38, sky wrote: > nit: private: DISALLOW... Done. https://codereview.chromium.org/2127653002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:3772: EXPECT_EQ("10000-10100", port_range); On 2016/08/08 15:20:38, sky wrote: > Document where this string comes from. Done. Using a constant now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
guidou@chromium.org changed reviewers: + jam@chromium.org - pfeldman@chromium.org
+jam: can you take a look at content/public?
lgtm
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, sky@chromium.org, mkwst@chromium.org, pkasting@chromium.org, mek@chromium.org, isherman@chromium.org, cschuet@chromium.org Link to the patchset: https://codereview.chromium.org/2127653002/#ps80001 (title: "sky's comments and rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
guidou@chromium.org changed reviewers: + tnagel@chromium.org
tnagel: Can you take a look at components/policy/resources/policy_templates.json ?
https://codereview.chromium.org/2127653002/diff/80001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2127653002/diff/80001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:1955: "os": ["win", "linux", "mac", "chromeos", "android"], From policy_templates.json it seems that Android is not supported? https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8811: 'desc': '''Restricts the UDP port range used by WebRTC in this machine. Nit: I'm not in favour of duplicating the caption in the description (although there is ample precedent for it). https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8813: If this policy is left not set, or if it is set to an empty string or an invalid port range, WebRTC will be allowed to use any available port.''', Nit: In my opinion present tense is better style.
The CQ bit was checked by guidou@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...
https://codereview.chromium.org/2127653002/diff/80001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2127653002/diff/80001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:1955: "os": ["win", "linux", "mac", "chromeos", "android"], On 2016/08/11 09:29:26, Thiemo Nagel wrote: > From policy_templates.json it seems that Android is not supported? Fixed policy_templates.json. Android is supported. https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8811: 'desc': '''Restricts the UDP port range used by WebRTC in this machine. On 2016/08/11 09:29:26, Thiemo Nagel wrote: > Nit: I'm not in favour of duplicating the caption in the description (although > there is ample precedent for it). Done. https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8813: If this policy is left not set, or if it is set to an empty string or an invalid port range, WebRTC will be allowed to use any available port.''', On 2016/08/11 09:29:26, Thiemo Nagel wrote: > Nit: In my opinion present tense is better style. Done.
lgtm % comment https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8811: 'desc': '''Restricts the UDP port range used by WebRTC in this machine. > Done. As a second thought, I think it would be better spell out what the policy does a bit more explicitly, eg.: "If the policy is set, the UDP port range used by WebRTC is restricted to the specified port interval (endpoints included)." https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8813: If this policy is left not set, or if it is set to an empty string or an invalid port range, WebRTC will be allowed to use any available port.''', > Done. Optional nit: Actually it's still future tense. ;)
https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8811: 'desc': '''Restricts the UDP port range used by WebRTC in this machine. On 2016/08/11 10:22:31, Thiemo Nagel wrote: > > Done. > > As a second thought, I think it would be better spell out what the policy does a > bit more explicitly, eg.: "If the policy is set, the UDP port range used by > WebRTC is restricted to the specified port interval (endpoints included)." Done. https://codereview.chromium.org/2127653002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8813: If this policy is left not set, or if it is set to an empty string or an invalid port range, WebRTC will be allowed to use any available port.''', On 2016/08/11 10:22:32, Thiemo Nagel wrote: > > Done. > > Optional nit: Actually it's still future tense. ;) Done. I originally thought you meant you didn't like passive voice :)
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, sky@chromium.org, jam@chromium.org, mkwst@chromium.org, pkasting@chromium.org, mek@chromium.org, isherman@chromium.org, cschuet@chromium.org, tnagel@chromium.org Link to the patchset: https://codereview.chromium.org/2127653002/#ps120001 (title: "tnagel's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add policy to control valid UDP port range in WebRTC BUG=342476 ========== to ========== Add policy to control valid UDP port range in WebRTC BUG=342476 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add policy to control valid UDP port range in WebRTC BUG=342476 ========== to ========== Add policy to control valid UDP port range in WebRTC BUG=342476 Committed: https://crrev.com/ca055a5420598ca1b7cae3eb825829248d841de8 Cr-Commit-Position: refs/heads/master@{#411300} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ca055a5420598ca1b7cae3eb825829248d841de8 Cr-Commit-Position: refs/heads/master@{#411300}
Message was sent while issue was closed.
On 2016/08/11 at 11:18:10, commit-bot wrote: > Patchset 7 (id:??) landed as https://crrev.com/ca055a5420598ca1b7cae3eb825829248d841de8 > Cr-Commit-Position: refs/heads/master@{#411300} After debugging for three hours, I'm 99% sure this patch is the reason my local build is failing, due to the inconsistency between kWebRtcUdpPortRange and kWebRtcUDPPortRange (note case).
Message was sent while issue was closed.
On 2016/08/11 at 17:20:02, pilgrim_google wrote: > On 2016/08/11 at 11:18:10, commit-bot wrote: > > Patchset 7 (id:??) landed as https://crrev.com/ca055a5420598ca1b7cae3eb825829248d841de8 > > Cr-Commit-Position: refs/heads/master@{#411300} > > After debugging for three hours, I'm 99% sure this patch is the reason my local build is failing, due to the inconsistency between kWebRtcUdpPortRange and kWebRtcUDPPortRange (note case). Specifically: """ gen/policy/cloud_policy_generated.cc:6078:25: error: no member named 'kWebRtcUdpPortRange' in namespace 'policy::key' map->Set(key::kWebRtcUdpPortRange, ~~~~~^ 1 error generated. """ because its name is kWebRtcUDPPortRange (note case)
Message was sent while issue was closed.
On 2016/08/11 17:29:00, pilgrim_google wrote: > On 2016/08/11 at 17:20:02, pilgrim_google wrote: > > On 2016/08/11 at 11:18:10, commit-bot wrote: > > > Patchset 7 (id:??) landed as > https://crrev.com/ca055a5420598ca1b7cae3eb825829248d841de8 > > > Cr-Commit-Position: refs/heads/master@{#411300} > > > > After debugging for three hours, I'm 99% sure this patch is the reason my > local build is failing, due to the inconsistency between kWebRtcUdpPortRange and > kWebRtcUDPPortRange (note case). > > Specifically: > > """ > gen/policy/cloud_policy_generated.cc:6078:25: error: no member named > 'kWebRtcUdpPortRange' in namespace 'policy::key' > map->Set(key::kWebRtcUdpPortRange, > ~~~~~^ > 1 error generated. > """ > > because its name is kWebRtcUDPPortRange (note case) My understanding is that policy::key::kWebRtcUdpPortRange is generated from the policy_templates.json file, while the prefs::kWebRTCUDPPortRange is defined in chrome/common/pref_names.h They are associated in configuration_policy_handler_list_factory.cc and that should be it. The difference in case is due to the different conventions in the places where both names are defined, but I don't see anything wrong with that. The main waterfall seems to be OK with it too. tnagel: can you confirm?
Message was sent while issue was closed.
> My understanding is that policy::key::kWebRtcUdpPortRange is generated from the > policy_templates.json file, while the prefs::kWebRTCUDPPortRange is defined in > chrome/common/pref_names.h > They are associated in configuration_policy_handler_list_factory.cc and that > should be it. > The difference in case is due to the different conventions in the places where > both names are defined, but I don't see anything wrong with that. The main > waterfall seems to be OK with it too. > > tnagel: can you confirm? That sounds correct to me. Pilgrim, have you tried blowing away your out directory and recompiling?
Message was sent while issue was closed.
On 2016/08/12 at 12:20:50, tnagel wrote: > > My understanding is that policy::key::kWebRtcUdpPortRange is generated from the > > policy_templates.json file, while the prefs::kWebRTCUDPPortRange is defined in > > chrome/common/pref_names.h > > They are associated in configuration_policy_handler_list_factory.cc and that > > should be it. > > The difference in case is due to the different conventions in the places where > > both names are defined, but I don't see anything wrong with that. The main > > waterfall seems to be OK with it too. > > > > tnagel: can you confirm? > > That sounds correct to me. > > Pilgrim, have you tried blowing away your out directory and recompiling? No longer able to reproduce. No idea what changed. Sorry for the spam. |
