Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751953002:
sendBeacon(): once transmission allowance has been reached, always fail. (Closed)
DescriptionsendBeacon(): once transmission allowance has been reached, always fail.
Fix allowance checking logic for Beacon transmissions upon reaching
the limit. If the allowance limit was 'perfectly' exhausted after N
Beacon requests, subsequent Beacon requests would go ahead without
the (now zero) allowance limit imposed.
R=mkwst
BUG=701678
Review-Url: https://codereview.chromium.org/2751953002
Cr-Commit-Position: refs/heads/master@{#457088}
Committed: https://chromium.googlesource.com/chromium/src/+/c1a211aa583ade023aca652e9493c01603623aa3
Patch Set 1 #Patch Set 2 : fix test description imprecision #
Messages
Total messages: 27 (13 generated)
The CQ bit was checked by sigbjornf@opera.com 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...
sigbjornf@opera.com changed reviewers: + mkwst@chromium.org, tyoshino@chromium.org
please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com 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: This issue passed the CQ dry run.
Ha! Nice find. LGTM!
(I'm assuming this is a Chrome thing and not a specced limit? If I'm wrong, consider moving the test to //external/wpt so other vendors can reap the benefits).
On 2017/03/15 14:57:30, Mike West (Slow.) wrote: > (I'm assuming this is a Chrome thing and not a specced limit? If I'm wrong, > consider moving the test to //external/wpt so other vendors can reap the > benefits). Spec only imposes a "SHOULD" wrt some limit in https://www.w3.org/TR/beacon/#sec-sendBeacon-method. The Blink implementation went with a moderate 64k ( https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Set... ), but others seem to have a higher (or no) limit. https://www.chromestatus.com/metrics/feature/timeline/popularity/495 captures when that limit was exceeded ( https://www.chromestatus.com/metrics/feature/timeline/popularity/494 for overall SendBeacon usage.)
Description was changed from ========== sendBeacon(): once transmission allowance has been reached, always fail. Fix allowance checking logic for Beacon transmissions upon reaching the limit. If the allowance limit was 'perfectly' exhausted after N Beacon requests, subsequent Beacon requests would go ahead without the (now zero) allowance limit imposed. R= BUG=701678 ========== to ========== sendBeacon(): once transmission allowance has been reached, always fail. Fix allowance checking logic for Beacon transmissions upon reaching the limit. If the allowance limit was 'perfectly' exhausted after N Beacon requests, subsequent Beacon requests would go ahead without the (now zero) allowance limit imposed. R=mkwst BUG=701678 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489591542832240,
"parent_rev": "7951f084cb9266ee3ca752a11a54228d35b3145e", "commit_rev":
"c1a211aa583ade023aca652e9493c01603623aa3"}
Message was sent while issue was closed.
Description was changed from ========== sendBeacon(): once transmission allowance has been reached, always fail. Fix allowance checking logic for Beacon transmissions upon reaching the limit. If the allowance limit was 'perfectly' exhausted after N Beacon requests, subsequent Beacon requests would go ahead without the (now zero) allowance limit imposed. R=mkwst BUG=701678 ========== to ========== sendBeacon(): once transmission allowance has been reached, always fail. Fix allowance checking logic for Beacon transmissions upon reaching the limit. If the allowance limit was 'perfectly' exhausted after N Beacon requests, subsequent Beacon requests would go ahead without the (now zero) allowance limit imposed. R=mkwst BUG=701678 Review-Url: https://codereview.chromium.org/2751953002 Cr-Commit-Position: refs/heads/master@{#457088} Committed: https://chromium.googlesource.com/chromium/src/+/c1a211aa583ade023aca652e9493... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c1a211aa583ade023aca652e9493...
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
On 2017/03/15 15:35:47, tyoshino wrote: > lgtm Seems we should fix maxBeaconTransmission to be unsigned and assert allowance being non-negative here.
Message was sent while issue was closed.
On 2017/03/15 15:38:16, tyoshino wrote: > On 2017/03/15 15:35:47, tyoshino wrote: > > lgtm > > Seems we should fix maxBeaconTransmission to be unsigned and assert allowance > being non-negative here. The assert holds regardless of rep; that it didn't handle the zero case was the missing part here.
Message was sent while issue was closed.
On 2017/03/15 15:45:05, sof wrote: > On 2017/03/15 15:38:16, tyoshino wrote: > > On 2017/03/15 15:35:47, tyoshino wrote: > > > lgtm > > > > Seems we should fix maxBeaconTransmission to be unsigned and assert allowance > > being non-negative here. > > The assert holds regardless of rep; that it didn't handle the zero case was the > missing part here. As I lgtm-ed, the fix is fine. I just suggested that we could also remove "allowance < 0 ||" and assert "allowance >= 0" instead there.
Message was sent while issue was closed.
On 2017/03/15 15:48:53, tyoshino wrote: > On 2017/03/15 15:45:05, sof wrote: > > On 2017/03/15 15:38:16, tyoshino wrote: > > > On 2017/03/15 15:35:47, tyoshino wrote: > > > > lgtm > > > > > > Seems we should fix maxBeaconTransmission to be unsigned and assert > allowance > > > being non-negative here. > > > > The assert holds regardless of rep; that it didn't handle the zero case was > the > > missing part here. > > As I lgtm-ed, the fix is fine. I just suggested that we could also remove > "allowance < 0 ||" and assert "allowance >= 0" instead there. Unless, we want to give a special meaning to allowance being negative here.
Message was sent while issue was closed.
On 2017/03/15 15:49:45, tyoshino wrote: > On 2017/03/15 15:48:53, tyoshino wrote: > > On 2017/03/15 15:45:05, sof wrote: > > > On 2017/03/15 15:38:16, tyoshino wrote: > > > > On 2017/03/15 15:35:47, tyoshino wrote: > > > > > lgtm > > > > > > > > Seems we should fix maxBeaconTransmission to be unsigned and assert > > allowance > > > > being non-negative here. > > > > > > The assert holds regardless of rep; that it didn't handle the zero case was > > the > > > missing part here. > > > > As I lgtm-ed, the fix is fine. I just suggested that we could also remove > > "allowance < 0 ||" and assert "allowance >= 0" instead there. > > Unless, we want to give a special meaning to allowance being negative here. I think that was the intent at some point, to have an embedder be able to set a negative max to opt out of any limit. But the implementation doesn't support that, so unless we feel that's valuable, switching to 'unsigned' makes good sense.
Message was sent while issue was closed.
On 2017/03/15 16:07:23, sof wrote: > I think that was the intent at some point, to have an embedder be able to set a > negative max to opt out of any limit. But the implementation doesn't support > that, so unless we feel that's valuable, switching to 'unsigned' makes good > sense. OK. Anyway, let's decide it at some point and document in Settings.json5 :)
Message was sent while issue was closed.
On 2017/03/15 16:08:25, tyoshino wrote: > On 2017/03/15 16:07:23, sof wrote: > > I think that was the intent at some point, to have an embedder be able to set > a > > negative max to opt out of any limit. But the implementation doesn't support > > that, so unless we feel that's valuable, switching to 'unsigned' makes good > > sense. > > OK. Anyway, let's decide it at some point and document in Settings.json5 :) https://codereview.chromium.org/2753863003/ is my proposal |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
