Avoid using JS_GetStringBytes and JS_GetStringChars internally

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 11 obsolete attachments)

70.74 KB, patch
brendan
: review+
Details | Diff | Splinter Review
68.61 KB, patch
Details | Diff | Splinter Review
71.17 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
2.93 KB, patch
brendan
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
The functions JS_GetStringBytes and JS_GetStringChars are the only string functions that do not take JSContext* while accessing runtime structures/allocating memory. It would be nice to keep the functions only for compatibility while switching all the code to there JSContext* versions. See, for example, bug 366446, where the lack of JSContext* prevents to use some memory accounting schemas.
One brute force solution: #ifdef jsapi.h so embeddings that are ready for the new cx-parameterized APIs can just use the same old names, passing cx up front.

In any event, the internal calls should all be changed ASAP to js_* counterparts, which should be fixsed to take cx, not rt (as js_GetStringBytes does) or no such arg (as js_GetStringChars does).  Igor, how about an initial patch to clean these internal calls and functions up?

/be
(Assignee)

Comment 2

11 years ago
(In reply to comment #1)
> One brute force solution: #ifdef jsapi.h so embeddings that are ready for the
> new cx-parameterized APIs can just use the same old names, passing cx up front.

That does not work since the new API can return NULL. Thus the embeddings most likely will need to update their code in any case. Thus I suggest new names + warning printouts on the first call to the deprecated function. 

> 
> In any event, the internal calls should all be changed ASAP to js_*
> counterparts, which should be fixsed to take cx, not rt (as js_GetStringBytes
> does) or no such arg (as js_GetStringChars does).  Igor, how about an initial
> patch to clean these internal calls and functions up?

It does not work like that due to passing of nulls as JSContext. I am going to finish a patch soon that just duplicates the code for JS_GetString(Chars|Bytes) while removing null-checks for the function which I call js_StringTo(Chars|Bytes).
(Assignee)

Comment 3

11 years ago
I looked at the usage of JS_GetStringBytes. The majority of cases comes from various code fragments reporting errors, debugging printouts and decompiler. In all this cases calling JS_GetStringBytes is waste of memory and time to deflate string into a cache just to copy the deflated copy into the final destination.

Perhaps it is better to replace at least error reporting cases with explicit Deflate/JS_free calls?
Depends on: 366809

Comment 4

11 years ago
Please make the new routines (if any) return const char * or const jschar *, instead of non-const?
(Assignee)

Comment 5

11 years ago
(In reply to comment #4)
> Please make the new routines (if any) return const char * or const jschar *,
> instead of non-const?

As far as I can see it is better to replace GetStringBytes for js_DeflateString/JSFree pair or similar functions using arena. In most cases where the function is used it is very unlikely that the byte cache will be reused.
Igor: errors are rare, I minimized error case code footprint by using JS_GetStringBytes (although some cases date back to when SpiderMonkey used 8-bit chars in JSStrings, most do not; I thought about getting fancier but decided it was not worth the code size for uncommon cases).  But if you can hack an even better way to report errors, please do.  E.g., with JS_C_STRINGS_ARE_UTF8, could or should we try to pass UTF-8 in C to the underlying reporter callback?

Brian's right, const-ipation is called for.  I still would hope to avoid keeping the bad old APIs undef some #ifdef, and the names are enviably clear, compared to JS_GetStringChars2 or JS_GetStringCharsForRealTakingContextFirstArg or whatever :-).

/be
/be
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)
> Igor: errors are rare, I minimized error case code footprint by using
> JS_GetStringBytes (although some cases date back to when SpiderMonkey used
> 8-bit chars in JSStrings, most do not; I thought about getting fancier but
> decided it was not worth the code size for uncommon cases).  But if you can
> hack an even better way to report errors, please do.

We can define a version of ReportError that takes an extra argument defining the type of error message arguments, which can be char*, jschar* or JString *. 
Status: NEW → ASSIGNED
(Assignee)

Comment 8

11 years ago
Created attachment 251473 [details] [diff] [review]
Using proper API internally v0

This is a work in progress to use js_GetStringBytes(cx, str) and js_GetStringChars(cx, str) everywhere in SM. In few places the patch removes the calls altogether and in other it adds checks for a null result of the functions.

The patch does not yet change the return type of functions to const char*/const jschar*.

The patch assumes that the patch from bug 366809 is applied.
(Assignee)

Comment 9

11 years ago
Created attachment 251512 [details] [diff] [review]
Using proper API internally v0.1

Compared with the previous version, the patch add const to the return type of js_GetStringBytes/js_GetStringChars.

The patch assumes that the patch from bug 366809 is applied.
(Assignee)

Comment 11

11 years ago
Created attachment 252317 [details] [diff] [review]
Using proper API v1

The patch either changes code to call js_GetString(Chars|Bytes) with cx passed instead of JS_ versions or it replaces the calls by the code directly deflating the strings.
Attachment #252317 - Flags: review?

Comment 12

11 years ago
 JS_PUBLIC_API(char *)
 JS_GetStringBytes(JSString *str)
...
 JS_PUBLIC_API(jschar *)
 JS_GetStringChars(JSString *str)
...

Igor:  Did you experiment with making these return const char * and const jschar * respectively?

+     * with explicit malloc call and ignore its errors.
+     * If we fails to convert a dependent string into an independent one, our

Should be "If we fail to convert"...

I looked over the rest and didn't see anything obvious, but I'm not really qualified to review the bulk of this patch.  I was looking more out of curiosity.  Did you mean to r? brendan (currently no reviewer set)?
(Assignee)

Comment 13

11 years ago
(In reply to comment #12)
>  JS_PUBLIC_API(char *)
>  JS_GetStringBytes(JSString *str)
> ...
>  JS_PUBLIC_API(jschar *)
>  JS_GetStringChars(JSString *str)
> ...
> 
> Igor:  Did you experiment with making these return const char * and const
> jschar * respectively?

The patch does not touch the public API. Instead it switches SM internals to use js_GetString(Bytes|Chars) that returns const strings and take cx as argument. 

> I looked over the rest and didn't see anything obvious, but I'm not really
> qualified to review the bulk of this patch.  I was looking more out of
> curiosity.  Did you mean to r? brendan (currently no reviewer set)?

I will ask Brendan for a review.
(Assignee)

Comment 14

11 years ago
Created attachment 252365 [details] [diff] [review]
Using proper API internally v1b

The is the previous patch with a comment fix.
Attachment #252365 - Flags: review?(brendan)

Updated

11 years ago
Attachment #252317 - Flags: review?
Comment on attachment 252365 [details] [diff] [review]
Using proper API internally v1b

JS_GetStringChars
    Blank line in comment between first two (virtual) paragraphs.

date_toLocaleFormat
    Use JS_ConvertArguments for smaller code.

jsgc.c
    Are the null checks for js_GetStringBytes failures worth it?  *printf will format "(null)"
    which looks as good as or better than "(error)" to me, and saves sources and compiled (latter
    not an issue since only GC_MARK_DEBUG, but source size and complexity are issues) code size.

LogCall
    Please don't change this, it'll conflict with a patch I have and it's not important.

js_Interpret
    Tracing code elaboration to format "<error>" or "<null>" seems excessive to me -- tracing is
    not working well in all cases (timeless has details) and it's only if !JS_THREADED_INTERP.
    Even if fixed, it seems wrong to add so many lines when the output need not be perfect in the
    face of OOM errors. 

js_FindIdentifierBase
    The 'if (short || long_function_name(...,
                                         ...)) {'
    style is at odds with prevailing style -- break after || and don't indent quite so much.

SprintString
    Looks good!

Decompile
    Nice, js_puts of jp2's buffer directly!

FoldXMLConstants
    Same comments as above for tracing code and GC_MARK_DEBUG complexity -- my DEBUG_brendanXXX
    hacks do not care so much about errors that they want the extra source lines.

ReportCompileErrorNumber
    rs=me, I didn't extract and re-diff with -w (if you did and it looks good, great).

js_MarkScopeProperty
    Ditto overkill on tracing/debugging code size.

DumpSubtree
    Please don't change this, it's nonsense (note the cx param is not passed in!) and I have a fix
    that will conflict.

js_GetStringBytes
    s/calls this/calls us/ in the comment.

    Later, there's a gratuitous change to indentation that I think makes the code the tinyiest bit
    possible harder to read ;-), unindenting the JSSTRING_LENGTH(str) to underhang cx, instead of
    exactly underhanging JSSTRING_CHARS(str) -- I know, prevailing style underhangs first arg, but
    this was always an exception to that rule.

