The default bug view has changed. See this FAQ.

Eliminating JS_GetStringBytes

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

/be
(Assignee)

Comment 2

7 years ago
(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
(Assignee)

Comment 3

7 years ago
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

7 years ago
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.

Updated

7 years ago
No longer blocks: 606065
(Assignee)

Updated

7 years ago
Depends on: 607695
(Assignee)

Comment 5

7 years ago
(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?
(Assignee)

Comment 6

7 years ago
(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

7 years ago
Sure, if we can get something landed that quickly we should definitely take the GetStringBytes elimination.
(Assignee)

Comment 8

7 years ago
Created attachment 487441 [details] [diff] [review]
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

7 years ago
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.
(Assignee)

Comment 10

7 years ago
(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.

Updated

7 years ago
blocking2.0: ? → betaN+
(Assignee)

Comment 11

7 years ago
Created attachment 487761 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Attachment #487441 - Attachment is obsolete: true

Updated

7 years ago
Blocks: 609440
(Assignee)

Updated

7 years ago
Depends on: 610198
(Assignee)

Comment 12

7 years ago
Created attachment 489456 [details] [diff] [review]
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.
Attachment #487761 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 490454 [details] [diff] [review]
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.
Attachment #489456 - Attachment is obsolete: true
Attachment #490454 - Flags: review?(gal)

Comment 14

6 years ago
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+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/tracemonkey/rev/f7171a41a816
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

6 years ago
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...

Comment 17

6 years ago
I will get some data on 2) for a gmail workload. 1) needs addressing. Thanks for looking bz.
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 23

6 years ago
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).

Updated

6 years ago
Blocks: 613589

Comment 24

6 years ago
http://hg.mozilla.org/mozilla-central/rev/f7171a41a816
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
And http://hg.mozilla.org/mozilla-central/rev/f394014be68e for the comment 23 followup.
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?
(Assignee)

Comment 27

6 years ago
(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.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 29

6 years ago
(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
(Assignee)

Updated

6 years ago
Duplicate of this bug: 608799
(Assignee)

Updated

6 years ago
Blocks: 608799
And I updated <https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_ConvertArguments> for the removal of 's' support.
(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.
Keywords: dev-doc-complete → dev-doc-needed
Done.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.