libFuzzer for blink::MHTMLParser

libFuzzer for blink::MHTMLParser

For more information about libFuzzer, please see
https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/README.md

BUG=

Committed: https://crrev.com/c32cfbb16c41dc98fbf72894fba83916076d70ee
Cr-Commit-Position: refs/heads/master@{#412335}

[Łukasz Anforowicz, 2016-07-29 23:43:57]

lukasza@chromium.org changed reviewers:
+ mmoroz@chromium.org

[Łukasz Anforowicz, 2016-07-29 23:43:58]

Max, can you take a look from libFuzzer perspective?  (I'll get owners review
later)

Is this useful?  FWIW, it didn't find any real crashes yet (so far libFuzzer
only forced me to remove not quite correct asserts from the code).

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:666: seed_corpus =
"//third_party/WebKit/LayoutTests/mhtml"
Maybe this is a little bit wasteful, as in addition to *.mht files this
directory also includes *.html and *-expected.* files.

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (left):

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:102:
blink::HTTPNames::init();
Initialization of unit tests has been consolidated by this CL into
ScopedUnittestsEnvironmentSetup class.

[mmoroz, 2016-08-01 17:41:37]

I left some comments, LGTM on libfuzzer side.

I think it should be interesting, thanks for implementing that!

https://codereview.chromium.org/2199493002/diff/40001/testing/libfuzzer/fuzze...
File testing/libfuzzer/fuzzers/dicts/mhtml.dict (right):