js_XDRCStringAtom
    Similar underhanging nit-picking applies here.

namespace_mark_vector, xml_mark_vector
    etc. on overdoing GC_MARK_DEBUG code.

Looks good otherwise.  Passes testsuite?

/be
(Assignee)

Comment 16

11 years ago
(In reply to comment #15)
>     Even if fixed, it seems wrong to add so many lines when the output need not
> be perfect in the
>     face of OOM errors. 

With JS_C_STRINGS_ARE_UTF8 defined the result of js_GetStringBytes will be null for any string that is not UTF16. This tells me that for debugging a version of uneval that does not print "" around the string would be a better option then js_GetStringBytes. 



> 
> js_FindIdentifierBase
>     The 'if (short || long_function_name(...,
>                                          ...)) {'
>     style is at odds with prevailing style -- break after || and don't indent
> quite so much.
> 
> SprintString
>     Looks good!
> 
> Decompile
>     Nice, js_puts of jp2's buffer directly!
> 
> FoldXMLConstants
>     Same comments as above for tracing code and GC_MARK_DEBUG complexity -- my
> DEBUG_brendanXXX
>     hacks do not care so much about errors that they want the extra source
> lines.
> 
> ReportCompileErrorNumber
>     rs=me, I didn't extract and re-diff with -w (if you did and it looks good,
> great).
> 
> js_MarkScopeProperty
>     Ditto overkill on tracing/debugging code size.
> 
> DumpSubtree
>     Please don't change this, it's nonsense (note the cx param is not passed
> in!) and I have a fix
>     that will conflict.
> 
> js_GetStringBytes
>     s/calls this/calls us/ in the comment.
> 
>     Later, there's a gratuitous change to indentation that I think makes the
> code the tinyiest bit
>     possible harder to read ;-), unindenting the JSSTRING_LENGTH(str) to
> underhang cx, instead of
>     exactly underhanging JSSTRING_CHARS(str) -- I know, prevailing style
> underhangs first arg, but
>     this was always an exception to that rule.
> 
> js_XDRCStringAtom
>     Similar underhanging nit-picking applies here.
> 
> namespace_mark_vector, xml_mark_vector
>     etc. on overdoing GC_MARK_DEBUG code.
> 
> Looks good otherwise.  Passes testsuite?
> 
> /be
> 

