830500 - Need way to convert two-byte characters to jsid outside the engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

In the XBL code, I have a field name as a PRUnichar*, and I need it as an id. I can make it a JSString, but there's no way to convert that to a jsid right now without interning it. This would leak the string for the lifetime of the runtime, which we definitely don't want.

Patch coming up.

Comment 1

[Bobby Holley (:bholley)]

Waldo points out that you can do PRUnichar* -> JSString -> JSVal -> JS_ValueToId. But I think a direct API is probably better.

Comment 2

[Jeff Walden [:Waldo]]

Indeed.  And PRUnichar* -> JSString probably gives you a flat or linear string, which would make JS_ValueToId slower than it ideally would be.  jschar* -> jsid directly means we can internally do jschar* -> JSAtom* -> jsid, which is much more efficient (only one string created, not two).

Watch out for jschar* that contain an integer-valued jsid.  I'm not sure offhand what's the best sequence of intermediate calls to handle that possibility, but it does have to be addressed right now.  :-\  Or we could assert that the jschar* never would convert to an integer-valued jsid; I dunno if your use case permits that sort of assertion or not.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Watch out for jschar* that contain an integer-valued jsid.  I'm not sure
> offhand what's the best sequence of intermediate calls to handle that
> possibility, but it does have to be addressed right now.  :-\  Or we could
> assert that the jschar* never would convert to an integer-valued jsid; I
> dunno if your use case permits that sort of assertion or not.

I don't know what that's about. Can you explain? I'm just trying to convert a regular old UTF16 string to a jsid.

Comment 4

[Jeff Walden [:Waldo]]

In the general run of affairs there are two different kinds of jsid: one that contains certain kinds of integers, and one that contains atomized strings.  (There are a few more for E4X now that we might use for ES6 stuff eventually.)  If your characters don't spell out a number, then the correct jsid representation is through an atomized string.  If they do spell out a certain kind of number, then the correct jsid representation is the integer form.  You can determine if an integer matches the integer form using INT_FITS_IN_JSID(int32_t).

This means the first thing you'd want to do is test the characters to see if they contain such an integer.  If they do, you return the integer-id kind.  Otherwise you create a PropertyName* and return NameToId(propname).  But if you can require that your chars never contain such an integer, you could straightaway create the PropertyName* and jsid for it.  I don't know if your use case would let you say that no such integers would ever flow through or not.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Implement JS_StringToId. v1

Hm. Well, <field>s probably don't have integer names (though I'd need to
double-check the XBL spec), but it seems like if this is an API method it should
handle the general case, right? So where do I check this? Before the call to
AtomizeChars? And how do I do the requisite atoi?

Comment 6

[Steve Fink [:sfink] [:s:]]

If I am interpreting Mr. Waldo's comments correctly, I believe it would look something like

  JS_FunkyStringToId(jschar *chars, uint32_t len) {
    if (len == 0)
      return JSID_EMPTY;

    double d;
    int32_t i;
    if (js_strtod(chars, chars + len, &end, &d) && end == chars + len && MOZ_DOUBLE_IS_INT32(d, &i))
     return INT_TO_JSID(i);

    return NON_INTEGER_ATOM_TO_JSID(AtomizeChars(chars, len));
  }

Maybe. r?Waldo

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 702002 [details] [diff] [review]
Implement JS_StringToId. v1

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

The general case is whatever the API defines it as.  Because we're moving toward bifurcating uint32_t and non-uint32_t accesses, it probably makes sense to require that the string not contain a uint32_t, and not the slightly-different case of not containing an integer id.  Thus in terms of the created-property space this covers, it covers the complement of JS_IndexToId.

But I guess if it's covering non-uint32_t, then trying to optimize this into always creating an atom-valued jsid is impossible and/or misguided.  :-(  So I think something like this is probably the most reasonable thing to do:

