1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

https://dxr.mozilla.org/comm-central/search?q=convertFromByteArray&redirect=false

Mostly used in Calendar and causing test failures, see bug 1457087 comment #37.

The use in jsmime.js is "dead" code. The FakeTextDecoder is only used for UTF-7, and that doesn't run
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/mime/src/jsmime.jsm#65

I'm not sure how to handle that.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1469499-disable-test.patch

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5365296a9690
temporarily disable test_ics_parser.js. rs=bustage-fix

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Review commit: https://reviewboard.mozilla.org/r/251582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251582/

Comment 4

[Jorg K (CEST = GMT+2)]

Umm, you've removed the FakeTextDecoder() from JS Mime, unless I'm missing something. We need that to decode UTF-7 in message headers and there are tests covering that.

I believe https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/mime/src/jsmime.jsm#62-71 is dead code, since the fake decoder is only used for UTF-7 which is handled above that block.

I suggest to comment-out or remove that block and replace it with a Cu.reportError() of some sort since that code should be reached. That's pretty clear since convertFromByteArray() was removed and we had no test failures in JS Mime.

Comment 5

[Geoff Lankow (:darktrojan)]

Oh, I thought you were saying the whole thing was dead code. Oops.

Comment 6

[Jorg K (CEST = GMT+2)]

Looking at
https://hg.mozilla.org/try-comm-central/rev/a9e9944f84f883f8bc858f9afa1512bc305572b6#l6.33
I think I gave you the wrong advice. Joshua particularly doesn't like any Mozilla-isms in his "pure JS" library. So it would be better to replace the Cu.reportError() by some throw, like:
  throw new Error("FakeTextDecoder: This code should never be reached for " + this.charset);
Sorry about the change of mind.

Comment 7

[Jorg K (CEST = GMT+2)]

If I'm not mistaken, you didn't backout https://hg.mozilla.org/comm-central/rev/5365296a9690 for your try run.

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251582/diff/1-2/

Comment 9

[Geoff Lankow (:darktrojan)]

You're right. I've made another try push with the changes, and now I've folded the backout into the patch.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251582/diff/2-3/

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

r+ for the JS Mime hunk.

Comment 12

[Jorg K (CEST = GMT+2)]

Actually, I've changed my mind again.

Let me explain what's happening here in Joshua's tricky way.

We start with
var RealTextDecoder = TextDecoder;
function FallbackTextDecoder(charset, options) {
  try {
    return new RealTextDecoder(charset, options);
  } catch (e) {
    return new FakeTextDecoder(charset, options);
  }
}
TextDecoder = FallbackTextDecoder;

Aceman has a patch in bug 1424359 to defuse this trickery.

Basically when calling |new TextDecoder(charset, ...)| elsewhere in JS Mime, either the original TextDecoder is used, or in the catch block, our fake one. The catch should only be entered for UTF-7 which the original decoder doesn't support, and never supported. Prior to bug 1261841 the scriptable Unicode converter could be "enhanced" with more charsets, and TB added UTF-7 (and others?) to it. Today the scriptable Unicode converter is mapped onto encoding_rs and supports exactly what the original TextDecoder supports.

So it's time for some/most/all of this trickery to go.

Looking at your patch again, you need to remove more code:
    this._encoder = Cc[
      "@mozilla.org/intl/scriptableunicodeconverter"]
      .createInstance(Ci.nsIScriptableUnicodeConverter);
    this._encoder.isInternal = true;
can go.

this._encoder.charset = this.charset;
can be replaced by the same throw we have already.

get encoding() { return this._encoder.charset; },
can go, too.

And then, the comment above those functions needs to be tweaked as well.

Since JS Mime isn't failing after the removal of convertFromByteArray(), I suggest to remove the JS Mime hunk from your patch and let us handle it in bug 1424359.

Comment 13

[Jorg K (CEST = GMT+2)]

See attachment 8986641 [details] [diff] [review] for what I mean.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Sorry about the noise. Lets' deal with the JS Mime part in bug 1424359.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

https://reviewboard.mozilla.org/r/251582/#review258448

r+ with comments considered

