Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750853002:
TaskQueue[Win] DOS handling (Closed)
DescriptionTaskQueue[Win] DOS handling
BUG=webrtc:7341
Review-Url: https://codereview.webrtc.org/2750853002
Cr-Commit-Position: refs/heads/master@{#17242}
Committed: https://chromium.googlesource.com/external/webrtc/+/83722268d67f5bd28e77fd86678600d5ee461c81
Patch Set 1 #Patch Set 2 : Cleanup #
Messages
Total messages: 19 (13 generated)
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14885)
Cleanup
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14889)
Description was changed from ========== TaskQueue[Win] DOS handling BUG= ========== to ========== TaskQueue[Win] DOS handling BUG=webrtc:7341 ==========
tommi@webrtc.org changed reviewers: + ilnik@webrtc.org
hej ilnik - I might not have much time to take care of the remaining things in TaskQueue that should accompany this CL, so I put down TODOs for now and will follow up later. This addresses the issues you ran into and also raises the limit of how many tasks can be pushed in a burst, to something closer to how the other implementations have.
On 2017/03/14 20:44:03, tommi (webrtc) wrote: > hej ilnik - I might not have much time to take care of the remaining things in > TaskQueue that should accompany this CL, so I put down TODOs for now and will > follow up later. This addresses the issues you ran into and also raises the > limit of how many tasks can be pushed in a burst, to something closer to how the > other implementations have. LGTM. Just out of curiosity, do you know why did everything break when 'while' were replaced with 'if' in the ProcessQueuedMessages()? Having timelimit there doesn't break anything, but it seems to be a potential source of bugs in the future.
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489577668038570,
"parent_rev": "35b7de480b1ae796e232fd862e4fa3e6704dca0e", "commit_rev":
"83722268d67f5bd28e77fd86678600d5ee461c81"}
Message was sent while issue was closed.
Description was changed from ========== TaskQueue[Win] DOS handling BUG=webrtc:7341 ========== to ========== TaskQueue[Win] DOS handling BUG=webrtc:7341 Review-Url: https://codereview.webrtc.org/2750853002 Cr-Commit-Position: refs/heads/master@{#17242} Committed: https://chromium.googlesource.com/external/webrtc/+/83722268d67f5bd28e77fd866... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/83722268d67f5bd28e77fd866...
Message was sent while issue was closed.
On 2017/03/15 08:37:18, ilnik wrote: > On 2017/03/14 20:44:03, tommi (webrtc) wrote: > > hej ilnik - I might not have much time to take care of the remaining things in > > TaskQueue that should accompany this CL, so I put down TODOs for now and will > > follow up later. This addresses the issues you ran into and also raises the > > limit of how many tasks can be pushed in a burst, to something closer to how > the > > other implementations have. > > LGTM. > Just out of curiosity, do you know why did everything break when 'while' were > replaced with 'if' in the ProcessQueuedMessages()? > Having timelimit there doesn't break anything, but it seems to be a potential > source of bugs in the future. In the testing I did, I saw that the windows message queue became full and PostThreadMessage started failing. E.g. in the TaskQueue.PostALot unit test. That limit is 0xffff iirc, so I'd like to double check if that was really what was happening in your case. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
