Closed Bug 938390 Opened 9 years ago Closed 9 years ago

don't create a string to hold chars in ScriptSource::chars


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)


(Whiteboard: [qa-])


(1 file)

If I understand correctly, we aren't really taking advantage of the JSString created in ScriptSource::chars anywhere.  We just hold it in a HashMap that gets flushed on every GC.  The JSString poses a problem because JSString::MAX_LENGTH limits the max script length to 2^28 chars which we actually hit in un-optimized un-minified Emscripten builds.  I think we should be able to store the malloc'd chars in the map directly and FreeOp::free_ them in purge().
Seems to be a simple enough fix.
Attachment #831875 - Flags: review?(benjamin)
Comment on attachment 831875 [details] [diff] [review]

Review of attachment 831875 [details] [diff] [review]:

Hmm, I'm not sure why the source was ever stored in a string. Perhaps for the convenience of the debugger at some point. It is rather scary to have to such large source files that this is a problem!

::: js/src/jsscript.cpp
@@ +1049,5 @@
> +    if (!map_)
> +        return;
> +
> +    for (Map::Range r = map_->all(); !r.empty(); r.popFront())
> +        js_delete(const_cast<jschar*>(r.front().value));

Should probably be "jschar *" (space added).

@@ +1066,5 @@
>  #ifdef USE_ZLIB
>      if (compressed()) {
> +        if (const jschar *decompressed = cx->runtime()->sourceDataCache.lookup(this))
> +            return decompressed;
> +      

Trailing ws, oh noes!!!

@@ +1070,5 @@
> +      
> +        const size_t nbytes = sizeof(jschar) * (length_ + 1);
> +        jschar *decompressed = static_cast<jschar *>(js_malloc(nbytes));
> +        if (!decompressed)
> +            return nullptr;

This is not your problem, but I suspect we need a JS_ReportOutOfMemory(cx) here.
Attachment #831875 - Flags: review?(benjamin) → review+
Now that I'm looking at this again, I see the memory ownership behavior of ScriptSource::chars() is insane. It basically gives you a pointer that is valid only until the next full gc, which means you might not even be able to do something useful with it before it gets freed under you. I don't actually see how you could write safe code that uses it. I don't think your patch makes this worse, but we should definitely make chars() less terrible as an API.
Oh good point, I hadn't been considering the context of its usage!  So if this is for lazy parsing, is there anything that inhibits GC?
We might want some simple flag on the sourceDataCache to prevent GC
Err, s/GC/purging during GC/
Oh, hah, all the significant callers have an AutoSupressGC that mentions preserving the chars.  Safe for now, but we should clean this up by having SourceDataCache::lookup/put take a PurgeGuard (is what I was calling it my half-done patch) that increments a counter in SourceDataCache that inhibits purging when non-zero.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.