::: calendar/base/modules/utils/calProviderUtils.jsm:103
(Diff revision 3)
>       * @param {String} aCharset         The character set of the bytes, defaults to utf-8
> -     * @param {Boolean} aThrow          If true, the function will raise an exception on error
>       * @return {?String}                The string result, or null on error
>       */
> -    convertByteArray: function(aResult, aResultLength, aCharset, aThrow) {
> -        try {
> +    convertByteArray: function(aResult, aCharset="utf-8") {
> +        return new TextDecoder(aCharset).decode(aResult);

It is highly possible this function is used by addons. I'd suggest to get rid of this function (almost) entirely by using TextDecoder directly in Lightning where before convertByteArray was used. Then keep this function around with the old arguments and a warning using Deprecated.jsm, and call the text decoder as you did.

Note you may have to use Cu.importGlobalProperties to get TextDecoder everywhere.

Comment 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251582/diff/3-4/

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251582/diff/4-5/

Comment 18

[Geoff Lankow (:darktrojan)]

Happy with these changes, Philipp?

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

https://reviewboard.mozilla.org/r/251582/#review259024

Looks good, one minor comment.

::: calendar/base/modules/utils/calProviderUtils.jsm:106
(Diff revisions 3 - 5)
> +     * @param {Boolean} aThrow          If true, the function will raise an exception on error
>       * @return {?String}                The string result, or null on error
>       */
> -    convertByteArray: function(aResult, aCharset="utf-8") {
> +    convertByteArray: function(aResult, aResultLength, aCharset="utf-8", aThrow) {
> +        try {
> -        return new TextDecoder(aCharset).decode(aResult);
> +            return new TextDecoder(aCharset).decode(aResult);

Somewhere here I'd add a Deprecated.warning("The cal.provider.convertByteArray() function has been superseded by the use of TextDecoder", "https://link/to/bug-id"); and import Deprecated.jsm (lazily)

Comment 20

[Geoff Lankow (:darktrojan)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251582/diff/5-6/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2135caba7fd1
Port bug 1444329: Replace use of nsIScriptableUnicodeConverter::convertFromByteArray. r=philipp

Comment 22

[Geoff Lankow (:darktrojan)]

I don't think this works. TextDecoder won't decode an Array, only an ArrayBuffer or ArrayBufferView. The fix is simple, but the fact that no tests broke (other than test_gdata_provider.js which is disabled) is a little worrying.

Comment 23

[Geoff Lankow (:darktrojan)]

Attachment: 1469499-textdecoder-1.diff

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8988070 [details] [diff] [review]
1469499-textdecoder-1.diff

Review of attachment 8988070 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, we don't really have provider tests. I have a caldav provider test in my pipeline for the autoconfig bug though.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8988070 [details] [diff] [review]
1469499-textdecoder-1.diff

Needs uplift to TB 62/Cal 6.4 if we want to get Calendar going again at that version.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986126 [details]
Bug 1469499 - Port Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray

Oops, that's already landed on TB 62.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bb50c248bd5d
Convert Arrays to Uint8Arrays before trying to decode them with TextDecoder. r=philipp DONTBUILD

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (Calendar 6.4):
https://hg.mozilla.org/releases/comm-beta/rev/dda2d0fbcf09643930a1c08c4409b2781c4b4ff6
Needs to be PUA (post-uplift approval) or you give me the right to approve trivial things myself ;-)

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8988070 [details] [diff] [review]
1469499-textdecoder-1.diff

I'm not a fan of uplifting without approval, this was not so urgent that it could not have waited for a+.

Comment 30

[Jorg K (CEST = GMT+2)]

Philipp, I think the approval process is there so the module owner gets the chance to approve or reject "risky" uplift requests. This one here was purely administrative, since parts of the bug already landed on TB 62/Cal 6.4.

Also, I land beta stuff in bulk, and right now, I have to manage two beta branches, 60 and 62, which is a mess and highly error prone since there is also only one beta flag and it's therefore quite difficult to find which patch goes to what beta. So I'd like to do things while they are fresh in memory. I also need to update to different branches locally, which is another source of error. So please let me do my work in peace. It appears that you like to contradict me out of principle, but this is really groundless in this case.

As I said, grant me the right to approve trivial stuff.