Validate generated MHTML in mhtml_generation_browsertest.cc tests.

Validate generated MHTML in mhtml_generation_browsertest.cc tests.

Attempts to load the generated MHTML file after creating it for all tests in
mhtml_generation_browsertest.cc. This allows for validation of its contents
being well formed by verifying that the newly added console message is not
reported.

Also simplifies the checking for the MHTML generation succeeding by asserting
from inside the generation call instead of having checks for test failure in
multiple tests.

BUG=672313
Review-Url: https://codereview.chromium.org/2866873002
Cr-Commit-Position: refs/heads/master@{#472939}
Committed: https://chromium.googlesource.com/chromium/src/+/656b9cae0b536ee2f9b8a2777f76ba4a4d775173

[carlosk, 2017-05-05 23:39:34]

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

[commit-bot: I haz the power, 2017-05-05 23:40:09]

Dry run: CQ is trying da patch.

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

[carlosk, 2017-05-05 23:46:26]

carlosk@chromium.org changed reviewers:
+ lukasza@chromium.org, nasko@chromium.org

[carlosk, 2017-05-05 23:46:27]

lukasza@, nasko@: PTAL.

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:444: CommitData(0, 0);
IIUC the local frame is only initialized when data is first committed. As for
empty documents (precisely the case for invalid MHTML) this only happens right
above I chose to have the console message reported here in FinishedLoading
instead of MaybeCreateArchive where I initially thought I would add it.

[Łukasz Anforowicz, 2017-05-06 00:14:49]

Thanks for tackling this - I think this (verifying in browser tests by looking
out for the console error message) is indeed the best possible option.

Could you please add a test for presence of the new console message?  I think
you could just add a test under third_party/WebKit/LayoutTests/mhtml (I can help
with overall layout test question if needed :-).

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:444: CommitData(0, 0);
On 2017/05/05 23:46:27, carlosk wrote:
> IIUC the local frame is only initialized when data is first committed. As for
> empty documents (precisely the case for invalid MHTML) this only happens right
> above I chose to have the console message reported here in FinishedLoading
> instead of MaybeCreateArchive where I initially thought I would add it.

Thank you for the comment above.  Unfortunately, I still fail to understand why
the console error is reported here and not in MaybeCreateArchive (in the
|!main_resource| case).  It seems wrong (and I think the current CL does this)
to report the console error when there is a writer (i.e. we have a non-empty
document?) which I think (?) would happen when returning false from
MaybeCreateArchive in |!frame_| case.

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:450: "Malformed
multipart archive: " + response_.Url().ElidedString()));
I see that KURL::ElidedString returns an empty string if the url is shorter than
1024.  I would not have expected (maybe KURL::ElidedString is wrong?), but in
any case, I think the developers would prefer to see a full URL in an error
message.

[Łukasz Anforowicz, 2017-05-06 00:42:47]

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:450: "Malformed
multipart archive: " + response_.Url().ElidedString()));
On 2017/05/06 00:14:48, Łukasz A. wrote:
> I see that KURL::ElidedString returns an empty string if the url is shorter
than
> 1024.

Ooops.  I think I misread the code - what I said above is incorrect.

> I would not have expected (maybe KURL::ElidedString is wrong?), but in
> any case, I think the developers would prefer to see a full URL in an error
> message.

Still - I wanted to check - do you think it is better to report elided string VS
full string?

[commit-bot: I haz the power, 2017-05-06 01:03:41]

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

[commit-bot: I haz the power, 2017-05-06 01:03:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nasko, 2017-05-08 17:16:58]

I'd defer this review to lukasza@ and am happy to stamp it once he is happy with
it.

https://codereview.chromium.org/2866873002/diff/1/content/browser/download/mh...
File content/browser/download/mhtml_generation_browsertest.cc (right):

https://codereview.chromium.org/2866873002/diff/1/content/browser/download/mh...
content/browser/download/mhtml_generation_browsertest.cc:137:
NavigateToURL(shell(), net::FilePathToFileURL(params.file_path));
EXPECT_TRUE, otherwise failure to load, such as file not found, will not be
detected.

[carlosk, 2017-05-16 20:41:11]

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

[commit-bot: I haz the power, 2017-05-16 20:42:56]

Dry run: CQ is trying da patch.

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

[carlosk, 2017-05-16 20:56:08]

Thanks.

The mhtml_in_iframe.html layout test showed an inconsistency with the solution:
the log was reporting a policy/security issue -- no MHTML allowed in iframes --
as a malformed MHTML. That check is enforced in ResourceFetcher::CreateArchive
and returns the same null value we were interpreting in
DocumentLoader::MaybeCreateArchive as "invalid MHTML".

One option is to move the iframe check one level up, into
DocumentLoader::MaybeCreateArchive. But reading ResourceFetcher's class doc it
seems it is supposed to be responsible for security checks, what seems to be the
case here.

Another, is to move the console log into ResourceFetcher::CreateArchive. There
is already another occurrence of console logging from
ResourceFetcher::WarnUnusedPreloads. I went with this one as it seemed a safer
bet. And as I was already changing that code I also included another log error
for the policy error, now verified by the mhtml_in_iframe.html test.

https://codereview.chromium.org/2866873002/diff/1/content/browser/download/mh...
File content/browser/download/mhtml_generation_browsertest.cc (right):

https://codereview.chromium.org/2866873002/diff/1/content/browser/download/mh...
content/browser/download/mhtml_generation_browsertest.cc:137:
NavigateToURL(shell(), net::FilePathToFileURL(params.file_path));
On 2017/05/08 17:16:58, nasko (out) wrote:
> EXPECT_TRUE, otherwise failure to load, such as file not found, will not be
> detected.

Done.

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:444: CommitData(0, 0);
On 2017/05/06 00:14:48, Łukasz A. wrote:
> On 2017/05/05 23:46:27, carlosk wrote:
> > IIUC the local frame is only initialized when data is first committed. As
for
> > empty documents (precisely the case for invalid MHTML) this only happens
right
> > above I chose to have the console message reported here in FinishedLoading
> > instead of MaybeCreateArchive where I initially thought I would add it.
> 
> Thank you for the comment above.  Unfortunately, I still fail to understand
why
> the console error is reported here and not in MaybeCreateArchive (in the
> |!main_resource| case).  It seems wrong (and I think the current CL does this)
> to report the console error when there is a writer (i.e. we have a non-empty
> document?) which I think (?) would happen when returning false from
> MaybeCreateArchive in |!frame_| case.

I was confused by all the checks for |frame_| not being null and I need it to be
able to log. I assumed the CommitData call above would take care of the writer
and the frame. Upon further looking into the code I now see the frame is
supposed to be passed in upon initialization.

Note that the final outcome in case of an invalid MHTML is an empty document so
that CommitData and further |writer_| creation will happen anyway.

But anyway, as explained before, this all moved to
ResourceFetcher::CreateArchive.

https://codereview.chromium.org/2866873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:450: "Malformed
multipart archive: " + response_.Url().ElidedString()));
On 2017/05/06 00:42:47, Łukasz A. wrote:
> On 2017/05/06 00:14:48, Łukasz A. wrote:
> > I see that KURL::ElidedString returns an empty string if the url is shorter
> than
> > 1024.
> 
> Ooops.  I think I misread the code - what I said above is incorrect.
> 
> > I would not have expected (maybe KURL::ElidedString is wrong?), but in
> > any case, I think the developers would prefer to see a full URL in an error
> > message.
> 
> Still - I wanted to check - do you think it is better to report elided string
VS
> full string?

This is me copy/pasting without fully understanding what I'm doing. The total
lack of documentation on KURL doesn't help either. :/

You are right, it's detrimental to the developer to not see the full URL.
Changed to GetString().

[carlosk, 2017-05-16 21:52:25]

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

[commit-bot: I haz the power, 2017-05-16 21:53:25]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-16 22:37:00]

