607292 - Eliminating JS_GetStringBytes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

See bug 606065 comment 27

Comment 1

[Brendan Eich [:brendan]]

Is this blocking bug 606065 then? I made some comments there after 27.

/be

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Is this blocking bug 606065 then? I made some comments there after 27.

It looks like the usage of JS_GetStringBytes and frends can be eliminated from FF. From about 220 cases about 100 can be replaced with using jschar or id versions of JS API. Then there are various debug dumping. Those should use js_PutEscapedString/js_FileEscapedString for the added benefit of reliably ouputing non-ascii chars.

The vast majority of the remaining cases can be trivially served with autoclasses or via extending JS_printf and friends to accept something like %S to print JS strings directly.

The only hard case that I find is XPCNativeInterface::GetMemberName that return the result of JS_GetStringBytes. But that can replaced with storing the bytes in XPCNativeInterface.

So yes, this should block 606065.

Comment 3

[Igor Bukanov]

I forgot to mention it is harder to deal with JS_GetFunctionName as the debugger API returns its result. But function names are atoms and the corresponding bytes can be stored in the atom table directly. Or, not to bloat that table, a simplified cache can be used tailored for atoms.

Comment 4

[Andreas Gal :gal]

I did a quick run-through last weekend and replaced all the 220 cases. Some were hacked up a bit. We can easily modify the debugger API, its mostly used from script. Lets not add any complex cache for the atom table. And jsd2 is coming anyway. And no, this can't block 606065. 606065 has to be fixed within a week. We can't take a dependency on this, as much as I would love to see the cache gone.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> I did a quick run-through last weekend and replaced all the 220 cases.

What about reviewing and landing it?

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> 606065 has to be fixed within a week.

I should have a patch for bug 607695 tested tomorrow and the rest of code eliminating GetStringBytes over weekend. Is it OK?

Comment 7

[Andreas Gal :gal]

Sure, if we can get something landed that quickly we should definitely take the GetStringBytes elimination.

Comment 8

[Igor Bukanov]

Attachment: wip

The patch eliminates JS_GetStringBytes in most of the places. This was rather more involved than I initially anticipated as I missed various js_AtomToPrintableString. But now the difficult part is done and what remains is to convert about 60 or so easy cases of JS_GetStringBytes callers.

Comment 9

[Andreas Gal :gal]

Igor, can you split out the uncontroversial parts into a separate bug (where you just use existing ById APIs). Thats a super easy review we can do right now.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Igor, can you split out the uncontroversial parts into a separate bug (where
> you just use existing ById APIs). Thats a super easy review we can do right
> now.

The changes in the patch here explicitly focus on unavoidable string->bytes conversions and replacing JS_GetStringBytes with a helper class build around JS_EncodeString(). The changes where string->bytes can be avoided are in the bug 607695. There the vast majority of cases are based on new API like JS_MatchStringAndAscii, JS_DefineFunctionById. I do not think that splitting the patch in that bug would be helpful.

Comment 11

[Igor Bukanov]

Attachment: wip

This converts almost all JS_GetStringBytes users. The remaining case is XPCConvert::JSData2Native, http://hg.mozilla.org/tracemonkey/file/b71e965f4093/js/src/xpconnect/src/xpcconvert.cpp#l515 , that returns JS_GetStringBytes to the caller and is used in a few places in XPConnect.

Comment 12

[Igor Bukanov]

Attachment: v1

This eliminates remaining JS_GetStringBytes users that are left from the bug 610198. I will ask for a review after landing that bug.

The patch still keeps JS_GetFunctionName used by the debugger interface. I will remove that in another bug, but for now the changes allows to keep the deflated hash only for atoms.

Comment 13

[Igor Bukanov]

Attachment: v2

This passes the tests. The patch essence is:

1. Eliminating JS_GetStringBytes and 's' support from JS_ConvertArgumentsVA.

2. For convince the patch adds JS_EncodeStringToBuffer and JS_GetStringEncodingLength to encode string into the buffer.

3. The new functions are used in xpcconvert.cpp to encode the string into the buffer provided by the local allocator. To make that work correctly the patch forces various parts of parameter conversion to use the allocator when the native parameters are char*. That was sufficient to pass the try server. But in principle there are still code paths that could ask for unallocated char* parameter which would report an error with the patch. But it should be trivial to fix that if we would get the error reports.

Comment 14

[Andreas Gal :gal]

Comment on attachment 490454 [details] [diff] [review]
v2

Please add a real bug for the FIXME. Great work. Thanks for helping with this.

