Closed Bug 1067141 Opened 8 years ago Closed 7 years ago

Scratchpad Open File uses fallbackCharsetList on UTF-8 NS_ERROR_ILLEGAL_INPUT

Categories

(DevTools Graveyard :: Scratchpad, defect)

35 Branch
x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: anaran, Assigned: anaran)

Details

Attachments

(5 files, 3 obsolete files)

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?
Component: Untriaged → Developer Tools: Scratchpad
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.
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.
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";
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.
Default setting:
Conversion from UTF-8 fails, but then conversion from fallback succeeds, causing no final critical warning.
Setting the fallback to "klingon", to force it to fail, causes a final critical notification.
Initial patch
Attachment #8490472 - Flags: review?(nfitzgerald)
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?
Attachment #8490472 - Flags: review?(nfitzgerald) → feedback+
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.
Attachment #8490472 - Attachment is obsolete: true
Attachment #8491912 - Flags: review?(nfitzgerald)
Assignee: nobody → adrian.aichner
Status: UNCONFIRMED → NEW
Ever confirmed: true
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)
Attachment #8491912 - Flags: review?(nfitzgerald) → review+
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
Attachment #8491912 - Attachment is obsolete: true
Attachment #8493407 - Flags: review?(nfitzgerald)
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
Attachment #8493407 - Flags: review?(nfitzgerald) → review+
anaran, in a few hours can you check the try results and add the "checkin-needed" keyword if everything looks good?
Flags: needinfo?(adrian.aichner)
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?
Flags: needinfo?(adrian.aichner)
Keywords: checkin-needed
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
Keywords: checkin-needed
Summary: Scratchpad->File->Open File fails to report NS_ERROR_ILLEGAL_INPUT → Scratchpad Open File uses fallbackCharsetList on UTF-8 NS_ERROR_ILLEGAL_INPUT
(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.
Rebased against fx-team minutes ago.
Uses new bug summary to comply with commit message guidelines.
Attachment #8493407 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5dae7e831ad7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
 # 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 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 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.
Attachment #8533905 - Flags: review?(tonnes.mb) → review+
Thanks for the review, Ton.

Might not be worth a new bug.

Can you carry this patch into firefox?
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?
I landed it, but next time please file a separate bug.
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.