1067141 - Scratchpad Open File uses fallbackCharsetList on UTF-8 NS_ERROR_ILLEGAL_INPUT

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Adrian Aichner [:anaran]]

Attachment: NS_ERROR_ILLEGAL_INPUT.txt

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140913030206

Steps to reproduce:

Open NS_ERROR_ILLEGAL_INPUT.txt in Scratchpad


Actual results:

File is not opened.

The Browser Console, however, knows what's wrong:

17:57:28.431 NS_ERROR_ILLEGAL_INPUT: Component returned failure code: 0x8050000e (NS_ERROR_ILLEGAL_INPUT) [nsIScriptableUnicodeConverter.ConvertToUnicode] scratchpad.js:1084


Expected results:

Scratchpad should report to the user the file could not be converted to Unicode.

Perhaps the file can be converted to Unicode, with a warning to the user?

Comment 1

[Adrian Aichner [:anaran]]

Another related problem: The Scratchpad title gets set to the filename it later can't load.
This is very confusing when Scratchpad already had another file loaded before.
It now shows the wrong path for the text in the editor.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It seems we could have two patches:

1. Just a quick and dirty notification that we couldn't convert to unicode. This alone would be valuable and we can land this as soon as its ready so users know what's up while we are working on 2.

2. Figure out how to strip characters we can't convert, then do the conversion, and showing a warning explaining what we did.

Comment 3

[Adrian Aichner [:anaran]]

My browser (nightly) has this
intl.fallbackCharsetList.ISO-8859-1: windows-1252
        converter.charset = "windows-1252";
instead of
        converter.charset = "UTF-8";
passes the conversion.

Using
  importFromFile: function SP_importFromFile(aFile, aSilentError, aCallback)
  {
    // Prevent file type detection.
    let channel = NetUtil.newChannel(aFile);
    channel.contentType = "application/javascript";

    NetUtil.asyncFetch(channel, (aInputStream, aStatus) => {
      let content = null;

      if (Components.isSuccessCode(aStatus)) {
        let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
                        createInstance(Ci.nsIScriptableUnicodeConverter);
        converter.charset = "UTF-8";

Comment 4

[Adrian Aichner [:anaran]]

Some progress.

I have a little patch that tries conversion from UTF-8 followed by the list found in
Services.prefs.getCharPref("intl.fallbackCharsetList.ISO-8859-1")
"windows-1252"

It's easy to test the behaviour with
Services.prefs.setCharPref("intl.fallbackCharsetList.ISO-8859-1", "")
or
Services.prefs.setCharPref("intl.fallbackCharsetList.ISO-8859-1", "klingon")

I am attaching a screenshot for my default browser settings,
then I set the fallback list to "klingon" and the notification goes critical.

Comment 5

[Adrian Aichner [:anaran]]

Attachment: Screen Shot 2014-09-17 at 01.56.46.png

Default setting:
Conversion from UTF-8 fails, but then conversion from fallback succeeds, causing no final critical warning.

Comment 6

[Adrian Aichner [:anaran]]

Attachment: Screen Shot 2014-09-17 at 02.08.19.png

Setting the fallback to "klingon", to force it to fail, causes a final critical notification.

Comment 7

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140917T022422+0200.patch

Initial patch

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8490472 [details] [diff] [review]
0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140917T022422+0200.patch

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

Don't forget a test, too.

::: browser/devtools/scratchpad/scratchpad.js
@@ +1083,5 @@
>                          createInstance(Ci.nsIScriptableUnicodeConverter);
> +        let charsets = Services.prefs.getCharPref(
> +          FALLBACK_CHARSET_LIST).split(",").filter(function (value) {
> +            return value.length; });
> +        charsets.unshift("UTF-8");

Let's split out this getting-of-charsets to its own helper method. I'm getting cognitive overload with all the splitting and filtering in the middle of where I just want a list of charsets.

Also, function-closing-brackets on a newline, please.

@@ +1088,4 @@
>          content = NetUtil.readInputStreamToString(aInputStream,
>                                                    aInputStream.available());
> +        let sp = this;
> +        let success = charsets.some(function (charset) {

Instead of setting this to sp and then referring to sp in the function, you can just use an arrow function, which automatically keeps the outer scope's this binding.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

@@ +1101,5 @@
> +              null,
> +              sp.notificationBox.PRIORITY_WARNING_HIGH,
> +              null);
> +          }
> +        });

Likewise, can we make the whole give-me-the-converted-content logic a helper function? There's a ton of stuff going on here and it would be nice to break it down into smaller pieces that we just compose.

@@ +1105,5 @@
> +        });
> +        if (!success) {
> +          this.notificationBox.appendNotification(
> +            this.strings.formatStringFromName("importFromFile.convert.failed",
> +                                              [ charsets.join(",") ], 1),

Nit: I think it would result in a more readable message if we join the charsets with ", " rather than ",".

@@ +1109,5 @@
> +                                              [ charsets.join(",") ], 1),
> +            "file-import-convert-failed",
> +            null,
> +            this.notificationBox.PRIORITY_CRITICAL_MEDIUM,
> +            null);