lgtm - thanks!

[carlosk, 2017-05-16 22:50:25]

carlosk@chromium.org changed reviewers:
+ japhet@chromium.org

[carlosk, 2017-05-16 22:50:26]

japhet@: PTAL at ResourceFetcher.

[Nate Chapin, 2017-05-16 22:55:33]

I take it there's no .html file from which the .mhtml archive was generated
(since it's intentionally malformed)? If so, lgtm.

Optional: have the test start at a valid html page that calls
testRunner.dumpAsText(), then redirects to the malformed mhtml archive, so that
you don't have the spurious .png expectation.

https://codereview.chromium.org/2866873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2866873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1162:
"Attempt to load a multipart archive into an subframe: " +
Attempt->Attempted ?

[carlosk, 2017-05-17 00:42:51]

Thanks.

As I'm unable to call any Javascript from the final MHTML page I was unable to
create a HTML based test. So I'm keeping the PNG cruft for simplicity.

https://codereview.chromium.org/2866873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2866873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1162:
"Attempt to load a multipart archive into an subframe: " +
On 2017/05/16 22:55:33, Nate Chapin wrote:
> Attempt->Attempted ?

Done.

[carlosk, 2017-05-17 00:55:30]

carlosk@chromium.org changed reviewers:
+ dtrainor@chromium.org

[carlosk, 2017-05-17 00:55:31]

dtrainor@: PTAL at mhtml_generation_browsertest.cc.

Adding dtrainor@ for owner's review as nasko@ is out.

[David Trainor- moved to gerrit, 2017-05-17 17:32:43]

mhtml_generation_browsertest.cc lgtm

[carlosk, 2017-05-17 18:20:15]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-05-17 18:20:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2866873002/#ps80001
(title: "Textual changes.")

[commit-bot: I haz the power, 2017-05-17 18:21:26]

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, 2017-05-17 20:35:09]

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

