`about:blank` is not mixed content.

`about:blank` is not mixed content.

The refactoring in [1] broke our handling of `about:blank` by moving from
`SecurityOrigin::isSecure(url)` to `SecurityOrigin::create(url)`. The
latter treats contextually-bound URLs like `about:blank` as unique security
origins, which I completely missed when writing tests.

This patch corrects the error, and adds a unit test that I should have
added a long time ago. :)

[1]: https://chromium.googlesource.com/chromium/src/+/130ee686fa00b617bfc001ceb3bb49782da2cb4e

BUG=624275
R=yoav@yoav.ws

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

[Mike West, 2016-07-04 08:14:59]

Yoav, have a moment to check this?

[Yoav Weiss, 2016-07-04 09:02:36]

LGTM + nagging, unrelated questions :)

Tracking down what isSecure() does, I saw a *lot* of locks around the
SchemeRegistry code. Could we get rid of some of that by initializing the
HashSets early (as part of init) and only read from them after that point? Seems
like these locks standing in the way of issuing requests could have a perf
impact.

https://codereview.chromium.org/2123463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (left):

https://codereview.chromium.org/2123463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:95: if (isAllowed
&& url.protocolIs("http") && url.host() == "localhost")
unrelated to current change, but IIUC "127.0.0.1" is considered potentially
trustworthy while localhost is not. Is that correct? If so, just out of
curiosity, why?

[Mike West, 2016-07-04 09:41:23]

On 2016/07/04 at 09:02:36, yoav wrote:
> LGTM + nagging, unrelated questions :)

Thanks!

> Tracking down what isSecure() does, I saw a *lot* of locks around the
SchemeRegistry code. Could we get rid of some of that by initializing the
HashSets early (as part of init) and only read from them after that point? Seems
like these locks standing in the way of issuing requests could have a perf
impact.

kinuko@'s the right person to talk to for the locks; she did a bunch of work
here around thread safety.

>
https://codereview.chromium.org/2123463002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (left):
> 
>
https://codereview.chromium.org/2123463002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:95: if
(isAllowed && url.protocolIs("http") && url.host() == "localhost")
> unrelated to current change, but IIUC "127.0.0.1" is considered potentially
trustworthy while localhost is not. Is that correct? If so, just out of
curiosity, why?

Because `*.localhost` can resolve via a network request, which is absurd but
true. Talk to rsleevi@ and estark@ for the gory details, or consult the thread
at https://lists.w3.org/Archives/Public/public-webappsec/2016May/0006.html.

[Mike West, 2016-07-04 09:41:25]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-07-04 09:41:36]

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-07-04 09:45:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-04 09:47:01]

Description was changed from

==========
`about:blank` is not mixed content.

The refactoring in [1] broke our handling of `about:blank` by moving from
`SecurityOrigin::isSecure(url)` to `SecurityOrigin::create(url)`. The
latter treats contextually-bound URLs like `about:blank` as unique security
origins, which I completely missed when writing tests.

This patch corrects the error, and adds a unit test that I should have
added a long time ago. :)

[1]:
https://chromium.googlesource.com/chromium/src/+/130ee686fa00b617bfc001ceb3bb...

BUG=624275
R=yoav@yoav.ws
==========

to

==========
`about:blank` is not mixed content.

The refactoring in [1] broke our handling of `about:blank` by moving from
`SecurityOrigin::isSecure(url)` to `SecurityOrigin::create(url)`. The
latter treats contextually-bound URLs like `about:blank` as unique security
origins, which I completely missed when writing tests.

This patch corrects the error, and adds a unit test that I should have
added a long time ago. :)

[1]:
https://chromium.googlesource.com/chromium/src/+/130ee686fa00b617bfc001ceb3bb...

BUG=624275
R=yoav@yoav.ws

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

[commit-bot: I haz the power, 2016-07-04 09:47:01]

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