Shouldn't we exit early here, now? We can't really proceed without content we understand, right?

Comment 9

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140919T015409+0200.patch

Addresses all review feedback to the best of my knowledge.

In addition, filename and recent files are no longer updated in openFile (where the outcome of the import cannot be known) but rather in importFormFile when appropriate.

This fixes the awkward issue where file content and scratchpad title get out of sync due to a failed import.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8491912 [details] [diff] [review]
0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140919T015409+0200.patch

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

r+ with couple tiny comments below.

::: browser/devtools/scratchpad/test/browser_scratchpad_open.js
@@ +90,5 @@
> +          let cn = nb.currentNotification;
> +          is(cn.priority, nb.PRIORITY_WARNING_HIGH, "notification priority is correct");
> +          is(cn.value, "file-import-convert-failed", "notification value is corrent");
> +          is(cn.type, "warning", "notification type is correct");
> +          // nb.removeCurrentNotification();

Remove this if you don't need it.

@@ +95,5 @@
> +          done();
> +        });
> +      ok(true, "importFromFile does not cause exception");
> +    } catch (exception) {
> +      ok(false, "importFromFile causes exception " + exception);

DevToolsUtils.safeErrorString(exception)

Comment 11

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140923T004356+0200.patch

Addressed you comments as usual, tests still pass.
I don't think I can quite yet ask the trybot to do its work and will need so pointers to do so anyway.

Could you please do that for me again?

Thanks!
Adrian

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8493407 [details] [diff] [review]
0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140923T004356+0200.patch

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

For the future:

https://wiki.mozilla.org/ReleaseEngineering/TryServer
http://trychooser.pub.build.mozilla.org/

Try push for now:

https://tbpl.mozilla.org/?tree=Try&rev=32ff6c1800ae

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

anaran, in a few hours can you check the try results and add the "checkin-needed" keyword if everything looks good?

Comment 14

[Adrian Aichner [:anaran]]

So, the results are not all green.

1.
Linux x86-64 Mulet try build on 2014-09-22 16:31:40 PDT for push 32ff6c1800ae
failed

2.
Ubuntu VM 12.04 try debug test mochitest-devtools-chrome-1 on 2014-09-22 16:52:30 PDT for push 32ff6c1800ae
slave: tst-linux32-spot-263
738 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_options_disable_cache.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - expected PASS

3.
Ubuntu VM 12.04 x64 try debug test mochitest-other on 2014-09-22 16:54:17 PDT for push 32ff6c1800ae
slave: tst-linux64-spot-166
ReferenceError: can't access let declaration `beenHere1' before initialization: next@chrome://mochitests/content/chrome/toolkit/devtools/tests/mochitest/test_eventemitter_basic.html:61:14
ReferenceError: can't access let declaration `beenHere1' before initialization: next@chrome://mochitests/content/chrome/toolkit/devtools/tests/mochitest/test_eventemitter_basic.html:61:14

At first these don't seem related to my changes.

3 might be related to "temporal dead zone"

How should we proceed?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I'm getting the following error trying to apply this patch:

Parsing...Patch id=8491912 desc="0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140919T015409+0200.patch" diff data were discarded:
UnicodeDecodeError: 'utf8' codec can't decode byte 0xe4 in position 7234: invalid continuation byte
Patch id=8493407 desc="0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-20140923T004356+0200.patch" diff data were discarded:
UnicodeDecodeError: 'utf8' codec can't decode byte 0xe4 in position 7234: invalid continuation byte
 done

Please fix your patch. Also, please make sure your commit message follows the guidelines below. Thanks :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 16

[Adrian Aichner [:anaran]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> I'm getting the following error trying to apply this patch:
> 
> Parsing...Patch id=8491912
> desc="0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-
> 20140919T015409+0200.patch" diff data were discarded:
> UnicodeDecodeError: 'utf8' codec can't decode byte 0xe4 in position 7234:
> invalid continuation byte

This first patch is marked obsolete.

> Patch id=8493407
> desc="0001-Bug-1067141-Scratchpad-File-Open-File-fails-to-repor-
> 20140923T004356+0200.patch" diff data were discarded:
> UnicodeDecodeError: 'utf8' codec can't decode byte 0xe4 in position 7234:
> invalid continuation byte
>  done

This is the right one and has the same problem, which is caused by the non-UTF-8 character in the test file.

How can I fix that?

> 
> Please fix your patch. Also, please make sure your commit message follows
> the guidelines below. Thanks :)
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Committing_Rules_and_Responsibilities#Checkin_comment

Summary now describes solution and preserves error type users might search for in bugzilla.

Comment 17

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1067141-Scratchpad-Open-File-uses-fallbackCharse-20140925T010434+0200.patch

Rebased against fx-team minutes ago.
Uses new bug summary to comply with commit message guidelines.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/5dae7e831ad7

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dae7e831ad7

Comment 20

[Ton [:Tonnes]]

 # LOCALIZATION NOTE  (openFile.failed): This is the message displayed when file
 # open fails.
 openFile.failed=Failed to read the file.
 
+# LOCALIZATION NOTE  (openFile.failed): This is the message displayed when file
+# open fails.
+importFromFile.convert.failed=Failed to convert file to Unicode from %1$S.
+

Note that the new comment was probably copied over and left unchanged. A comment about what %1$S represents would also be welcome. Could anyone fix this some time?

Comment 21

[Adrian Aichner [:anaran]]

Attachment: Bug1067141-locales-20141209T203611+0100.patch

Comment 22

[Adrian Aichner [:anaran]]

Comment on attachment 8533905 [details] [diff] [review]
Bug1067141-locales-20141209T203611+0100.patch

Thanks for pointing out slip, Ton.

Are you involved with localisation for Firefox?

Please review my attached patch and open a new bug since this one is closed.

Comment 23

[Ton [:Tonnes]]

Comment on attachment 8533905 [details] [diff] [review]
Bug1067141-locales-20141209T203611+0100.patch

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

Sorry for late reply Adrian and thanks for the patch.

Yes I'm involved in l10n, hence I noticed it.

Do you think a new bug is worth filing just for a comment? My point was to take this along when someone is doing new edits some day like I see happening occasionally.

Comment 24

[Adrian Aichner [:anaran]]

Thanks for the review, Ton.

Might not be worth a new bug.

Can you carry this patch into firefox?

Comment 25

[Ton [:Tonnes]]

Well in short, no - I do some hg commits and supply patches for l10n sections, but applying patches is another thing, especially for en-US where access or even other permissions might be an issue. I’d also need to pull the entire central repository and Tortoise on Windows has a bad reputation for applying patches unless by command line perhaps. So could you, or anyone above?

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/6c3fd57d905b

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

I landed it, but next time please file a separate bug.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c3fd57d905b