Closed Bug 607292 Opened 14 years ago Closed 14 years ago

Eliminating JS_GetStringBytes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

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

/be
(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.
Blocks: 606065
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.
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.
No longer blocks: 606065
Depends on: 607695
(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?
(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?
Sure, if we can get something landed that quickly we should definitely take the GetStringBytes elimination.
Attached patch wip (obsolete) — Splinter Review
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.
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.
(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.
blocking2.0: ? → betaN+
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #487441 - Attachment is obsolete: true
Blocks: 609440
Depends on: 610198
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #487761 - Attachment is obsolete: true
Attached patch v2Splinter Review
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.
Attachment #489456 - Attachment is obsolete: true
Attachment #490454 - Flags: review?(gal)
Comment on attachment 490454 [details] [diff] [review]
v2

Please add a real bug for the FIXME. Great work. Thanks for helping with this.
Attachment #490454 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/f7171a41a816
Whiteboard: fixed-in-tracemonkey
Blocks: 612150
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...
I will get some data on 2) for a gmail workload. 1) needs addressing. Thanks for looking bz.
(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.
(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.
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.
> 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.
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.
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).
Blocks: 613589
http://hg.mozilla.org/mozilla-central/rev/f7171a41a816
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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?
(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.
Documentation updated:

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

Also mentioned in the other places where appropriate.
(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.
Fixed; thanks.
Depends on: 617215
Blocks: 608799
(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.
You need to log in before you can comment on or make changes to this bug.