783858 - Unicode in script names breaks scratchpad's recent files

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P2)

Description

[Thaddee Tyl [:espadrine]]

Reproducible.
1. Create a file on your hard drive, name it ☕.
2. Open a scratchpad, File > Open, select the file you have just created.
3. Restart Firefox.
4. Open the scratchpad: the File > Open Recent menu is greyed out.

We get the following output in the terminal:
JavaScript error: chrome://browser/content/scratchpad.js, line 744: SyntaxError: JSON.parse: bad control character in string literal

Comment 1

[Anton Kovalyov (:anton)]

Attachment: Use set/getComplexValue instead of set/getCharPref

This patch fixes the issue (get/setCharPref don't play nice with unicode strings) and updates recent-files test to test for unicode filenames.

Comment 2

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 674068 [details] [diff] [review]
Use set/getComplexValue instead of set/getCharPref

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

looks good. Use some consts!

::: browser/devtools/scratchpad/scratchpad.js
@@ +737,3 @@
>      if (branch.prefHasUserValue("recentFilePaths")) {
> +      let data = branch.getComplexValue("recentFilePaths",
> +        Components.interfaces.nsISupportsString).data;

you can use Ci.nsISupportsString here.

@@ +786,5 @@
> +    // WARNING: Do not use setCharPref here, it doesn't play nicely with
> +    // Unicode strings.
> +
> +    let str = Components.classes["@mozilla.org/supports-string;1"]
> +      .createInstance(Components.interfaces.nsISupportsString);

Cc and Ci. We have those consts for convenience!

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js
@@ +27,4 @@
>  
>  // Temporary file names.
>  let gFileName01 = "file01_ForBug651942.tmp"
> +let gFileName02 = "☕" // See bug 783858 for more information

delicious.

Comment 3

[Anton Kovalyov (:anton)]

Attachment: Use set/getComplexValue instead of set/getCharPref

Changed the patch to use Cc and Ci constants. Carrying over r+.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 674303 [details] [diff] [review]
Use set/getComplexValue instead of set/getCharPref

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

Wanted to land this patch, but I'd like to know if you have a green try push?

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js
@@ +27,4 @@
>  
>  // Temporary file names.
>  let gFileName01 = "file01_ForBug651942.tmp"
> +let gFileName02 = "☕" // See bug 783858 for more information

This might fail - based on previous experience you might need to use the unicode escape \uNNNN.

Comment 5

[Anton Kovalyov (:anton)]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=349cfa48cc70

Comment 6

[Anton Kovalyov (:anton)]

The build seems green to me.

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/9e56a20d367f

Comment 8

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

Backed out on suspicion of causing mochitest-1 and 3 permaorange:
https://hg.mozilla.org/integration/fx-team/rev/c5e7f0bbdb84

Comment 9

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

Relanded:
https://hg.mozilla.org/integration/fx-team/rev/45bfc549dcac

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/45bfc549dcac