Take care of a FIXME in SecurityOrigin.cpp to check the validity of any innerURL during constructio…

Take care of a FIXME in SecurityOrigin.cpp to check the validity of any innerURL during construction and to handle invalidity as a 'unique' origin.

TBR=mkwst
BUG=514076
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201022

[michaeln, 2015-08-15 00:43:06]

michaeln@chromium.org changed reviewers:
+ jsbell@chromium.org

[michaeln, 2015-08-15 00:43:07]

ptal

[jsbell, 2015-08-17 16:30:28]

lgtm, palmer@ should probably take a look too?

[michaeln, 2015-08-17 19:01:12]

michaeln@chromium.org changed reviewers:
+ palmer@chromium.org

[michaeln, 2015-08-17 19:01:12]

@palmer, ptal

[palmer, 2015-08-17 19:07:33]

palmer@chromium.org changed reviewers:
+ mkwst@chromium.org

[palmer, 2015-08-17 19:07:33]

LGTM, but mkwst is also a good origin maven to consult.

[Mike West, 2015-08-17 19:42:41]

On 2015/08/17 at 19:07:33, palmer wrote:
> LGTM, but mkwst is also a good origin maven to consult.

This looks correct to me, but I'd suggest adding tests to SecurityOriginTest.cpp
to verify that it's doing what we expect.

[michaeln, 2015-08-17 23:59:01]

ptal, hooray for unittests!
blink arghhh, it took me a while to figure out the includes ordering check is
case sensitive

[palmer, 2015-08-18 00:09:17]

https://codereview.chromium.org/1294933004/diff/40001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/1294933004/diff/40001/Source/platform/weborig...
Source/platform/weborigin/SecurityOriginTest.cpp:393:
"data:text/plain,hello_world",
What should happen for file: URLs? And filesystem:, and others?

[michaeln, 2015-08-18 20:22:16]

https://codereview.chromium.org/1294933004/diff/40001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/1294933004/diff/40001/Source/platform/weborig...
Source/platform/weborigin/SecurityOriginTest.cpp:393:
"data:text/plain,hello_world",
i've added some more cases, if you'd like to see other cases comment with the {
url, expectedBool, expectedString } lines and i'll plug them in

[michaeln, 2015-08-20 00:42:25]

palmer@ ping care to take a look at the tests?

[palmer, 2015-08-20 00:52:04]

LGTM. Thanks!

[michaeln, 2015-08-20 19:05:56]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2015-08-20 19:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1294933004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-08-20 19:06:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294933004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294933004/60001

[commit-bot: I haz the power, 2015-08-20 19:16:14]

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

[commit-bot: I haz the power, 2015-08-20 19:16:14]

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

[michaeln, 2015-08-20 21:12:01]

michaeln@chromium.org changed reviewers:
+ brettw@chromium.org

[michaeln, 2015-08-20 21:12:48]

brettw@ for owners review, ptal

[Mike West, 2015-08-21 18:41:20]

LGTM stampity stamp.

[michaeln, 2015-08-21 18:42:06]

On 2015/08/21 18:41:20, Mike West (traveling) wrote:
> LGTM stampity stamp.

thnx, i was just adding you as tbr and about to hit the commitQ button :)

[michaeln, 2015-08-21 18:43:26]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2015-08-21 18:43:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294933004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294933004/60001

[commit-bot: I haz the power, 2015-08-21 21:17:46]

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

[commit-bot: I haz the power, 2015-08-21 21:17:46]

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

[michaeln, 2015-08-21 22:03:57]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2015-08-21 22:04:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294933004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294933004/60001

[commit-bot: I haz the power, 2015-08-21 22:42:14]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201022

[dstockwell, 2015-08-25 04:09:43]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1315793002/ by dstockwell@chromium.org.

The reason for reverting is: Speculative revert for "Too many opened files in
the system"..

[sof, 2015-08-25 13:39:52]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-08-25 13:39:52]

https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:112: if
(schemeRequiresAuthority(relevantURL) && relevantURL.host().isEmpty())
If relevantURL is now checked for validity, is this check needed? The
String(Impl) allocation churn it brings, pains me :)

[michaeln, 2015-08-25 22:47:42]

https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
Source/platform/weborigin/SecurityOrigin.cpp:112: if
(schemeRequiresAuthority(relevantURL) && relevantURL.host().isEmpty())
On 2015/08/25 13:39:52, sof wrote:
> If relevantURL is now checked for validity, is this check needed? The
> String(Impl) allocation churn it brings, pains me :)

hmmm... good question... i'm not sure?

[sof, 2015-08-26 07:38:26]

On 2015/08/25 22:47:42, michaeln wrote:
>
https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
> File Source/platform/weborigin/SecurityOrigin.cpp (right):
> 
>
https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
> Source/platform/weborigin/SecurityOrigin.cpp:112: if
> (schemeRequiresAuthority(relevantURL) && relevantURL.host().isEmpty())
> On 2015/08/25 13:39:52, sof wrote:
> > If relevantURL is now checked for validity, is this check needed? The
> > String(Impl) allocation churn it brings, pains me :)
> 
> hmmm... good question... i'm not sure?

Me neither, but with the extra validity check added by this CL, I think we
should try to remove it. https://bugs.webkit.org/show_bug.cgi?id=72342 mooted it
as removable some time ago.

[michaeln, 2015-08-26 21:58:29]

On 2015/08/26 07:38:26, sof wrote:
> On 2015/08/25 22:47:42, michaeln wrote:
> >
>
https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
> > File Source/platform/weborigin/SecurityOrigin.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1294933004/diff/60001/Source/platform/weborig...
> > Source/platform/weborigin/SecurityOrigin.cpp:112: if
> > (schemeRequiresAuthority(relevantURL) && relevantURL.host().isEmpty())
> > On 2015/08/25 13:39:52, sof wrote:
> > > If relevantURL is now checked for validity, is this check needed? The
> > > String(Impl) allocation churn it brings, pains me :)
> > 
> > hmmm... good question... i'm not sure?
> 
> Me neither, but with the extra validity check added by this CL, I think we
> should try to remove it. https://bugs.webkit.org/show_bug.cgi?id=72342 mooted
it
> as removable some time ago.

I guess the question is whether an http+s or ftp url can be valid with an empty
host?

[palmer, 2015-08-26 22:07:23]

> I guess the question is whether an http+s or ftp url can be valid with an
empty
> host?

I don't believe so, no.

[michaeln, 2015-08-27 01:42:06]

ok, lets see wha happens over in https://codereview.chromium.org/1320793002/