WindowFeatures arguments shoud ignore invalid characters in values

WindowFeatures arguments shoud ignore invalid characters in values

Original patch from http://trac.webkit.org/changeset/169849

We were getting the length of the string before calling lower() on
it but we were looping over the new string which might have a
different length than the original.

BUG=383704
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176235

[arv (Not doing code reviews), 2014-06-12 21:26:48]

Update test further since our tests are different from webkits

[arv (Not doing code reviews), 2014-06-12 21:28:21]

PTAL

[cbiesinger, 2014-06-12 23:49:39]

lgtm

some random musings below.

https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html
(right):

https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html:15:
var w = window.open("resources/window-property-invalid-characters-ignored.html",
"blank", "width=123\u0130,height=123\u0130");
This test doesn't test what the code is changing, I think. However, I'm not sure
what a good test for the code change is.

For reference, here's what happens:
U+0130 = LATIN CAPITAL LETTER I WITH DOT ABOVE

which gets lowercased to:
U+0069 LATIN SMALL LETTER I
U+0307 COMBINING DOT ABOVE

I don't know why we add that combining dot. Maybe we decompose the string before
lowering it...? Note that Firefox does not lowercase the string the way we do:
"\u0130".length
1
"\u0130".toLowerCase().length
1
"\u0130".toLowerCase().charCodeAt(0).toString(16)
"69"

[arv (Not doing code reviews), 2014-06-13 14:12:03]

On 2014/06/12 at 23:49:39, cbiesinger wrote:
> lgtm
> 
> some random musings below.
> 
>
https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
> File
LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html
(right):
> 
>
https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
>
LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html:15:
var w = window.open("resources/window-property-invalid-characters-ignored.html",
"blank", "width=123\u0130,height=123\u0130");
> This test doesn't test what the code is changing, I think. However, I'm not
sure what a good test for the code change is.

How so? In the old code we get an index out bounds because we use length that is
too large in the loop.

> 
> For reference, here's what happens:
> U+0130 = LATIN CAPITAL LETTER I WITH DOT ABOVE
> 
> which gets lowercased to:
> U+0069 LATIN SMALL LETTER I
> U+0307 COMBINING DOT ABOVE
> 
> I don't know why we add that combining dot. Maybe we decompose the string
before lowering it...? Note that Firefox does not lowercase the string the way
we do:
> "\u0130".length
> 1
> "\u0130".toLowerCase().length
> 1
> "\u0130".toLowerCase().charCodeAt(0).toString(16)
> "69"

What Firefox does is not really relevant. This is about how WTF::String::lower
works.

[arv (Not doing code reviews), 2014-06-13 14:27:04]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-13 14:28:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/40001

[commit-bot: I haz the power, 2014-06-13 15:41:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11917)

[cbiesinger, 2014-06-13 15:57:49]

On 2014/06/13 14:12:03, arv wrote:
> On 2014/06/12 at 23:49:39, cbiesinger wrote:
> > lgtm
> > 
> > some random musings below.
> > 
> >
>
https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
> > File
> LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html
> (right):
> > 
> >
>
https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
> >
>
LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html:15:
> var w =
window.open("resources/window-property-invalid-characters-ignored.html",
> "blank", "width=123\u0130,height=123\u0130");
> > This test doesn't test what the code is changing, I think. However, I'm not
> sure what a good test for the code change is.
> 
> How so? In the old code we get an index out bounds because we use length that
is
> too large in the loop.

Yeah, but it's not guaranteed that that would crash.

> > For reference, here's what happens:
> > U+0130 = LATIN CAPITAL LETTER I WITH DOT ABOVE
> > 
> > which gets lowercased to:
> > U+0069 LATIN SMALL LETTER I
> > U+0307 COMBINING DOT ABOVE
> > 
> > I don't know why we add that combining dot. Maybe we decompose the string
> before lowering it...? Note that Firefox does not lowercase the string the way
> we do:
> > "\u0130".length
> > 1
> > "\u0130".toLowerCase().length
> > 1
> > "\u0130".toLowerCase().charCodeAt(0).toString(16)
> > "69"
> 
> What Firefox does is not really relevant. This is about how WTF::String::lower
> works.