(In reply to comment #16)
> (In reply to comment #15)
> >     Even if fixed, it seems wrong to add so many lines when the output need not
> > be perfect in the
> >     face of OOM errors. 
> 
> With JS_C_STRINGS_ARE_UTF8 defined the result of js_GetStringBytes will be null
> for any string that is not UTF16. This tells me that for debugging a version of
> uneval that does not print "" around the string would be a better option then
> js_GetStringBytes. 

Hmm, that seems like a bug in js_GetStringBytes for JS_C_STRINGS_ARE_UTF8 -- but I am confused by what you meant by "not UTF16".  What JS strings cause js_GetStringBytes compiled with JS_C_STRINGS_ARE_UTF8 defined to return null?

/be
(Assignee)

Comment 21

11 years ago
(In reply to comment #18)
> Hmm, that seems like a bug in js_GetStringBytes for JS_C_STRINGS_ARE_UTF8 --
> but I am confused by what you meant by "not UTF16".  What JS strings cause
> js_GetStringBytes compiled with JS_C_STRINGS_ARE_UTF8 defined to return null?

Here is the code from js_DeflateStringToBuffer with JS_C_STRINGS_ARE_UTF8 defined:

        if ((c >= 0xDC00) && (c <= 0xDFFF))
            goto badSurrogate;

That is, js_DeflateStringToBuffer treats chars as UTF-16-encoded sequence of Unicode characters. The encoding uses the surrogate pairs and when the surrogate is invalid, the function returns null. 

To avoid that I will later attach a patch that adds:

extern size_t
js_PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *f,
                        JSString *str, uint32 quote);

/*
 * Write str into buffer escaping any non-printable or non-ASCII character.
 * Guarantees that a NUL is at the end of the buffer. Returns the length of
 * the written output, NOT including the NUL.
 * If buffer is null, just returns the length of the output.
 * If quote is not 0, it must be a single or double quote character that will
 * quote the output.  
*/
#define js_PutEscapedString(buffer, bufferSize, str, quote)                   \
    js_PutEscapedStringImpl(buffer, bufferSize, NULL, str, quote)

#define js_FileEscapedString(file, str, quote)                                \
    (JS_ASSERT(file), js_PutEscapedStringImpl(NULL, 0, file, str, quote))

The functions never fail and always escape non-ascii. IMO this is more useful for a debug output.
(Assignee)

Comment 22

11 years ago
Created attachment 252768 [details] [diff] [review]
Implementation v2

Changes compared with the previous version:

1. To avoid GetStringBytes I added debug-only js_PutEscapedString and js_FileEscapedString that writes escaped strings to a string buffer/file. The function never fails and avoids introducing new GC-objects. Moreover, it reduced the source complexity of the tracing/debugging code compared with the current version.

2. I added 
extern size_t
js_GetDeflatedStringLength(JSContext *cx, const jschar *chars,
                           size_t charsLength);
since using js_DeflateStringToBuffer is very inconvenient when one wants just to calculate the required buffer size.
Attachment #252768 - Flags: review?(brendan)
(Assignee)

Comment 23

11 years ago
Created attachment 252770 [details] [diff] [review]
Implementation v2 (diff -b)
Comment on attachment 252768 [details] [diff] [review]
Implementation v2

Sorry, thought I reviewed this. Looks good, but the patch is out of date. Can you update a fresh one, and a diff -w one (no need to request review on that, I'll read it and stamp the full patch).

/be
Attachment #252768 - Flags: review?(brendan)

Updated

11 years ago
Attachment #252365 - Flags: review?(brendan)
(Assignee)

Comment 25

11 years ago
Created attachment 255337 [details] [diff] [review]
Implementation v3

Here is a patch update to sync with CVS head.
Attachment #251473 - Attachment is obsolete: true
Attachment #251512 - Attachment is obsolete: true
Attachment #252317 - Attachment is obsolete: true
Attachment #252365 - Attachment is obsolete: true
Attachment #252768 - Attachment is obsolete: true
Attachment #252770 - Attachment is obsolete: true
Attachment #255337 - Flags: review?(brendan)
(Assignee)

Comment 26

11 years ago
Created attachment 255338 [details] [diff] [review]
Implementation v3 for real

I should write on the wall: check the content of the patch before attaching one.
Attachment #255337 - Attachment is obsolete: true
Attachment #255338 - Flags: review?(brendan)
Attachment #255337 - Flags: review?(brendan)
(Assignee)

Comment 27

11 years ago
Created attachment 255339 [details] [diff] [review]
Implementation v3 (diff -w)
(In reply to comment #27)
> Created an attachment (id=255339) [details]
> Implementation v3 (diff -w)

Set patch flag? Or is this really application/octet-stream?

/be

(Assignee)

Comment 29

11 years ago
Comment on attachment 255339 [details] [diff] [review]
Implementation v3 (diff -w)

This is a patch
Attachment #255339 - Attachment is patch: true
Attachment #255339 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 255338 [details] [diff] [review]
Implementation v3 for real


>         /* let PRMJTime format it.       */
>-        result_len = PRMJ_FormatTime(buf, sizeof buf, format, &split);
>+        result_len = PRMJ_FormatTime(buf, sizeof buf, (char *)format, &split);

Sigh, we need a FIXME: citing a bug on prmjtime.c -- it needs constipation and probably other love.

> static void
> DumpSubtree(JSScopeProperty *sprop, int level, FILE *fp)
> {
>-    char buf[10];
>+    JSString *str;
>     JSScopeProperty *kids, *kid;
>     PropTreeKidsChunk *chunk;
>     uintN i;
> 
>-    fprintf(fp, "%*sid %s g/s %p/%p slot %lu attrs %x flags %x shortid %d\n",
>-            level, "",
>-            JSID_IS_ATOM(sprop->id)
>-            ? JS_GetStringBytes(ATOM_TO_STRING(JSID_TO_ATOM(sprop->id)))
>-            : JSID_IS_OBJECT(sprop->id)
>-            ? js_ValueToPrintableString(cx, OBJECT_JSID_TO_JSVAL(sprop->id))
>-            : (JS_snprintf(buf, sizeof buf, "%ld", JSVAL_TO_INT(sprop->id)),
>-               buf)
>+    fprintf(fp, "%*sid ", level, "");
>+    if (JSID_IS_ATOM(sprop->id)) {
>+        str =  ATOM_TO_STRING(JSID_TO_ATOM(sprop->id));
>+    } else if (JSID_IS_OBJECT(sprop->id)) {
>+        str = js_ValueToString(cx, OBJECT_JSID_TO_JSVAL(sprop->id));

Oops, still a broken dependency on cx here (note no such argument to DumpSubtree or its callers all the way back to js_GC -- could fix that two ways: add cx param from js_GC; remove cx dep here.

>+#if defined(DEBUG) ||                                                         \
>+    defined(GC_MARK_DEBUG) ||                                                 \
>+    defined(DUMP_CALL_TABLE) ||                                               \
>+    defined(DUMP_SCOPE_STATS)
>+size_t
>+js_PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *file,
>+                        JSString *str, uint32 quote)
>+{
>+    jschar *chars, *charsEnd;
>+    size_t n;
>+    char *charEscape;
>+    char c;
>+    uintN u, hex, shift;
>+    enum {
>+        STOP, FIRST_QUOTE, LAST_QUOTE, CHARS, ESCAPE_START, ESCAPE_MORE
>+    } state;
>+
>+    static char oneLetterEscapes[] =        "\b" "\f" "\n" "\r" "\t" "\v";
>+    static char oneLetterEscapesMapped[] =  "b"  "f"  "n"  "r"  "t"  "v";

Try to use js_EscapeMap (jsopcode.[ch]).

> /*
>  * Inflate bytes to JS chars into a buffer.
>  * 'chars' must be large enough for 'length' jschars.
>  * The buffer is NOT null-terminated.
>- * cx may be NULL, which means no errors are thrown.
>  * The destination length needs to be initialized with the buffer size, takes
>  * the number of chars moved.

May as well reformat this comment to wrap better while you are here.

>+/*
>  * Deflate JS chars to bytes into a buffer.
>  * 'bytes' must be large enough for 'length chars.
>  * The buffer is NOT null-terminated.
>- * cx may be NULL, which means no errors are thrown.
>  * The destination length needs to be initialized with the buffer size, takes
>  * the number of bytes moved.

Ditto.

>+/*
>+ * Write str into buffer escaping any non-printable or non-ASCII character.
>+ * Guarantees that a NUL is at the end of the buffer. Returns the length of
>+ * the written output, NOT including the NUL.
>+ * If buffer is null, just returns the length of the output.
>+ * If quote is not 0, it must be a single or double quote character that will
>+ * quote the output.

Or, if you prefer, put an extra blank comment line (spaces *\n) between paragraphs, here and below (and above in the mdaumli comments). Main thing I'm whining about is ragged right lines clumped together. Either the sentences should wrap into one paragraph, or if separate paras should be separated by "blank" line.

>+ *
>+ * The function is only defined for debug builds.
>+*/
>+#define js_PutEscapedString(buffer, bufferSize, str, quote)                   \
>+    js_PutEscapedStringImpl(buffer, bufferSize, NULL, str, quote)
>+
>+/*
>+ * Write str into file escaping any non-printable or non-ASCII character.
>+ * Returns the number of bytes written fto file.

"fto" typo and reformat per above.

>+ * If quote is not 0, it must be a single or double quote character that will
>+ * quote the output.
>+ *
>+ * The function is only defined for debug builds.
>+*/
>+#define js_FileEscapedString(file, str, quote)                                \
>+    (JS_ASSERT(file), js_PutEscapedStringImpl(NULL, 0, file, str, quote))
>+
> JS_END_EXTERN_C
> 
> #endif /* jsstr_h___ */

>     if (xdr->mode == JSXDR_ENCODE) {
>         JS_ASSERT(ATOM_IS_STRING(*atomp));
>-        bytes = JS_GetStringBytes(ATOM_TO_STRING(*atomp));
>-        return JS_XDRCString(xdr, &bytes);
>+        str = ATOM_TO_STRING(*atomp);
>+        bytes = js_DeflateString(xdr->cx,
>+                                 JSSTRING_CHARS(str),
>+                                 JSSTRING_LENGTH(str));
>+        if (!bytes)
>+            return JS_FALSE;
>+        ok = JS_XDRCString(xdr, &bytes);
>+        JS_free(xdr->cx, bytes);
>+        return ok;

Crazy: I forgot that the XDR code deflates strings. FIXME citing new bug, or is one on file already?

>+            n = (ns->prefix)
>+                ? js_PutEscapedString(ns->prefix, 0, buf, sizeof buf)
>+                : 0;

Nit: could avoid parenthesizing high-precedence ? condition expression.

/be
Attachment #255338 - Flags: review?(brendan) → review-
(Assignee)

Comment 31

11 years ago
(In reply to comment #30)
> (From update of attachment 255338 [details] [diff] [review])
> 
> >         /* let PRMJTime format it.       */
> >-        result_len = PRMJ_FormatTime(buf, sizeof buf, format, &split);
> >+        result_len = PRMJ_FormatTime(buf, sizeof buf, (char *)format, &split);
> 
> Sigh, we need a FIXME: citing a bug on prmjtime.c -- it needs constipation and
> probably other love.

Since PR_FormatTime in NSPR already got const, http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/misc/prtime.c#1675 ,I will add it to PRMJ_FormatTime within this patch.
(Assignee)

Comment 32

11 years ago
Created attachment 255483 [details] [diff] [review]
Implementation v4

This version fixes the nits (accept FIXME for js_XDRCStringAtom as it already got one at the function start), makes sure that SM compiles with -DDUMP_SCOPE_STATS -DGC_MARK_DEBUG -DDUMP_CALL_TABLE (previously I used the wrong order of arguments  in few places when calling js_PutEscapedString) and fixes GC_MARK_DEBUG crashes when dumping XML: now the patch do not assume that prefix is non null in Namespace/QName.
Attachment #255338 - Attachment is obsolete: true
Attachment #255339 - Attachment is obsolete: true
Attachment #255483 - Flags: review?(brendan)
(Assignee)

Comment 33

11 years ago
Created attachment 255484 [details] [diff] [review]
Implementation v4 (diff -w)
Comment on attachment 255483 [details] [diff] [review]
Implementation v4

+const char js_EscapeMap[2][10] = {
+    { '\b', '\f', '\n', '\r', '\t', '\v', '"', '\'', '\\', '\0' },
+    { 'b',  'f',  'n',  'r',  't',  'v',  '"', '\'', '\\', '\0' }
 };

Does this really improve code size, runtime, or readability? I'm thinking not.

         /* Use js_EscapeMap, \u, or \x only if necessary. */
-        if ((u = js_strchr(js_EscapeMap, c)) != NULL) {
+        if (!(c >> 8) && (e = strchr(js_EscapeMap[0], (int)c)) != NULL) {

Most input chars will make the first test true, at least in this hemisphere.

             ok = dontEscape
                  ? Sprint(sp, "%c", (char)c) >= 0
-                 : Sprint(sp, "\\%c", (char)u[1]) >= 0;
+                 : Sprint(sp, "\\%c",
+                          js_EscapeMap[1][e - js_EscapeMap[0]]) >= 0;

u[1] is also faster and less code than the new version.

@@ -4006,16 +4006,12 @@
     if (argc == 0) {
         str = cx->regExpStatics.input;
         if (!str) {
-            const char *bytes = js_GetStringBytes(cx, re->source);
-
-            if (bytes) {
-                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                                     JSMSG_NO_INPUT,
-                                     bytes,
-                                     (re->flags & JSREG_GLOB) ? "g" : "",
-                                     (re->flags & JSREG_FOLD) ? "i" : "",
-                                     (re->flags & JSREG_MULTILINE) ? "m" : "");
-            }
+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
+                                 JSMSG_NO_INPUT,
+                                 JS_GetStringBytes(re->source),
+                                 (re->flags & JSREG_GLOB) ? "g" : "",
+                                 (re->flags & JSREG_FOLD) ? "i" : "",
+                                 (re->flags & JSREG_MULTILINE) ? "m" : "");
             ok = JS_FALSE;
             goto out;
         }

This hunk goes back to JS_GetStringBytes, saving code for an error case but possibly using "" on OOM for the regexp source in the error message.

@@ -4032,12 +4028,16 @@
     if (argc == 0) {
         str = cx->regExpStatics.input;
         if (!str) {
-            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                                 JSMSG_NO_INPUT,
-                                 JS_GetStringBytes(re->source),
-                                 (re->flags & JSREG_GLOB) ? "g" : "",
-                                 (re->flags & JSREG_FOLD) ? "i" : "",
-                                 (re->flags & JSREG_MULTILINE) ? "m" : "");
+            const char *bytes = js_GetStringBytes(cx, re->source);
+
+            if (bytes) {
+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
+                                     JSMSG_NO_INPUT,
+                                     bytes,
+                                     (re->flags & JSREG_GLOB) ? "g" : "",
+                                     (re->flags & JSREG_FOLD) ? "i" : "",
+                                     (re->flags & JSREG_MULTILINE) ? "m" : "");
+            }
             ok = JS_FALSE;
             goto out;
         }

This goes the other way. Why the difference?

+ * Inflate bytes to JS chars into a buffer. 'chars' must be large enough for
+ * 'length' jschars. The buffer is NOT null-terminated. The destination length
+ * needs to be initialized with the buffer size, takes the number of chars
+ * moved.

Need "and" or something like it to like the first clause in the last sentence with the "takes the number of chars moved."

+ * Deflate JS chars to bytes into a buffer. 'bytes' must be large enough for
+ * 'length chars. The buffer is NOT null-terminated. The destination length
+ * needs to be initialized with the buffer size, takes the number of bytes
+ * moved.

Ditto.

+extern size_t
+js_PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *f,

Nit: canonical name is fp, not f.

/be
(Assignee)

Comment 35

11 years ago
Created attachment 257239 [details] [diff] [review]
Implementation v5

Addressing the nits. The new version still changes js_EscapeMap, but this time it just converts it from jschar to char so a compiler-optimized strchr can be used for the search.
Attachment #255483 - Attachment is obsolete: true
Attachment #255484 - Attachment is obsolete: true
Attachment #257239 - Flags: review?(brendan)
Attachment #255483 - Flags: review?(brendan)
(Assignee)

Comment 36

11 years ago
Created attachment 257240 [details] [diff] [review]
Implementation v5 (diff -w)
Comment on attachment 257239 [details] [diff] [review]
Implementation v5

Sorry, I forgot to stamp this!

/be
Attachment #257239 - Flags: review?(brendan) → review+
(Assignee)

Comment 38

11 years ago
Changing the title to reflect the nature of the patch: it does not deprecate JS_GetString(Bytes|Chars). Rather it makes sure that the functions are not used internally. Deprecating would be done in a separated patch. 
(Assignee)

Updated

11 years ago
Summary: Deprecate JS_GetStringBytes and JS_GetStringChars → Avoid using JS_GetStringBytes and JS_GetStringChars internally
(Assignee)

Comment 39

11 years ago
I committed the patch from comment 35 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.309; previous revision: 3.308
done
Checking in jsdate.c;
/cvsroot/mozilla/js/src/jsdate.c,v  <--  jsdate.c
new revision: 3.84; previous revision: 3.83
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.82; previous revision: 3.81
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.177; previous revision: 3.176
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.205; previous revision: 3.204
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.338; previous revision: 3.337
done
Checking in jsnum.c;
/cvsroot/mozilla/js/src/jsnum.c,v  <--  jsnum.c
new revision: 3.78; previous revision: 3.77
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.332; previous revision: 3.331
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.212; previous revision: 3.211
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.43; previous revision: 3.42
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.270; previous revision: 3.269
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.135; previous revision: 3.134
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.121; previous revision: 3.120
done
Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.58; previous revision: 3.57
done
Checking in jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.43; previous revision: 3.42
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.138; previous revision: 3.137
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.139; previous revision: 3.138
done
Checking in jsstr.h;
/cvsroot/mozilla/js/src/jsstr.h,v  <--  jsstr.h
new revision: 3.35; previous revision: 3.34
done
Checking in jsxdrapi.c;
/cvsroot/mozilla/js/src/jsxdrapi.c,v  <--  jsxdrapi.c
new revision: 1.31; previous revision: 1.30
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.146; previous revision: 3.145
done
Checking in prmjtime.c;
/cvsroot/mozilla/js/src/prmjtime.c,v  <--  prmjtime.c
new revision: 3.59; previous revision: 3.58
done
Checking in prmjtime.h;
/cvsroot/mozilla/js/src/prmjtime.h,v  <--  prmjtime.h
new revision: 3.16; previous revision: 3.15
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 373152
c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax error : missing ';' before 'else'

Comment 41

11 years ago
(In reply to comment #40)
> c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax
> error : missing ';' before 'else'
> 

~/firefox/mozilla/js/src> cvs commit -m "fix for bug 366725 missed a semicolon and broke msvc 2005 express for some people." jsinterp.c
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.339; previous revision: 3.338
done
(Assignee)

Comment 42

11 years ago
(In reply to comment #41)
> (In reply to comment #40)
> > c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax
> > error : missing ';' before 'else'
> > 
> 
> ~/firefox/mozilla/js/src> cvs commit -m "fix for bug 366725 missed a semicolon
> and broke msvc 2005 express for some people." jsinterp.c
> Checking in jsinterp.c;
> /cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
> new revision: 3.339; previous revision: 3.338
> done

Thanks! 

(Assignee)

Comment 43

11 years ago
Created attachment 257799 [details] [diff] [review]
Implementation v5 + build fix

This the committed patch + build fix for references. I forgot to test the patch with -DJS_THREADED_INTERP=0.
Attachment #257799 - Flags: review+
(Assignee)

Updated

11 years ago
Blocks: 373160

Updated

11 years ago
Flags: in-testsuite-

Comment 44

11 years ago
With JS_C_STRINGS_ARE_UTF8 defined, js shell fails to build

jsstr.c:2841: error: too few arguments to function ‘js_GetDeflatedStringLength’
(Assignee)

Comment 45

11 years ago
(In reply to comment #44)
> With JS_C_STRINGS_ARE_UTF8 defined, js shell fails to build
> 
> jsstr.c:2841: error: too few arguments to function
> ‘js_GetDeflatedStringLength’
> 

I will fix this later today.
(Assignee)

Comment 46

11 years ago
Created attachment 259933 [details] [diff] [review]
Fixing JS_C_STRINGS_ARE_UTF8 issues

The patch makes things compile and fixes couple of warnings due char versus unsigned char madness. I just wish signed chars would never exist...
Attachment #259933 - Flags: review?(brendan)
Comment on attachment 259933 [details] [diff] [review]
Fixing JS_C_STRINGS_ARE_UTF8 issues

r=me, sorry for the tardy review.

/be
Attachment #259933 - Flags: review?(brendan) → review+
(Assignee)

Comment 48

11 years ago
i committed the patch from comment 46 to the trunk to fix JS_C_STRINGS_ARE_UTF8 compilation:

Checking in jsprf.c;
/cvsroot/mozilla/js/src/jsprf.c,v  <--  jsprf.c
new revision: 3.23; previous revision: 3.22
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.140; previous revision: 3.139
done
(Assignee)

Updated

11 years ago
Depends on: 378789
Blocks: 339721
You need to log in before you can comment on or make changes to this bug.