See attached screenshot from Cristian.

Looks like the worker and/or OS.File stuff can't cope. Cristian said the profile name is something like "Åδú[relaxed emoji]╫▲Ω¼".

Adding the profile directly here since copy/paste from slack seems to not do the trick.

Åδú☺╫▲Ω¼

The priority flag is not set for this bug.
:tspurway, could you have a look please?

For more information, please visit auto_nag documentation.

Gijs, is there any user facing breakage on the new tab page as a result of this bug?

(In reply to Tim Spurway [:tspurway] from comment #3)

Gijs, is there any user facing breakage on the new tab page as a result of this bug?

I would expect no working screenshot-based thumbnails for anything given that no files are saved, but I haven't tried to reproduce this bug.

Can you get us some more details around the complexity of this fix?

Attaching screenshot of replicated error in browser console. No breakage of thumbnails generation seen for profile name with unicode characters

Its complaining here https://searchfox.org/mozilla-central/source/toolkit/components/thumbnails/PageThumbs.jsm#502

NI David who touched this code in https://bugzilla.mozilla.org/show_bug.cgi?id=753768 to help explain implication of this error. Thanks!

The most likely culprit would be OS.File not handling properly Unicode names depending on the file system. If I recall correctly, OS.File uses the default translation of strings provided by js-ctypes (I think it's UTF-8), which may not may to what the file system is using.
Could this explain the issue here?

If that's the issue, I'm unfortunately not sure how to fix it.

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #8)

The most likely culprit would be OS.File not handling properly Unicode names depending on the file system. If I recall correctly, OS.File uses the default translation of strings provided by js-ctypes (I think it's UTF-8), which may not may to what the file system is using.
Could this explain the issue here?

I don't know. How would we check?

The simplest way I can think of:

1. Under e.g. Linux, create & mount a file system that does not UTF-8 encoding, for instance a JP encoding. You can probably do this with a ramdisk, but I don't know the details.
2. Use OS.File to write a file whose name contains non-Latin1 chars.
3. Check whether it works.
4. Check whether the command-line whether the file has the right name.

I couldn't reproduce, but one question I have is: Is this a problem across all of firefox, or just a problem in newtab?

Seemingly if a user makes a profile with that name, other places in the browser are also doing OS.FIle makedir, at the very least to create the Root Directory and Local Directory I see listed in about:profiles, right? So either this is solved already, or it's broken across the browser. I'm guessing it IS solved, and I'm guessing the hardest part about this is getting it reproduced.

(In reply to Scott [:thecount] Downe from comment #11)

I couldn't reproduce, but one question I have is: Is this a problem across all of firefox, or just a problem in newtab?

Seemingly if a user makes a profile with that name, other places in the browser are also doing OS.FIle makedir, at the very least to create the Root Directory and Local Directory I see listed in about:profiles, right? So either this is solved already, or it's broken across the browser. I'm guessing it IS solved, and I'm guessing the hardest part about this is getting it reproduced.

No, there are at least 2, and probably more, ways to do IO - OS.File and XPCOM nsILocalFile. The latter is probably better compatible with unicode stuff because it's much older and better tested, and explains why we don't just break everything everywhere (though it still happens e.g. bug 1388584, bug 329898).

Maybe it's reasonable to look into converting some of the problematic places in PageTHumbs.jsm/PageThumbsWorker.js to use nsILocalFile and see if that fixes it?

(In reply to Scott [:thecount] Downe from comment #13)

Maybe it's reasonable to look into converting some of the problematic places in PageTHumbs.jsm/PageThumbsWorker.js to use nsILocalFile and see if that fixes it?

This isn't easily doable - the point of OS.File is that it does its IO off the main thread, and you can't really do that in JS using XPCOM/nsILocalFile.

Unfortunately, OS.File is something of a pile of hacks. Largely my fault, too :/

See also bug 1231711 for a (currently unmanned) effort to rewrite OS.File in native code and get rid of the hacks.

I can reproduce the problem by running the following script in Browser Console:

XPCOMUtils.defineLazyServiceGetter(this, "PageThumbsStorageService", "@mozilla.org/thumbnails/pagethumbs-service;1", "nsIPageThumbsStorageService");
ChromeUtils.import("resource://gre/modules/PromiseWorker.jsm", this);
var PageThumbsWorker = new BasePromiseWorker("resource://gre/modules/PageThumbsWorker.js");
PageThumbsWorker.ExceptionHandlers["OS.File.Error"] = OS.File.Error.fromMsg;
PageThumbsWorker.post("makeDir",
      [PageThumbsStorageService.path, {ignoreExisting: true}]).catch(function onError(aReason) {
          Cu.reportError("Could not create thumbnails directory" + aReason);
      });

PageThumbsStorageService.path is already broken:

> PageThumbsStorageService.path
< "C:\\Users\\nesuser\\AppData\\Local\\Mozilla\\Firefox\\Profiles\\qktp75kv.Å´ú:k²©¼\\thumbnails"

(In reply to Masatoshi Kimura [:emk] from comment #17)

PageThumbsStorageService.path is already broken:

> PageThumbsStorageService.path
< "C:\\Users\\nesuser\\AppData\\Local\\Mozilla\\Firefox\\Profiles\\qktp75kv.Å´ú:k²©¼\\thumbnails"

I assume this means OS.Constants.Path.localProfileDir is also broken?

If I create PageThumbsStorageService with the following snippet, PageThumbsStorageService.path is correct and no error happens:

let {PageThumbsStorageService} = ChromeUtils.import("resource://gre/modules/PageThumbsStorageService.jsm", {});
PageThumbsStorageService = new PageThumbsStorageService;

Some XPCOM wrapper goops breaks things?

Hey, nsIPageThumbsStorageService.path is declared as an ACString:
https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/toolkit/components/thumbnails/nsIPageThumbsStorageService.idl#24
It must be the culprit.

(In reply to :Gijs (he/him) from comment #18)

I assume this means OS.Constants.Path.localProfileDir is also broken?

To be clear, only nsIPageThumbsStorageService.path is broken. OS.File handles Unicode pathnames correctly unless the input path is already broken.

https://hg.mozilla.org/mozilla-central/rev/b686d09763cc

https://hg.mozilla.org/mozilla-central/rev/b686d09763cc

Please nominate this for Beta approval when you get a chance.

Comment on attachment 9070302 [details]
Bug 1549386 - Turn nsIPageThumbsStorageService.path into AString. r?Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Some users (mostly on non-English locales) may not see thumbnails on new tabs.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #16.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only corrected an XPCOM attribute type.
• String changes made/needed:

Comment on attachment 9070302 [details]
Bug 1549386 - Turn nsIPageThumbsStorageService.path into AString. r?Gijs

fix thumbnails with non-ascii paths, approved for 68.0b10

I have verified that the issue is no longer reproducible on the latest Nightly 69.0a1 (Build ID 20190611213517) on Windows 10, macOS 10.14, and Arch Linux 4.14.3.
Leaving the qe-verify flag in place until the issue is verified on Beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/2708dccfdc6a

Marking this as verified based on comment 29.