875348 - sandboxName needs to support non-ascii chars

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[Anthony Lieuallen]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0 (Beta/Release)
Build ID: 20130326150557

Steps to reproduce:

Downstream bug report to Greasemonkey:
https://github.com/greasemonkey/greasemonkey/issues/1739

Reduced to test case:

1. Open about:memory (in test, this is the only open tab).
2. Run the script pasted below in a Environment>Browser Scratchpad.
3. Click measure.

================
var windowMediator = Components
   .classes['@mozilla.org/appshell/window-mediator;1']
   .getService(Components.interfaces.nsIWindowMediator);
var win = windowMediator.getMostRecentWindow("navigator:browser");
var contentWin = win.gBrowser.contentWindow;

var contentSandbox = new Components.utils.Sandbox(
    contentWin,
    {
      'sandboxName': '豆藤 Bean vine',
      'sandboxPrototype': contentWin,
      'wantXrays': true,
    });
Components.utils.evalInSandbox('alert("in sandbox!")', contentSandbox);
================

Firefox nightly "Built from http://hg.mozilla.org/mozilla-central/rev/00b264c7cced"

The same seems to happen with wantXrays:false.


Actual results:

The about:memory page displays:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMemoryMultiReporter.collectReports]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/content/aboutMemory.js :: processMemoryReporters :: line 212" data: no]


Expected results:

The tree of memory consumption/comparments/whatever should be displayed.

Comment 1

[Drew Willcoxon :adw]

So in summary, you run some custom script in a sandbox in about:memory's window, and after that clicking the Measure button results in the error you pasted.

Comment 2

[Anthony Lieuallen]

Ok, I over-simplified the steps in my reduction.  Include switching to any page in any tab before step two, or only opening about:memory after doing that, and it still happens.  It doesn't have to be run in about:memory.

Or:

1. Open about:mozilla.
2. Run the above script
3. Open about:memory
4. Press measure (if Firefox is new enough to have that button), fail

And actually, happens in Firefox 21 as well.  Something just doesn't handle that character being in the sandboxName when it tries to read that compartment.

Comment 3

[Drew Willcoxon :adw]

I probably should have moved this to wherever sandbox bugs go (Core::JS Engine?) instead of about:memory.

Comment 4

[Boris Zbarsky [:bzbarsky]]

When setting up the sandbox we do:

3642     char *tmp = JS_EncodeString(cx, value.toString());

That's a lossy operation; after that the string will contain garbage in this case.

Then we store that garbage in the CompartmentPrivate.

Not sure what goes wrong in later processing of that garbage in the memory reporter code, of course.

Basically, we only support ASCII sanbox names at the moment, given that JS_EncodeString bit.

Comment 5

[Bobby Holley (:bholley)]

I'm unlikely to fix this, but will take a patch with tests.

Comment 6

[janekptacijarabaci]

Confirmed on:
Firefox 26.0
Firefox 27.0 Beta

Not confirmed on:
Firefox 28.0a2 Aurora (2014-02-03)
Firefox 29.0a1 Nightly (2014-02-03)

Comment 7

[Elbart]

Breaking of about:memory has been fixed in this range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=412ab2e915cf&tochange=cca1dea21a0d

Probably by
cca1dea21a0d	Nicholas Nethercote — Bug 929797 - Implement proper memory reporting for child processes. r=khuey.

And the line mentioned in comment 4 at least has been changed by

e2c492172295	Gabor Krizsanits — Bug 925293 - Refactoring SandboxOptions parsing. r=bholley

http://hg.mozilla.org/mozilla-central/diff/e2c492172295/js/xpconnect/src/Sandbox.cpp#l1.287

So, is the issue fixed properly?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Elbart from comment #7)
> And the line mentioned in comment 4 at least has been changed by
> 
> e2c492172295	Gabor Krizsanits — Bug 925293 - Refactoring SandboxOptions
> parsing. r=bholley
> 
> http://hg.mozilla.org/mozilla-central/diff/e2c492172295/js/xpconnect/src/
> Sandbox.cpp#l1.287

It looks like the new code is still doing lossy UTF->ASCII conversion described in comment 4.

So even though about:memory may be handling this better, I think we still need to fix the underlying problem. We need to replace the char* with char16_t*, and the nsCString with an nsString.

Comment 9

[vinayakagarwal6996]

Hello,
Is the bug still available to fix?

Thanks

Comment 10

[Bobby Holley (:bholley)]

Sure :-)

Comment 11

[vinayakagarwal6996]

I needed some help for the same.
I can't find a string class function that returns char16_t from a js string.
Also there is a function overloading for OptionsBase::ParseString for both nsCString and nsString.
That is the only place nsCString is used.

