Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2191323005:
A couple fixes to net_data_job_fuzzer. (Closed)
|
Created:
4 years, 4 months ago by mmenke Modified:
4 years, 4 months ago Reviewers:
Charlie Harrison CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionA couple fixes to net_data_job_fuzzer.
In particular, the code was being run on non-data URLs, which caused it
to do weird and exciting things, and it had an incorrect DCHECK.
BUG=632605
Committed: https://crrev.com/4688e186bfb90fcba1725ff713e54af756cd00e9
Cr-Commit-Position: refs/heads/master@{#408737}
Patch Set 1 #Patch Set 2 : Remove DCHECK #Messages
Total messages: 18 (9 generated)
Patchset #1 (id:1) has been deleted
mmenke@chromium.org changed reviewers: + csharrison@chromium.org
It was making real HTTP requests, and they were traumatizing the test, poor thing.
On 2016/07/29 19:00:09, mmenke wrote: > It was making real HTTP requests, and they were traumatizing the test, poor > thing. Oh, the DCHECK's also wrong, I think, if the URL is invalid. Think I'll just remove it.
The CQ bit was checked by mmenke@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...
Ah good catch haha. eroman@ and I went back and forth on this but I didn't think about this case. Two questions: 1. Why is the DCHECK wrong? 2. Is the regression range just wrong do you think?
On 2016/07/29 19:05:49, csharrison wrote:
> Ah good catch haha. eroman@ and I went back and forth on this but I didn't
think
> about this case.
>
> Two questions:
> 1. Why is the DCHECK wrong?
if (!data_url.is_valid())
data_url = GURL("data:text/html;charset=utf-8,<p>test</p>");
Imagine that data_url is 1 byte long, and you have a single read of length 1.
The first read will get a "<", and then the second read will get a "p".
using_populated_read will be false, and bytes_read will be 1.
> 2. Is the regression range just wrong do you think?
The regression range is probably because DCHECKs were presumably only recently
enabled on the fuzzers. I'm too lazy to look through CL descriptions, but be my
guest to see if you can find that CL.
lgtm thanks for fixing this.
On 2016/07/29 19:16:01, csharrison wrote: > lgtm thanks for fixing this. No problem.
The CQ bit was unchecked by mmenke@chromium.org
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== A couple fixes to net_data_job_fuzzer. In particular, the code was being run on non-data URLs, which caused it to do weird and exciting things. BUG=632605 ========== to ========== A couple fixes to net_data_job_fuzzer. In particular, the code was being run on non-data URLs, which caused it to do weird and exciting things, and it had an incorrect DCHECK. BUG=632605 ==========
Message was sent while issue was closed.
Description was changed from ========== A couple fixes to net_data_job_fuzzer. In particular, the code was being run on non-data URLs, which caused it to do weird and exciting things, and it had an incorrect DCHECK. BUG=632605 ========== to ========== A couple fixes to net_data_job_fuzzer. In particular, the code was being run on non-data URLs, which caused it to do weird and exciting things, and it had an incorrect DCHECK. BUG=632605 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== A couple fixes to net_data_job_fuzzer. In particular, the code was being run on non-data URLs, which caused it to do weird and exciting things, and it had an incorrect DCHECK. BUG=632605 ========== to ========== A couple fixes to net_data_job_fuzzer. In particular, the code was being run on non-data URLs, which caused it to do weird and exciting things, and it had an incorrect DCHECK. BUG=632605 Committed: https://crrev.com/4688e186bfb90fcba1725ff713e54af756cd00e9 Cr-Commit-Position: refs/heads/master@{#408737} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4688e186bfb90fcba1725ff713e54af756cd00e9 Cr-Commit-Position: refs/heads/master@{#408737} |
