Allow loading MHTML archive from content scheme

Allow loading MHTML archive from content scheme

BUG=630753
Committed: https://crrev.com/77ce66b313e37fd0bba77d3f85769e593c10a353
Cr-Commit-Position: refs/heads/master@{#415775}

[Vivian, 2016-08-15 20:55:14]

weiran@google.com changed reviewers:
+ jianli@chromium.org

[Vivian, 2016-08-15 20:55:15]

weiran@google.com changed reviewers:
+ jianli@chromium.org

[Vivian, 2016-08-15 20:55:16]

Hi Jian,

This patch uses the same appraoch with your earlier CL, allowing content scheme
in MHTML by adding a check. ptal

Thanks.

[Vivian, 2016-08-15 20:55:17]

Hi Jian,

This patch uses the same appraoch with your earlier CL, allowing content scheme
in MHTML by adding a check. ptal

Thanks.

[jianli, 2016-08-15 23:12:02]

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

[jianli, 2016-08-15 23:12:02]

lukasza, please also approve this change for security perspective.

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:73: // The latter is
now allowed due to full sandboxing enforcement on MHTML pages.
Please also update comment.

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:74: if
(!SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()) &&
!url.protocolIsInHTTPFamily() && !url.protocolIsContent())
The check for content protocol should only be for Android platform.

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/weborigin/KURL.h (right):

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/weborigin/KURL.h:140: bool
protocolIsContent() const { return protocolIs("content"); }
Probably no need to add a general check here since it is only used in one place.

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

On 2016/08/15 23:12:02, jianli wrote:
> lukasza, please also approve this change for security perspective.
> 
Allowing rendering of MHTML delivered via Android's content: protocol LGTM (for
the same reasons why we recently allowed rendering of MHTML delivered via http:
and https: - see https://codereview.chromium.org/2234543002/).

[Vivian, 2016-08-16 21:00:44]

Comments addressed. ptal

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:73: // The latter is
now allowed due to full sandboxing enforcement on MHTML pages.
On 2016/08/15 23:12:02, jianli wrote:
> Please also update comment.

Done.

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:74: if
(!SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()) &&
!url.protocolIsInHTTPFamily() && !url.protocolIsContent())
On 2016/08/15 23:12:02, jianli wrote:
> The check for content protocol should only be for Android platform.

Done.

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/weborigin/KURL.h (right):

https://codereview.chromium.org/2247063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/weborigin/KURL.h:140: bool
protocolIsContent() const { return protocolIs("content"); }
On 2016/08/15 23:12:02, jianli wrote:
> Probably no need to add a general check here since it is only used in one
place.

Done.

[jianli, 2016-08-17 01:12:49]

Also, don't forget to update the test MHTMLTest.cpp.

https://codereview.chromium.org/2247063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/2247063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:75: #if (DE_OS ==
DE_OS_ANDROID)
I don't think this is a correct check. You should use:
#if OS(ANDROID)

Also, we should try to avoid span this check in multi-line like this. I suggest
to introduce a helper function like CanLoad to improve readability.

[Vivian, 2016-08-17 17:24:45]

The CQ bit was checked by weiran@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 17:25:15]

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

[Vivian, 2016-08-17 17:27:18]

Comment addressed. For test, I only added several lines in MHTMLFromScheme. Let
me know if a test like EnforceSandboxFlags is also needed.

https://codereview.chromium.org/2247063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/2247063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:75: #if (DE_OS ==
DE_OS_ANDROID)
On 2016/08/17 01:12:49, jianli wrote:
> I don't think this is a correct check. You should use:
> #if OS(ANDROID)
> 
> Also, we should try to avoid span this check in multi-line like this. I
suggest
> to introduce a helper function like CanLoad to improve readability.

Done.

[commit-bot: I haz the power, 2016-08-17 20:10:27]

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

[commit-bot: I haz the power, 2016-08-17 20:10:28]

Dry run: This issue passed the CQ dry run.

[jianli, 2016-08-17 20:27:21]

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

https://codereview.chromium.org/2247063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:74: if
(!SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()) &&
!url.protocolIsInHTTPFamily() && !canLoadContent(url))
I think it would be better to move all these checks into a helper function,
like:
bool MHTMLArchive::canLoadArchive(onst KURL& url)
{
    // MHTML pages can only be loaded ...
    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol())
        return true;
    if (url.protocolIsInHTTPFamily())
        return true;
#if OS(ANDROID)
    if (url.protocolIs("content"))
        return true;
#endif
    return false;
}

[Vivian, 2016-08-17 20:38:17]

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

https://codereview.chromium.org/2247063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:74: if
(!SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()) &&
!url.protocolIsInHTTPFamily() && !canLoadContent(url))
On 2016/08/17 20:27:21, jianli wrote:
> I think it would be better to move all these checks into a helper function,
> like:
> bool MHTMLArchive::canLoadArchive(onst KURL& url)
> {
>     // MHTML pages can only be loaded ...
>     if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol())
>         return true;
>     if (url.protocolIsInHTTPFamily())
>         return true;
> #if OS(ANDROID)
>     if (url.protocolIs("content"))
>         return true;
> #endif
>     return false;
> }

Done.

[Vivian, 2016-08-19 17:19:43]

The CQ bit was checked by weiran@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 17:20:04]

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

[jianli, 2016-08-19 19:38:07]

You should also ask platform owner, like esprehn@chromium.org , to approve this
CL.

[jianli, 2016-08-19 19:38:15]

lgtm

[Vivian, 2016-08-19 19:39:33]

weiran@google.com changed reviewers:
+ esprehn@chromium.org

[Vivian, 2016-08-19 19:39:34]

esprehn@chromium.org: Ptal

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

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

[commit-bot: I haz the power, 2016-08-19 21:19:51]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-08-30 23:34:23]

lgtm

[jianli, 2016-08-31 20:16:08]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-08-31 20:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/2247063002/#ps60001
(title: "Move checks into a helper function")

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

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-31 22:10:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-31 22:13:31]

Description was changed from

==========
Allow loading MHTML archive from content scheme

BUG=630753
==========

to

==========
Allow loading MHTML archive from content scheme

BUG=630753

Committed: https://crrev.com/77ce66b313e37fd0bba77d3f85769e593c10a353
Cr-Commit-Position: refs/heads/master@{#415775}
==========

[commit-bot: I haz the power, 2016-08-31 22:13:32]

Patchset 4 (id:??) landed as
https://crrev.com/77ce66b313e37fd0bba77d3f85769e593c10a353
Cr-Commit-Position: refs/heads/master@{#415775}