Don't initialize ICU replacement character for UTF-32 encodings

Don't initialize ICU replacement character for UTF-32 encodings

The TextCodecICU wrapper erroneously called ucnv_setSubstChars() with
only a single byte replacement for non-byte-based encodings -
basically the UTF-32 family. The code path is only exercised when
computing names for files in form uploads. This would hit an ASSERT in
debug builds, and result in an empty filename in release builds.

BUG=662146, 417850
R=tkent@chromium.org

Committed: https://crrev.com/2e2c129641218bcf03de40f74124a5c4f7c21f86
Cr-Commit-Position: refs/heads/master@{#430356}

[jsbell, 2016-11-03 21:24:21]

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

[commit-bot: I haz the power, 2016-11-03 21:25:00]

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

[jsbell, 2016-11-03 21:30:01]

tkent@ - please take a look?

I have not yet added a LayoutTest. It would require an http test with a php that
echoes back the filename - doable but do you think it's worth it? I suppose I
probably should to ensure that all of the utf-32 variants respond to
TextEncoding::encodingForFormSubmission properly.

Also note that per https://crbug.com/417850 we do want to eliminate UTF-32 but
that's more work and investigation (i.e. is it necessary for XHTML support?). I
hate to add the UTF32Encoding (non-LE/BE) WTF_EXPORT but it is necessary to
avoid any behavior changes (and TextEncoding::isNonByteBasedEncoding was wrong)

[jsbell, 2016-11-03 21:44:13]

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

[commit-bot: I haz the power, 2016-11-03 21:44:59]

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

[jsbell, 2016-11-03 22:49:46]

https://codereview.chromium.org/2478653002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/TextCodecICU.cpp (right):

https://codereview.chromium.org/2478653002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/TextCodecICU.cpp:614: if
(!m_encoding.isNonByteBasedEncoding())
A side effect of correcting TextEncoding::isNonByteBasedEncoding() to include
UTF-32 (non-LE/BE) is that form submission (the original repro) uses UTF-8 for
pages with that encoding c/o TextEncoding::encodingForFormSubmission(). 

Which implies that this code should never get hit for UTF-32 since we believe
form submission is the only place where QuestionMarksForUnencodables gets used.
We could instead make this:

DCHECK(!m_encoding.isNonByteBasedEncoding())

... but I think it makes sense to keep the CL as is for now.

[commit-bot: I haz the power, 2016-11-03 22:59:46]

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

[commit-bot: I haz the power, 2016-11-03 22:59:47]

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_...)

[tkent, 2016-11-03 23:32:06]

lgtm.

I think lacking a layout test is ok.

[jsbell, 2016-11-03 23:33:39]

Bummer - MSVC doesn't like const uint_t data[] = {}; 

e:\b\c\b\win\src\third_party\webkit\source\wtf\text\textcodecicutest.cpp(47):
error C2466: cannot allocate an array of constant size 0

[jsbell, 2016-11-03 23:37:03]

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

[commit-bot: I haz the power, 2016-11-03 23:38:03]

Dry run: 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-11-04 04:00:11]

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

[commit-bot: I haz the power, 2016-11-04 04:00:12]

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_...)

[jsbell, 2016-11-04 16:04:09]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-11-04 16:04:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2478653002/#ps40001
(title: "Don't use zero-length array initializer, to keep MSVC happy")

[commit-bot: I haz the power, 2016-11-04 16:04:58]

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

[commit-bot: I haz the power, 2016-11-04 16:04:59]

This CL has an open dependency (Issue 2464373006 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[jsbell, 2016-11-04 17:04:33]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-11-04 17:05:15]

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

[commit-bot: I haz the power, 2016-11-04 17:05:16]

This CL has an open dependency (Issue 2464373006 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[jsbell, 2016-11-04 19:25:16]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-11-04 19: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, 2016-11-05 01:18:35]

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

[commit-bot: I haz the power, 2016-11-05 01:18:37]

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_...)

[jsbell, 2016-11-07 17:06:38]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-11-07 17:07:11]

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-11-07 20:18:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-07 20:22:16]

Description was changed from

==========
Don't initialize ICU replacement character for UTF-32 encodings

The TextCodecICU wrapper erroneously called ucnv_setSubstChars() with
only a single byte replacement for non-byte-based encodings -
basically the UTF-32 family. The code path is only exercised when
computing names for files in form uploads. This would hit an ASSERT in
debug builds, and result in an empty filename in release builds.

BUG=662146,417850
R=tkent@chromium.org
==========

to

==========
Don't initialize ICU replacement character for UTF-32 encodings

The TextCodecICU wrapper erroneously called ucnv_setSubstChars() with
only a single byte replacement for non-byte-based encodings -
basically the UTF-32 family. The code path is only exercised when
computing names for files in form uploads. This would hit an ASSERT in
debug builds, and result in an empty filename in release builds.

BUG=662146,417850
R=tkent@chromium.org

Committed: https://crrev.com/2e2c129641218bcf03de40f74124a5c4f7c21f86
Cr-Commit-Position: refs/heads/master@{#430356}
==========

[commit-bot: I haz the power, 2016-11-07 20:22:17]

Patchset 3 (id:??) landed as
https://crrev.com/2e2c129641218bcf03de40f74124a5c4f7c21f86
Cr-Commit-Position: refs/heads/master@{#430356}