Comment 12

[Bobby Holley (:bholley)]

(In reply to vinayakagarwal6996 from comment #11)
> I needed some help for the same.
> I can't find a string class function that returns char16_t from a js string.

You should be able to just use nsAutoJSString, but why not just switch to the other overload of ParseString?

> Also there is a function overloading for OptionsBase::ParseString for both
> nsCString and nsString.
> That is the only place nsCString is used.

Then you can remove it. :-)

Comment 13

[vinayakagarwal6996]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
 
> You should be able to just use nsAutoJSString, but why not just switch to
> the other overload of ParseString?


The overload is used else where.
Also,
> JSAutoByteString utf8str;
>    char* cstr = utf8str.encodeUtf8(cx, str);

nsAutoJSString still doesn't provide a way to return a char16_t pointer.
Sorry for the silly questions.

Comment 14

[Bobby Holley (:bholley)]

(In reply to vinayakagarwal6996 from comment #13)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
>  
> > You should be able to just use nsAutoJSString, but why not just switch to
> > the other overload of ParseString?
> 
> 
> The overload is used else where.

There are two overloads for ParseString. One of which parses to nsCString (8-bit chars, which might be ascii, latin1, or utf8), and one of which parses to nsString (16-bit chars, pretty much always UTF-16).

http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/js/xpconnect/src/xpcprivate.h#3049

Currently, the nsCString overload is used exactly once (when parsing sandboxName), and the nsString overload is used exactly once (when parsing charset). It's tempting to just make sandboxName nsString instead of nsCString, but it's used in various places that would make that difficult. So I don't think we can remove either overload.

My suggestion to fix this specific bug is to do something like:

AutoJSString str;
if (!str.init(mCx, value))
  return false;
prop.Assign(NS_ConvertUTF16toUTF8(str));


> Also,
> > JSAutoByteString utf8str;
> >    char* cstr = utf8str.encodeUtf8(cx, str);
> 
> nsAutoJSString still doesn't provide a way to return a char16_t pointer.

It's not clear to me why that's needed.

> Sorry for the silly questions.

No worries!

Comment 15

[vinayakagarwal6996]

How can I test the patch? As in to check if the bug is resolved or not?

Comment 16

[Josh Matthews [:jdm]]

Comment 2 describes steps to reproduce the error that is observed in comment 0. You should try to reproduce that without your code changes applied first.

Comment 17

[vinayakagarwal6996]

(In reply to Josh Matthews [:jdm] from comment #16)
> Comment 2 describes steps to reproduce the error that is observed in comment
> 0. You should try to reproduce that without your code changes applied first.

I am unable to reproduce the error:(
When I run the script in about:mozilla, I get this:

/*
Exception: TypeError: Components.classes is undefined
@Scratchpad/1:1:5
*/

Else, in about:memory, it runs and shows no error

Comment 18

[Bobby Holley (:bholley)]

You should be able to write an xpcshell test that does something like:

var some_non_ascii_string = ....;
var sb = new Cu.Sandbox(null, {sandboxName: some_non_ascii_string});
var location = Cu.getCompartmentLocation(sb);
do_check_true(location.indexOf(some_non_ascii_string) >= 0);

Or something like that - totally untested, but that's the approach I'd use.

Comment 19

[vinayakagarwal6996]

As per Comment 7, the about:memory error has been fixed. I am unable to reproduce the error where I can see the lossy ASCII -> UTF-8 conversions. Could you recommend a method for the same? Thanks!

Comment 20

[Bobby Holley (:bholley)]

(In reply to vinayakagarwal6996 from comment #19)
> As per Comment 7, the about:memory error has been fixed. I am unable to
> reproduce the error where I can see the lossy ASCII -> UTF-8 conversions.
> Could you recommend a method for the same? Thanks!

See comment 18. My suggestion is to create a test in js/xpconnect/tests/unit that does something along the lines of what I suggested.

Comment 21

[Rofael Aleezada [:rofael]]

Thought I'd take a hit as there hasn't been any activity in the last year.

I ran comment 18 pretty much verbatim. 

> const Cu = Components.utils;
> function run_test() {
>     // Check if a sandbox with a non-ASCII string as a title
>     // is successfully created
>     var some_non_ascii_string = "漢字";
>     var sb = new Cu.Sandbox(null, { sandboxName: some_non_ascii_string });
>     var location = Cu.getCompartmentLocation(sb);
>     Assert.ok(location.indexOf(some_non_ascii_string) >= 0);
> }

And here's the result

> run_test
> --------
> Expected PASS, got FAIL
> [run_test : 8] false == true
> [Parent]
> --------
> Expected PASS, got FAIL
> xpcshell return code: 0
>  0:01.00 LOG: MainThread INFO INFO | Result summary:
>  0:01.00 LOG: MainThread INFO INFO | Passed: 0
>  0:01.00 LOG: MainThread INFO INFO | Failed: 1
>  0:01.00 LOG: MainThread INFO INFO | Todo: 0
>  0:01.00 LOG: MainThread INFO INFO | Retried: 0
>  0:01.00 SUITE_END: MainThread
> xpcshell
> ~~~~~~~~
> Ran 2 checks (1 tests, 1 subtests)
> Expected results: 0
> Unexpected results: 2
>   test: 1 (1 fail)
>   subtest: 1 (1 fail)
> 
> Unexpected Logs
> ---------------
> js/xpconnect/tests/unit/test_bug875348.js
>   FAIL run_test
>   FAIL [Parent]

I'd like to take a knack at this bug whenever you're back.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Rofael Aleezada [:rofael] from comment #21)
> I'd like to take a knack at this bug whenever you're back.

Feel free to give it a shot - I'm back starting this coming monday.

Comment 23

[Rofael Aleezada [:rofael]]

I'm pretty sure the problem was this line: https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#1607. JS_EncodeString directly calls EncodeLatin1, and of course non ASCII characters aren't part of that alphabet.

I replaced the existing code with comment 14 (nsAutoJSString), but that didn't work either. Debugging seems to point to the conversion. With the test I wrote, the sandbox was being assigned the name "æ¼¢å¡-", the result of NS_ConvertUTF16toUTF8 on "漢字".

I'm at a roadblock here...

Comment 24

[Bobby Holley (:bholley)]

Some thoughts:
* Is your test encoded in utf8? See bug 687679.
* It's worth debugging the ParseString code and seeing where the encoding becomes incorrect. Is the original JS string garbage? Do we mess things up with the nsAutoString? Is it the NS_CovertUTF16toUTF8 at fault?

Comment 25

[Rofael Aleezada [:rofael]]

(In reply to Bobby Holley (:bholley) from comment #24)
> Some thoughts:
> * Is your test encoded in utf8? See bug 687679.

Yup, it is. UTF-8.

> * It's worth debugging the ParseString code and seeing where the encoding
> becomes incorrect. Is the original JS string garbage? Do we mess things up
> with the nsAutoString? Is it the NS_CovertUTF16toUTF8 at fault?

ParseString is returning correctly (prop.mData is set to "漢字"). I moved from Windows to Ubuntu to make sure it wasn't some weird formatting, but nope, the name of the single member of Cu.getCompartmentLocation(sb) is still the ANSI encoding "æ¼¢å­\x97". I know it's mot a weird display error, because (location.indexOf("æ¼¢å­\x97") >= 0) returns true.

Looks like the problem is further downstream. The lowest I've found so far is https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#3136, where result.mData is still correctly encoded.

This is getting... interesting...

Comment 26

[Rofael Aleezada [:rofael]]

I think I know what might be happening.

The UTF8 unit codes of "漢字" are E6, BC, A2, E5, AD, 97

"æ¼¢å­\x97" is equivalent to "\xE6\xBC\xA2\xE5\xAD\x97"

Something is interpreting each char as a Unicode character by itself, but what's causing that?

Comment 27

[Bobby Holley (:bholley)]

Nice investigative work!

(In reply to Rofael Aleezada [:rofael] from comment #25)
> Looks like the problem is further downstream. The lowest I've found so far
> is
> https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/
> XPCComponents.cpp#3136, where result.mData is still correctly encoded.
> 
> This is getting... interesting...

Ok, so this seems like it's getting stored correctly, but returning it from C++ to javascript is clobbering it.

(In reply to Rofael Aleezada [:rofael] from comment #26)
> Something is interpreting each char as a Unicode character by itself, but
> what's causing that?

That sounds to me like the C++<=>JS bindings are interpreting the bytes incorrectly. Looking at the idl definition, I see [1]. That looks suspicious, because it will take us to [2]. Note in particular the comment a few lines down.

Try changing the IDL definition at [1] to use AUTF8String instead?

[1] https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/js/xpconnect/idl/xpccomponents.idl#688
[2] https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/js/xpconnect/src/XPCConvert.cpp#305

Comment 28

[Rofael Aleezada [:rofael]]

> 0:01.83 TEST_STATUS: run_test PASS [run_test : 8] true == true
> 0:01.84 INFO (xpcshell/head.js) | test MAIN run_test finished (1)
> 0:01.85 INFO exiting test
> 0:01.87 pid:91874 [91874, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', 
> file /home/rofael/src/mozilla-central/dom/media/CubebUtils.cpp, line 394
> 0:01.87 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
> 0:02.03 pid:91874 [91874, Main Thread] WARNING: '!gThread', file /home/rofael/src/mozilla-central/xpcom/threads
> /nsTimerImpl.cpp, line 399
> 0:02.19 pid:91874 [91874, Main Thread] WARNING: OOPDeinit() without successful OOPInit(): file /home/rofael/src/mozilla-
> central/toolkit/crashreporter/nsExceptionHandler.cpp, line 3424
> 0:02.20 pid:91874 nsStringStats
> 0:02.20 pid:91874  => mAllocCount:           4473
> 0:02.20 pid:91874  => mReallocCount:           52
> 0:02.20 pid:91874  => mFreeCount:            4473
> 0:02.20 pid:91874  => mShareCount:           2160
> 0:02.20 pid:91874  => mAdoptCount:            147
> 0:02.20 pid:91874  => mAdoptFreeCount:        147
> 0:02.20 pid:91874  => Process ID: 91874, Thread ID: 140519590325952
> 0:02.21 TEST_END: Test PASS. Subtests passed 1/1. Unexpected 0
> 0:02.21 INFO INFO | Result summary:
> 0:02.21 INFO INFO | Passed: 1
> 0:02.21 INFO INFO | Failed: 0
> 0:02.21 INFO INFO | Todo: 0
> 0:02.21 INFO INFO | Retried: 0
> 0:02.21 SUITE_END

> xpcshell
> ~~~~~~~~
> Ran 2 checks (1 tests, 1 subtests)
> Expected results: 2
> OK

It works!!!!! That was the problem! I'm going to run all of the xpcshell tests on try to make sure it didn't break anything else.

Comment 29

[Bobby Holley (:bholley)]

Nice! Probably worth running more tests in addition to xpcshell.

|try: -b do -p linux64 -u all -t none| is a good way to go. If you want to use try fuzzy, I'd recommend the following filter |linux64 !qr !jsd !ccov !jittest !toolchain !stylo !jsreftest !talos !pgo !headless !accel|

Comment 30

[Rofael Aleezada [:rofael]]

(In reply to Bobby Holley (:bholley) from comment #29)
> Nice! Probably worth running more tests in addition to xpcshell.
> 
> |try: -b do -p linux64 -u all -t none| is a good way to go. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1847ee0ecf1b489072e87d218322c8c37ef82fd2

I'll go ahead and push it to try if everything works when it's done.

Comment 31

[Rofael Aleezada [:rofael]]

Attachment: Bug 875348 - Added support for non ASCII sandbox names.

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

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8945681 [details]
Bug 875348 - Added support for non ASCII sandbox names.

https://reviewboard.mozilla.org/r/215814/#review221712

Looks great! r=me with the whitespace nit fixed.

Thanks for the patch, and for the persistence in investigating the XPConnect craziness. That's just the sort attitude that makes a great Gecko hacker, so I hope you keep contributing!

::: js/xpconnect/src/Sandbox.cpp:1611
(Diff revision 1)
>  
> -    char* tmp = JS_EncodeString(mCx, value.toString());
> -    NS_ENSURE_TRUE(tmp, false);
> -    prop.Assign(tmp, strlen(tmp));
> -    js_free(tmp);
> +    nsAutoJSString strVal;
> +    if (!strVal.init(mCx, value.toString())) {
> +        return false;
> +    }
> +    

Nit: whitespace error.

Comment 33

[Rofael Aleezada [:rofael]]

Comment on attachment 8945681 [details]
Bug 875348 - Added support for non ASCII sandbox names.

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

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 35

[Andrew McCreight [:mccr8]]

The r+'d patch unfortunately appears lost to the sands of time, but it looks like the issue remains, although a few functions have been renamed in the meantime, so the code bz pointed at in comment 4 (from OptionsBase::ParseString in Sandbox.cpp) is now:
JS::UniqueChars tmp = JS_EncodeStringToLatin1(mCx, value.toString());

Comment 36

[Nika Layzell [:nika] (ni? for response)]

Given that we don't expose Sandbox to extension code anymore, I'm not sure this is a problem which we need to worry about, as we control all sandbox names used in firefox at this point, and none of them are unicode.

I believe these names are also only used for places like about:memory, so shouldn't be a big deal if they're lossy either.

If we do switch this to be unicode, we'll also need to update locations like https://searchfox.org/mozilla-central/rev/13d69189a8abfc5064fe44944550b9b6eb4544f5/js/xpconnect/idl/xpccomponents.idl#635-645 where it's read to support unicode text so that it's encoded correctly.