[commit-bot: I haz the power, 2017-05-17 20:35:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[carlosk, 2017-05-17 21:43:49]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-05-17 21:43:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, dtrainor@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2866873002/#ps100001
(title: "Fixed layout test expectation.")

[commit-bot: I haz the power, 2017-05-17 21:45:43]

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, 2017-05-17 23:59:10]

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

[commit-bot: I haz the power, 2017-05-17 23:59:10]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[carlosk, 2017-05-18 20:25:00]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2017-05-18 20:26:00]

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, 2017-05-18 21:46:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495139100641490,
"parent_rev": "c85a0ce422acb66d6cc3c2824b5a01f9b36d0fcf", "commit_rev":
"656b9cae0b536ee2f9b8a2777f76ba4a4d775173"}

[commit-bot: I haz the power, 2017-05-18 21:46:14]

Description was changed from

==========
Validate generated MHTML in mhtml_generation_browsertest.cc tests.

Attempts to load the generated MHTML file after creating it for all tests in
mhtml_generation_browsertest.cc. This allows for validation of its contents
being well formed by verifying that the newly added console message is not
reported.

Also simplifies the checking for the MHTML generation succeeding by asserting
from inside the generation call instead of having checks for test failure in
multiple tests.

BUG=672313
==========

to

==========
Validate generated MHTML in mhtml_generation_browsertest.cc tests.

Attempts to load the generated MHTML file after creating it for all tests in
mhtml_generation_browsertest.cc. This allows for validation of its contents
being well formed by verifying that the newly added console message is not
reported.

Also simplifies the checking for the MHTML generation succeeding by asserting
from inside the generation call instead of having checks for test failure in
multiple tests.

BUG=672313

Review-Url: https://codereview.chromium.org/2866873002
Cr-Commit-Position: refs/heads/master@{#472939}
Committed:
https://chromium.googlesource.com/chromium/src/+/656b9cae0b536ee2f9b8a2777f76...
==========

[commit-bot: I haz the power, 2017-05-18 21:46:28]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/656b9cae0b536ee2f9b8a2777f76...