JS_PUBLIC_API(JSBool)
JS_CharsToId(JSContext* cx, JS::TwoByteChars chars, jsid *idp)
{
    RootedAtom atom(cx, AtomizeChars(cx, chars.start(), chars.length()));
    if (!atom)
        return false;
#ifdef DEBUG
    uint32_t dummy;
    MOZ_ASSERT(!atom->isIndex(&dummy));
#endif
    *idp = AtomToId(atom);
    return true;
}

::: js/src/jsapi.cpp
@@ +7079,5 @@
>      return IndexToId(cx, index, id);
>  }
>  
>  JS_PUBLIC_API(JSBool)
> +JS_StringToId(JSContext* cx, const jschar *str, jsid *idp)

I'd name this JS_CharsToId, as String would suggest JSString* to me.  Also note that the prototype in jsapi.h doesn't match this!  And now that bug 811060 has landed, this should take a JS::TwoByteChars.

Comment 8

[Boris Zbarsky [:bzbarsky]]

We used to have a CheckForStringIndex thing... is that dead already?

Comment 9

[Jeff Walden [:Waldo]]

Yes, bhackett removed the need for it.  Now if you have a jsid, it's appropriately canonicalized.  Which, hm, come to think of it, might have meant there wasn't anything to worry about re getting the right jsid kind.  :-\

Sigh, jsid is a mess.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 702002 [details] [diff] [review]
Implement JS_StringToId. v1

Flagging for review given comment 9.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 702002 [details] [diff] [review]
Implement JS_StringToId. v1

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

Comment 7.  :-)

Comment 12

[Bobby Holley (:bholley)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Which, hm, come to think of it, might have
> meant there wasn't anything to worry about re getting the right jsid kind. 

I took this to mean that comment 7 was obsolete. If that's not true, I can implement comment 7.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Implement JS_CharsToId. v2

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 705836 [details] [diff] [review]
Implement JS_CharsToId. v2

Sorry for the lag, here.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 705836 [details] [diff] [review]
Implement JS_CharsToId. v2

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

::: js/src/jsapi.h
@@ +6162,5 @@
>  extern JS_PUBLIC_API(JSBool)
>  JS_IndexToId(JSContext *cx, uint32_t index, jsid *id);
>  
>  /*
> + * Convert chars into a jsid.

Please add a note that the chars can't be an index.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 705836 [details] [diff] [review]
Implement JS_CharsToId. v2

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

::: js/src/jsapi.cpp
@@ +7082,5 @@
>      return true;
>  }
>  
>  JS_PUBLIC_API(JSBool)
> +JS_CharsToId(JSContext* cx, JS::TwoByteChars chars, jsid *idp)

JS::MutableHandleId for the last parameter, these days.

@@ +7089,5 @@
> +    if (!atom)
> +        return false;
> +#ifdef DEBUG
> +    uint32_t dummy;
> +    MOZ_ASSERT(!atom->isIndex(&dummy));

Add a second argument "API misuse: |chars| must not encode an index" to explain exactly what's wrong for anyone who hits this.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> >  JS_PUBLIC_API(JSBool)
> > +JS_CharsToId(JSContext* cx, JS::TwoByteChars chars, jsid *idp)
> 
> JS::MutableHandleId for the last parameter, these days.

I was under the impression per bug 781070 comment 5 and bug 781070 comment 7 that we were explicitly not using the Handle API in JSAPI except for callbacks. Can you clarify whether that has changed?

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, we don't want that to be a handle right now.

Comment 19

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3985cb4da6b6

Comment 20

[Ed Morley [:emorley]]

Backed out for compile failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/436d46763d62

Comment 21

[Bobby Holley (:bholley)]

Oh, for crying out loud - the signature of AtomizeChars changed a few days ago to accept a new template parameter, which is why this bustage didn't show up for me locally or on try. :-(

Comment 22

[Bobby Holley (:bholley)]

Fixed and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9cc81bbc67

The build bustage was uniform (a missing template parameter for a function whose call signature changed since my last rebase), that appeared locally for me when I pulled and rebased. So I just made sure it built locally and pushed. Hopefully this is enough.

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/2f9cc81bbc67