Comment 15

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/f7171a41a816

Comment 16

[Boris Zbarsky [:bzbarsky]]

Two questions about the xpcconvert changes:

1) The code as written will write to memory it doesn't own if SetLength() fails.
   Is that by design (e.g. expecting infallible string allocations)?
2) Do we have any data on how often we used to hit the deflated buffer cache for
   the |string| and |ACString| cases in JSData2Native?  Some methods that take
   those are called a good bit, with large strings, but not sure whether it
   happens multiple times per string...

Comment 17

[Andreas Gal :gal]

I will get some data on 2) for a gmail workload. 1) needs addressing. Thanks for looking bz.

Comment 18

[Igor Bukanov]

(In reply to comment #16)
> 1) The code as written will write to memory it doesn't own if SetLength()
> fails.
>    Is that by design (e.g. expecting infallible string allocations)?

Thanks for finding this. Before the patch the code did not check for the rs->Assign result. For some reason I took that as infallible allocation assumption when other places there clearly do not expect that so that was just a bug in the existing code.

Comment 19

[Igor Bukanov]

(In reply to comment #18)
> (In reply to comment #16)
> > 1) The code as written will write to memory it doesn't own if SetLength()
> > fails.
> >    Is that by design (e.g. expecting infallible string allocations)?
> 
> Thanks for finding this. Before the patch the code did not check for the
> rs->Assign result. 

This is wrong - nsAString is in fact nsAString_internal which expects infallible allocation so SetLength and Assign are both void.

Comment 20

[Philip Taylor]

Looks like jsapi.h's documentation of JS_ConvertArguments should be updated - it still claims 's' produces a char*, but it's a synonym for 'S' now.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> which expects infallible allocation so SetLength and Assign are both void.

They're void, but the allocation is NOT infallible.  And internal string code checks for alloc failure when trying to Assign and silently doesn't assign.  It's a crappy API, but it's what we have to work with for the moment.

The way to check whether SetLength failed is to check whether Length() is the length you tried to set.

Comment 22

[Boris Zbarsky [:bzbarsky]]

So to be precise the old code was buggy in that it would silently pass an empty string instead of the right string value into the C++ callee.   The new code is buggy in that in the same circumstance it scribbles on memory.

Comment 23

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/f394014be68e - followup to eliminate 's' support from JS_ConvertArguments (comment 20) and to add failed SetLength check to XPCConvert::JSData2Native (comment 22).

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f7171a41a816

Comment 25

[Boris Zbarsky [:bzbarsky]]

And http://hg.mozilla.org/mozilla-central/rev/f394014be68e for the comment 23 followup.

Comment 26

[Eric Shepherd [:sheppy]]

MXR tells me that JS_GetStringBytes isn't actually gone yet, although it's close. Should I mark this as obsolete anyway in the docs? Also, I presume the same applies to JS_GetStringBytesZ?

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> MXR tells me that JS_GetStringBytes isn't actually gone yet, although it's
> close.

http://mxr.mozilla.org/mozilla-central/ident?i=JS_GetStringBytes

and

http://mxr.mozilla.org/mozilla-central/ident?i=JS_GetStringBytesZ

shows no hits for me.

> Should I mark this as obsolete anyway in the docs? Also, I presume the
> same applies to JS_GetStringBytesZ?

We should state that JS_GetStringBytesZ, JS_GetStringBytes and JS_GetFunctionName are no longer available. As a replacement the users should use JS_EncodeString, JS_GetStringEncodingLength/JSEncodeStringToBuffer and JS_GetFunctionId. In C++ they may also have a look at JSAutoByteString class that helps to manage the lifetime of the JS_EncodeString result.

Comment 28

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_GetStringBytes

Also mentioned in the other places where appropriate.

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> Documentation updated:
> 
> https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_GetStringBytes

The documentation regarding JS_EncodeStringToBuffer is wrong. It states:

> The number of bytes actually copied into the buffer is returned.

But it should be read:

The function returns the number of length of the whole string encoding or (size_t) -1 if the string cannot be encoded as bytes. Thus if the result is grater than the size of the supplied buffer, then the string was cut.

Comment 30

[Eric Shepherd [:sheppy]]

Fixed; thanks.

Comment 32

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

And I updated <https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_ConvertArguments> for the removal of 's' support.

Comment 33

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

(In reply to :Ms2ger from comment #32)
> And I updated
> <https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/
> JS_ConvertArguments> for the removal of 's' support.

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_User_Guide needs to be updated for that as well.

Comment 34

[Eric Shepherd [:sheppy]]

Done.