It's relevant in that we might change String::lower to match Firefox at some
point, in which case this test behavior may change.

[arv (Not doing code reviews), 2014-06-13 16:19:14]

The CQ bit was unchecked by arv@chromium.org

[arv (Not doing code reviews), 2014-06-13 16:27:18]

On 2014/06/13 15:57:49, cbiesinger wrote:
> On 2014/06/13 14:12:03, arv wrote:
> > On 2014/06/12 at 23:49:39, cbiesinger wrote:
> > > lgtm
> > > 
> > > some random musings below.
> > > 
> > >
> >
>
https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
> > > File
> > LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/333643008/diff/20001/LayoutTests/fast/dom/Win...
> > >
> >
>
LayoutTests/fast/dom/Window/window-property-invalid-characters-ignored.html:15:
> > var w =
> window.open("resources/window-property-invalid-characters-ignored.html",
> > "blank", "width=123\u0130,height=123\u0130");
> > > This test doesn't test what the code is changing, I think. However, I'm
not
> > sure what a good test for the code change is.
> > 
> > How so? In the old code we get an index out bounds because we use length
that
> is
> > too large in the loop.
> 
> Yeah, but it's not guaranteed that that would crash.

You are right. This test is flawed.

There are two possible cases where things could go wrong before:

1. The new string is longer than the original (the case tested) and we end up
exit too early. However, the way the test was constructed, the new string is
only 2 characters longer so it just skips the last \u0069\u0307. This means that
the tests pass no matter what.

2. The new string is shorter than the original and we would get index out of
bounds.

I fixed the test to handle 1 correctly.

I'm looking into finding a character sequence to test case 2.

> 
> > > For reference, here's what happens:
> > > U+0130 = LATIN CAPITAL LETTER I WITH DOT ABOVE
> > > 
> > > which gets lowercased to:
> > > U+0069 LATIN SMALL LETTER I
> > > U+0307 COMBINING DOT ABOVE
> > > 
> > > I don't know why we add that combining dot. Maybe we decompose the string
> > before lowering it...? Note that Firefox does not lowercase the string the
way
> > we do:
> > > "\u0130".length
> > > 1
> > > "\u0130".toLowerCase().length
> > > 1
> > > "\u0130".toLowerCase().charCodeAt(0).toString(16)
> > > "69"
> > 
> > What Firefox does is not really relevant. This is about how
WTF::String::lower
> > works.
> 
> It's relevant in that we might change String::lower to match Firefox at some
> point, in which case this test behavior may change.

You cannot really draw any conclusions what WTF::String::lower does based in JS'
String.prototype.toLowerCase. That was just for a quick and dirty comparison.
The ECMAScript standard falls back on the unicode standard to defines how \u0130
should be lower cased.

[arv (Not doing code reviews), 2014-06-13 16:28:11]

Fix test

[arv (Not doing code reviews), 2014-06-13 16:28:16]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-13 16:29:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/60001

[commit-bot: I haz the power, 2014-06-13 17:40:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11932)

[cbiesinger, 2014-06-13 17:42:46]

On 2014/06/13 16:27:18, arv wrote:
> You cannot really draw any conclusions what WTF::String::lower does based in
JS'
> String.prototype.toLowerCase. That was just for a quick and dirty comparison.
> The ECMAScript standard falls back on the unicode standard to defines how
\u0130
> should be lower cased.

Fair enough. Still, as far as I can tell that our JS toLowerCase does not do the
right thing -- according to
http://www.fileformat.info/info/unicode/char/0130/index.htm, the lowercase of
U+0130 is just the standard i (U+0069).

[commit-bot: I haz the power, 2014-06-13 18:46:40]

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

[commit-bot: I haz the power, 2014-06-13 18:46:40]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11932)

[arv (Not doing code reviews), 2014-06-13 18:52:11]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-13 18:52:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/60001

[commit-bot: I haz the power, 2014-06-13 19:53:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11952)

[commit-bot: I haz the power, 2014-06-13 20:48:40]

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

[commit-bot: I haz the power, 2014-06-13 20:48:41]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11959)

[arv (Not doing code reviews), 2014-06-13 21:01:10]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-13 21:01:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/60001