https://codereview.chromium.org/2199493002/diff/40001/testing/libfuzzer/fuzze...
testing/libfuzzer/fuzzers/dicts/mhtml.dict:2: # AFL dictionary for MHTML
If you haven't copied this dictionary from the AFL sources, there is no need for
this line. I would recommend to put Chromium copyright as we do in other
dictionaries created by us (e.g.
https://cs.chromium.org/chromium/src/net/data/dns/dns.dict).

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:666: seed_corpus =
"//third_party/WebKit/LayoutTests/mhtml"
On 2016/07/29 23:43:58, Łukasz Anforowicz wrote:
> Maybe this is a little bit wasteful, as in addition to *.mht files this
> directory also includes *.html and *-expected.* files.

Yeah, but may be interesting. Let's start with this. Also we can always upload
new interesting testcases to the cloud bucket.

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:31: //
ASSERT(mhtmlArchive->url().isValid());
It would be better to avoid asserts in the target function. I mean, if it is
something what we may hit by passing invalid inputs (LibFuzzer obviously does
that), we don't want to hit asserts, return 0 is needed. If assert is for
something impossible (i.e. real problem which we actually do not expect to
happen), it may take place.

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:37: // This probably
needs to be uncommented to support -detect_leaks=1 flag.
Good point. If LSan detects that, we have to enable GC. Otherwise we will be
always crashing on leaks :(

[Łukasz Anforowicz, 2016-08-01 21:09:40]

Thanks for the review.

https://codereview.chromium.org/2199493002/diff/40001/testing/libfuzzer/fuzze...
File testing/libfuzzer/fuzzers/dicts/mhtml.dict (right):

https://codereview.chromium.org/2199493002/diff/40001/testing/libfuzzer/fuzze...
testing/libfuzzer/fuzzers/dicts/mhtml.dict:2: # AFL dictionary for MHTML
On 2016/08/01 17:41:37, mmoroz wrote:
> If you haven't copied this dictionary from the AFL sources, there is no need
for
> this line. I would recommend to put Chromium copyright as we do in other
> dictionaries created by us (e.g.
> https://cs.chromium.org/chromium/src/net/data/dns/dns.dict).

Done.

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:666: seed_corpus =
"//third_party/WebKit/LayoutTests/mhtml"
On 2016/08/01 17:41:37, mmoroz wrote:
> On 2016/07/29 23:43:58, Łukasz Anforowicz wrote:
> > Maybe this is a little bit wasteful, as in addition to *.mht files this
> > directory also includes *.html and *-expected.* files.
> 
> Yeah, but may be interesting. Let's start with this. Also we can always upload
> new interesting testcases to the cloud bucket.

Acknowledged.

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:31: //
ASSERT(mhtmlArchive->url().isValid());
On 2016/08/01 17:41:37, mmoroz wrote:
> It would be better to avoid asserts in the target function. I mean, if it is
> something what we may hit by passing invalid inputs (LibFuzzer obviously does
> that), we don't want to hit asserts, return 0 is needed. If assert is for
> something impossible (i.e. real problem which we actually do not expect to
> happen), it may take place.

I was wondering if for an invalid input, MHTMLParser should return an empty
vector (rather than returning a vector with invalid data inside).  I am not sure
if this is desirable - you're probably right that the current API contract is
sufficient.

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:37: // This probably
needs to be uncommented to support -detect_leaks=1 flag.
On 2016/08/01 17:41:37, mmoroz wrote:
> Good point. If LSan detects that, we have to enable GC. Otherwise we will be
> always crashing on leaks :(

Ok - done.

Also - there is no perf regression even with GC.  I guess there were some
external variables that influenced my initial libfuzzer runs (right now I
consistently get 800+ exec/s with and without the collectAllGarbage call).

https://codereview.chromium.org/2199493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:58: new
blink::ScopedUnittestsEnvironmentSetup();
Ooops.  This results in a leak report by libfuzzer.  I need to change this to a
static variable instead.

[Łukasz Anforowicz, 2016-08-02 00:11:18]

Patchset #5 (id:80001) has been deleted

[Łukasz Anforowicz, 2016-08-02 00:11:27]

Patchset #5 (id:100001) has been deleted

[Łukasz Anforowicz, 2016-08-02 00:13:32]

lukasza@chromium.org changed reviewers:
+ tkent@chromium.org

[Łukasz Anforowicz, 2016-08-02 00:13:33]

tkent@, could you please take a look?

[tkent, 2016-08-02 23:00:45]

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:13: #include
<base/command_line.h>
Do not use |#include <>| for chromium headers.

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:41:
#include <base/memory/discardable_memory_allocator.h>
I know you just move the code, but we should not add new |#include <>| for
Chromium headers.

[Łukasz Anforowicz, 2016-08-03 16:02:46]

tkent@ - thanks for the review, can you take another look please (at the diff in
the 2 most recent patchsets)?

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:13: #include
<base/command_line.h>
On 2016/08/02 23:00:45, tkent wrote:
> Do not use |#include <>| for chromium headers.

Done.

This change is also making the includes be checked against DEPS rules.  So - in
the follow-up patchset I am folding base/command_line.h and base/i18n/icu_util.h
initialization into blink::ScopedUnittestsEnvironmentSetup (which by virtue of
being under platform/*testing* can include some extra base headers).

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2199493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:41:
#include <base/memory/discardable_memory_allocator.h>
On 2016/08/02 23:00:45, tkent wrote:
> I know you just move the code, but we should not add new |#include <>| for
> Chromium headers.

Thanks for pointing this out - I assumed that the style I am copying is the
official Blink style and didn't check :-(

Done.

[tkent, 2016-08-03 23:21:09]

tkent@chromium.org changed reviewers:
+ esprehn@chromium.org

[tkent, 2016-08-03 23:21:10]

Many blink unit tests are failing.  Please resolve it.

+esprehn for DEPS change.

[esprehn, 2016-08-03 23:24:19]

This needs a better change description, what bot runs these? It's not clear this
is a win for the MHTML parser instead of just teaching clusterfuzz to do it.

[Łukasz Anforowicz, 2016-08-04 03:50:23]

Description was changed from

==========
libFuzzer for blink::MHTMLParser

BUG=
==========

to

==========
libFuzzer for blink::MHTMLParser

For more information about libFuzzer, please see
https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/REA...

BUG=
==========

[Łukasz Anforowicz, 2016-08-04 04:07:17]

mmoroz@, could you please help with generic libfuzzer questions from esprehn@?

On 2016/08/03 23:21:10, tkent wrote:
> Many blink unit tests are failing.  Please resolve it.

Ooops.  I should have waited with replying until the tryjobs are done.  My
excuse is that the tests passed (and pass)  locally for some reason... :-/

To fix the tryjob failures I made the i18n initialization optional (to avoid
initializing it twice in case of RunAllTests.cpp).  The fix is in the latest
patchset.

On 2016/08/03 23:24:19, esprehn wrote:
> This needs a better change description, what bot runs these?

I am just a random dev who heard libfuzzer talk and wanted to try developing a
fuzzer for the functionality I've recently touched.  I tried to answer your
questions below, but libfuzzer's authors/owners (mmoroz@ from
testing/libfuzzer/OWNERS has already looked at this CL) can probably answer them
better than me.

AFAIK libFuzzer targets should be automatically picked up by clusterfuzz.

Please see
https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/REA...
for more information about libFuzzer.  FWIW, I tried to add to improve the CL
description by mentioning this link.  Does that look better?  Do you have
specific suggestions on how to improve the CL description?

> It's not clear this
> is a win for the MHTML parser instead of just teaching clusterfuzz to do it.

Well, doing fuzzing at the unit tests level is faster, so in theory libfuzzer
can discover useful, code-coverage-increasing inputs faster than the older
approaches.  OTOH, this approach only tests code reachable via MHTMLParser (and
for example doesn't actually try to do anything with the results returned by
MHTMLParser) so this has indeed less coverage than feeding fuzzed MHTML files to
the whole chrome browser.

I am not sure if there is official guidance for how to do fuzzing in Chromium. 
FWIW, it was the subject of Chrome tech talk last week ("Chrome Tech Talks:
Beyond Unit Testing: Scalable coverage guided fuzzing of Chrome components,
Friday, July 29, 10AM MTV time") and AFAIR there was a mini-fixit around it a
few month ago (which I missed + I can't find a link about).  I've found the
following snippet in chrome security Q4 2015 status email sent out to
chromium-dev: "We’ve integrated libFuzzer into ClusterFuzz, which means we can
do coverage-guided fuzz testing on a per-function basis. The result, as you may
have guessed, is several new bugs. The Bugs-- team has a larger goal this year
to help Chromium developers write a ClusterFuzz fuzzer alongside every unittest,
and libFuzzer integration is an important step toward achieving that goal."

Does that answer your question?

[esprehn, 2016-08-04 04:37:53]

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:23:
mhtmlArchives.clear();
why manually clear on stack vectors?

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:24:
ThreadHeap::collectAllGarbage();
Why do you need to do manual Oilpan GC's?

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLParser.cpp (right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLParser.cpp:112: DVLOG(1) << "Key
duplicate found in MIME header. Key is '" << key << "', previous value
replaced.";
Why did you need to change these?

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:255: if
(testType == TestType::LibFuzzer)
This doesn't feel right, why is the fuzzer running differently than unit tests?

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:124:
LibFuzzer,
I'd rather not do this, what code are you trying to share here? Why do we need a
separate setup?

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:131: class
Impl;
Why do you need the nested class? Maybe you want DEFINE_STATIC_LOCAL for your
static to avoid the destructors?

[Łukasz Anforowicz, 2016-08-04 17:37:30]

esprehn@, could you take another look please?

PS. Sorry for sending this out without waiting for tryjob results to come back -
my team is heading for an offsite in a few minutes and I wanted to reply to your
feedback before leaving.  I'll be back on Monday (August the 8th).

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:23:
mhtmlArchives.clear();
On 2016/08/04 04:37:53, esprehn wrote:
> why manually clear on stack vectors?

So that the garbage collection forced on the next line can reclaim memory that
has been allocated inside parseArchive method (i.e. the mhtmlArchives vector
should be the only "root" for that memory).

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:24:
ThreadHeap::collectAllGarbage();
On 2016/08/04 04:37:53, esprehn wrote:
> Why do you need to do manual Oilpan GC's?

Because otherwise libFuzzer will report a memory leak (it checks for memory
leaks after each LLVMFuzzerTestOneInput invocation AFAICT - without this
explicit garbage collection it would keep reporting memory leaks).

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLParser.cpp (right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLParser.cpp:112: DVLOG(1) << "Key
duplicate found in MIME header. Key is '" << key << "', previous value
replaced.";
On 2016/08/04 04:37:53, esprehn wrote:
> Why did you need to change these?

To make debugging of libFuzzer functionality easier - without this change, the
output from libFuzzer is drowned in the output from the DLOG(ERROR) statements.

I think this is an okay change to make:

1) these were debug-only logging statements, so the change has no impact on the
product

2) dev impact is minimal - it is easy to selectively turn these logging
statements back on.  hmmm... I thought that something like
--vmodule=MHTMLFuzzer=1 would work, but it doesn't (I've never used this for
Blink side of things - not sure how logging infrastructure extracts the name of
a module in case of blink...).

WDYT?  I can turn these back into DLOG(ERROR)-s if you feel strongly about it.

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:255: if
(testType == TestType::LibFuzzer)
On 2016/08/04 04:37:53, esprehn wrote:
> This doesn't feel right, why is the fuzzer running differently than unit
tests?

Good catch / call.

It turns out that test code should call
base::i18n::AllowMultipleInitializeCallsForTesting (there are already a few
calls to this function in the code base).

The problem is that 1) base/test/test_suite.cc doesn't call this and 2)
base/test/test_suite.cc makes icu initialization conditional on a command line
switch.  To solve this, I've added InitializeICUForTesting which 1) checks the
cmdline switch + calls AllowMultipleInitializeCallsForTesting and 2) can be
reused from both base/test/test_suite.cc and from
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp

Done?  (pending review of base/test OWNERS)

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:124:
LibFuzzer,
On 2016/08/04 04:37:53, esprehn wrote:
> I'd rather not do this, what code are you trying to share here?

I am trying to 1) consolidate all one-time test setup into
ScopedUnittestsEnvironmentSetup, 2) avoid i18n/ICU initialization from
MHTMLFuzzer.cpp (i.e. I want to limit base/i18n dependency to platform/testing
directory).

> Why do we need a separate setup?

We don't in the latest patchset - separate setup has been avoided by
introduction of InitializeICUForTesting in base/test/icu_test_util.h (and
described in a reply to your other feedback).

https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:131: class
Impl;
On 2016/08/04 04:37:53, esprehn wrote:
> Why do you need the nested class?

I wanted to avoid including base/ headers from TestingPlatformSupport.h (i.e.
limit including base/headers to TestingPlatformSupport.cpp).  This was before I
realized that most of the base classes will be wrapped in std::unique_ptr (and
therefore can be forward declared).  Still - there are some base objects
embedded in the Impl class - I think it is desirable to keep them there.

WDYT?

> Maybe you want DEFINE_STATIC_LOCAL for your
> static to avoid the destructors?

Hmmm... maybe.  Without the destructors (i.e. when used as a *static* variable),
RunAllTests.cpp behavior would change (i.e. no cleanup from ~Impl would happen).

[Łukasz Anforowicz, 2016-08-05 23:34:05]

It seems that the last patchset hit compile issues on linux trybots - this CL
might not yet be ready for a re-review.  I'll try to fix this on Monday and let
you know when it works.

[mmoroz, 2016-08-09 02:24:01]

Regarding #15, sorry for the delay - I was attending a security conference.
Lukasz, I think you've got everything right about libFuzzer/ClusterFuzz, I don't
have anything to add to your answers.

Looking forward to get it running at ClusterFuzz!

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:13: #include <stddef.h>
nit: It would be better to include standard headers first
(https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes).

[Łukasz Anforowicz, 2016-08-09 21:28:33]

tkent@ and esprehn@ - this CL should now be ready for a rereview please: 1) the
bots are green and 2) I think I've addressed your feedback:

- Reply to esprehn@'s feedback is at
https://codereview.chromium.org/2199493002/#msg17 and AFAIK the last patchset
you've commented on is #7

- Reply to tkent@'s feedback is at
https://codereview.chromium.org/2199493002/#msg10 and AFAIK the last patchset
you've commented on is #4 (I've also addressed bot redness you've pointed out in
patchset #6)

mmoroz@ - thanks for the feedback - I replied below.

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:13: #include <stddef.h>
On 2016/08/09 02:24:01, mmoroz wrote:
> nit: It would be better to include standard headers first
>
(https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes).

Unfortunately, Chromium and Blink style guides disagree here.  AFAIK there is a
plan to reformat Blink according to Chromium style, but this effort is blocked
on some tooling work.  Until the style guides are unified, I have to follow
Blink style which says:

https://www.chromium.org/blink/coding-style#TOC-include-Statements:

Includes of system headers must come after includes of other headers. This
happens to also be checked and ensured by alphabetical order, as < comes after
".

[tkent, 2016-08-10 01:32:16]

third_party/WebKit lgtm.  Please get esprehn's lgtm too.

[esprehn, 2016-08-10 04:05:29]

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:40: static
blink::ScopedUnittestsEnvironmentSetup testSetup(*argc, *argv);
DEFINE_STATIC_LOCAL(...)

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:243:
cc_blink::WebCompositorSupportImpl m_compositorSupport;
Just put all of this in the header, there's no need to hide it in the Impl class

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:291:
m_impl.reset();
manually clearing a unique_ptr in the destructor is very weird, why is this
needed?

[mmoroz, 2016-08-10 09:12:56]

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:13: #include <stddef.h>
On 2016/08/09 21:28:33, Łukasz Anforowicz wrote:
> On 2016/08/09 02:24:01, mmoroz wrote:
> > nit: It would be better to include standard headers first
> >
>
(https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes).
> 
> Unfortunately, Chromium and Blink style guides disagree here.  AFAIK there is
a
> plan to reformat Blink according to Chromium style, but this effort is blocked
> on some tooling work.  Until the style guides are unified, I have to follow
> Blink style which says:
> 
> https://www.chromium.org/blink/coding-style#TOC-include-Statements:
> 
> Includes of system headers must come after includes of other headers. This
> happens to also be checked and ensured by alphabetical order, as < comes after
> ".

Makes sense, sorry for incorrect suggestion from me!

[Łukasz Anforowicz, 2016-08-10 19:31:18]

esprehn@, can you take another look please?

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp (right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mhtml/MHTMLFuzzer.cpp:40: static
blink::ScopedUnittestsEnvironmentSetup testSetup(*argc, *argv);
On 2016/08/10 04:05:29, esprehn wrote:
> DEFINE_STATIC_LOCAL(...)

Done.  (it also requires ALLOW_UNUSED_LOCAL(...))

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:243:
cc_blink::WebCompositorSupportImpl m_compositorSupport;
On 2016/08/10 04:05:29, esprehn wrote:
> Just put all of this in the header, there's no need to hide it in the Impl
class

Done.

I need to note that I've also wrapped more things in std::unique_ptr:

- I think I should have done that before - to preserve the relative order of 1)
base::CommandLine::Init call and 2) construction of
base::TestDiscardableMemoryAllocator / cc_blink::WebCompositorSupportImpl / etc.

- Wrapping in std::unique_ptr allows to forward declare specific base and cc
classes (from TestingPlatformSupport.h) instead of including full base / cc
headers

https://codereview.chromium.org/2199493002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:291:
m_impl.reset();
On 2016/08/10 04:05:29, esprehn wrote:
> manually clearing a unique_ptr in the destructor is very weird, why is this
> needed?

You're right - this needs at least an explicit comment.  I am explicitly calling
reset to preserve the ordering of destruction from the code in RunAllTests.cpp
that is being replaced by ScopedUnittestsEnvironmentSetup class.

I've added a comment here to point out that we want to tear down testing
platform before shutting down WTF layer.  Does this help?

[esprehn, 2016-08-11 09:36:19]

lgtm

[Łukasz Anforowicz, 2016-08-11 14:08:23]

lukasza@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Łukasz Anforowicz, 2016-08-11 14:08:24]

phajdan.jr@, could you please take a look at the changes under base/test?

The new MHTMLFuzzer.cpp needs to initialize ICU via base::i18n::InitializeICU()
call, but needs to do so in a way compatible with (1) the goal of sharing test
initializtion code with
third_party/WebKit/Source/platform/testing/RunAllTests.cpp via the newly
introduced class ScopedUnittestsEnvironmentSetup and (2) make sure that
ScopedUnittestsEnvironmentSetup is compatible with a later call to
base::TestSuite::Initialize made by .../platform/testing/RunAllTests.cpp.

I agree (*) with esprehn@ wrt making ScopedUnittestsEnvironmentSetup behave the
same for all consumers of ScopedUnittestsEnvironmentSetup.  This means having to
make base::TestSuite::Initialize resilient to being called when ICU has already
been initialized, by first calling AllowMultipleInitializeCallsForTesting (there
are already a few places in the test code that do that).

To further facilitate code reuse, I am pulling the modified
base::TestSuite::Initialize into a separate base::test::InitializeICUForTesting
function in base/test/icu_test_util.h.  The new helper function is then used
both from base::TestSuite::Initialize and from MHTMLFuzzer.cpp.  Doing this
requires some extra shuffling of BUILD.gn files to expose
base/test/icu_test_util.h via base/test:test_support.

WDYT? :-)

(*)
https://codereview.chromium.org/2199493002/diff/160001/third_party/WebKit/Sou...

[Charlie Harrison, 2016-08-15 19:12:11]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2016-08-15 19:12:11]

FYI I am also interested using the test infrastructure added in this CL to use
for more fuzzers in Blink. Thanks for tackling this :)

[Paweł Hajdan Jr., 2016-08-16 09:04:08]

base/test LGTM

[Łukasz Anforowicz, 2016-08-16 14:39:40]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2016-08-16 14:39:40]

dcheng@, could you please stamp the changes in base/BUILD.gn (I am just moving
icu_test_util.h/.cc from base:base_unittests target into base/test:test_support
target and that move has already been reviewed and signed-off by phajdan.jr@).

[Łukasz Anforowicz, 2016-08-16 16:49:50]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 16:50:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 20:24:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 20:24:45]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-08-16 21:00:40]

rs lgtm for //base changes

[Łukasz Anforowicz, 2016-08-16 21:03:22]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-08-16 21:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2199493002/#ps240001
(title: "Addressed CR feedback from esprehn@.")

[commit-bot: I haz the power, 2016-08-16 21:03:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 21:08:56]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-08-16 21:11:50]

Description was changed from

==========
libFuzzer for blink::MHTMLParser

For more information about libFuzzer, please see
https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/REA...

BUG=
==========

to

==========
libFuzzer for blink::MHTMLParser

For more information about libFuzzer, please see
https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/REA...

BUG=

Committed: https://crrev.com/c32cfbb16c41dc98fbf72894fba83916076d70ee
Cr-Commit-Position: refs/heads/master@{#412335}
==========

[commit-bot: I haz the power, 2016-08-16 21:11:52]

Patchset 11 (id:??) landed as
https://crrev.com/c32cfbb16c41dc98fbf72894fba83916076d70ee
Cr-Commit-Position: refs/heads/master@{#412335}