js> "\x001".toSource() (new String("\01")) js> eval("\x001".toSource()) == "\x001" false js> eval("\x001".toSource()).toSource() (new String("\x01")) Representation of '\0' as "\0" in strings can conflict with octal escape sequences, so it should be better "\x00", unless it can be prooven that no octal digit follows.

This is a regression; shell from the 1.8 branch does not suffer from this bug.

Regression from bug 366725, perhaps?

(In reply to comment #2) > Regression from bug 366725, perhaps? Looking at the patch, you're probably right: js_strchr and strchr are incompatible. strchr will find the terminating '\0' while js_strchr will not. Thus, using js_EscapeMap with strchr instead of js_strchr will change behavior. And only '\0' seems to be affected, not '\x01'.

Comment on attachment 318880 [details] [diff] [review] a little more >-const char js_EscapeMap[] = { >+const jschar js_EscapeMap[] = { ... >- if (!(c >> 8) && (e = strchr(js_EscapeMap, (int)c)) != NULL) { >+ if (!(c >> 8) && (e = js_strchr(js_EscapeMap, c)) != NULL) { This is not necessary. A simpler fix is to remove the last '0' from the array and use memchr(js_EscapeMap, c, sizeof(js_EscapeMap) - 1), not strchr, with comments that memchr is used to avoid matching the case of c == '\0'.

Good idea. Coming up...

Actually, it seems like other references to js_EscapeMap may be broken by the absence of the \0 (as they might have been broken by my patch).

(In reply to comment #9) > Actually, it seems like other references to js_EscapeMap may be broken by the > absence of the \0 (as they might have been broken by my patch). I have not suggested to remove \0, I only suggested to remove the trailing '0'! This is compatible with the usage of js_EscapeMap in jsstr.c since there the code explicitly checks for '\0'. Less minimal solution would be to change js_EscapeMap into: extern const char js_EscapeMap[]; #define JS_ESCAPE_MAP_LEN 9 const char js_EscapeMap[] = "\b" "\f" ... "\\" "b" "f" ... "\\"; JS_STATIC_ASSERT(JS_ESCAPE_MAP_LEN * 2 + 1 == sizeof(js_EscapeMap)); and then use it in jsopcode.c and jsstr.c like in: if (... (e = memchr(js_EscapeMap, c, JS_ESCAPE_MAP_LEN)) != NULL) { ... e[JS_ESCAPE_MAP_LEN]. }

Mass unowning JS bugs... I'm not likely to be able to move these forward.

CC Brendan, i think he did some related change recently.

I think decompiling to \0 is always confusing. This patches always decompiles to \x00, and by the way fixes this bug.

Forgot to refresh.

Backed out because of test failures on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/04505e53439e

http://hg.mozilla.org/integration/mozilla-inbound/rev/edade842d1a8 Add back terminating \0 http://hg.mozilla.org/integration/mozilla-inbound/rev/43982bdb73dd

https://hg.mozilla.org/mozilla-central/rev/edade842d1a8 https://hg.mozilla.org/mozilla-central/rev/43982bdb73dd please specify the bug number in follow-up patches.