[mathiasb, 2014-06-13 22:10:29]

On 2014/06/13 17:42:46, cbiesinger wrote:
> On 2014/06/13 16:27:18, arv wrote:
> > You cannot really draw any conclusions what WTF::String::lower does based in
> JS'
> > String.prototype.toLowerCase. That was just for a quick and dirty
comparison.
> > The ECMAScript standard falls back on the unicode standard to defines how
> \u0130
> > should be lower cased.
> 
> Fair enough. Still, as far as I can tell that our JS toLowerCase does not do
the
> right thing -- according to
> http://www.fileformat.info/info/unicode/char/0130/index.htm, the lowercase of
> U+0130 is just the standard i (U+0069).

The normative document is
http://unicode.org/Public/UCD/latest/ucd/CaseFolding.txt which contains:

0130; F; 0069 0307; # LATIN CAPITAL LETTER I WITH DOT ABOVE
0130; T; 0069; # LATIN CAPITAL LETTER I WITH DOT ABOVE

The `F` entry should be used when performing full case folding and the `T` entry
is a special case specifically for Turkish languages. It seems the `F` variant
is what would be expected here.

[commit-bot: I haz the power, 2014-06-13 22:12:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/11382)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11959)

[mathiasb, 2014-06-13 22:14:20]

On 2014/06/13 16:27:18, arv wrote:
> There are two possible cases where things could go wrong before: […]
> 
> 2. The new string is shorter than the original and we would get index out of
> bounds.
> 
> I'm looking into finding a character sequence to test case 2.

For the ‘Full’ case mappings, the uppercase string is always shorter than its
lowercased version, which can be of equal length or longer (but not shorter).
http://unicode.org/Public/UCD/latest/ucd/CaseFolding.txt

[commit-bot: I haz the power, 2014-06-13 23:00:39]

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

[commit-bot: I haz the power, 2014-06-13 23:00:39]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/11382)

[arv (Not doing code reviews), 2014-06-13 23:15:53]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-13 23:16:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/60001

[commit-bot: I haz the power, 2014-06-14 01:22:08]

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

[commit-bot: I haz the power, 2014-06-14 01:22:09]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/11382)

[arv (Not doing code reviews), 2014-06-16 15:47:05]

Maybe update the expected.txt next time

[arv (Not doing code reviews), 2014-06-16 15:48:32]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-16 15:48:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/80001

[commit-bot: I haz the power, 2014-06-16 17:56:39]

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

[commit-bot: I haz the power, 2014-06-16 17:56:40]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12134)

[arv (Not doing code reviews), 2014-06-16 18:46:49]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-06-16 18:48:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/arv@chromium.org/333643008/80001

[commit-bot: I haz the power, 2014-06-16 19:36:04]

Change committed as 176235

[cbiesinger, 2014-06-17 01:12:24]

On 2014/06/13 22:10:29, mathiasb wrote:
> On 2014/06/13 17:42:46, cbiesinger wrote:
> > On 2014/06/13 16:27:18, arv wrote:
> > > You cannot really draw any conclusions what WTF::String::lower does based
in
> > JS'
> > > String.prototype.toLowerCase. That was just for a quick and dirty
> comparison.
> > > The ECMAScript standard falls back on the unicode standard to defines how
> > \u0130
> > > should be lower cased.
> > 
> > Fair enough. Still, as far as I can tell that our JS toLowerCase does not do
> the
> > right thing -- according to
> > http://www.fileformat.info/info/unicode/char/0130/index.htm, the lowercase
of
> > U+0130 is just the standard i (U+0069).
> 
> The normative document is
> http://unicode.org/Public/UCD/latest/ucd/CaseFolding.txt which contains:
> 
> 0130; F; 0069 0307; # LATIN CAPITAL LETTER I WITH DOT ABOVE
> 0130; T; 0069; # LATIN CAPITAL LETTER I WITH DOT ABOVE
> 
> The `F` entry should be used when performing full case folding and the `T`
entry
> is a special case specifically for Turkish languages. It seems the `F` variant
> is what would be expected here.

Oh... that must be so that roundtripping works correctly. Hmm. I guess that
makes sense. Thanks for pointing to the canonical reference!