Last Comment Bug 779017 - always create ScriptSource
: always create ScriptSource
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: mozilla17
Assigned To: :Benjamin Peterson
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 774471 779233
  Show dependency treegraph
Reported: 2012-07-30 18:46 PDT by :Benjamin Peterson
Modified: 2012-08-01 02:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

give every script a ScriptSource (20.46 KB, patch)
2012-07-30 18:48 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description User image :Benjamin Peterson 2012-07-30 18:46:38 PDT
JSScript's scriptSource_ pointer should never be NULL. If the source is not available, scriptSource_ should just be a shell. Adding the source code should be a different method. This will allow other items that are shared per compilation unit like filename and the source map to be moved to the ScriptSource struct.
Comment 1 User image :Benjamin Peterson 2012-07-30 18:48:43 PDT
Created attachment 647397 [details] [diff] [review]
give every script a ScriptSource
Comment 2 User image Jason Orendorff [:jorendorff] 2012-07-31 14:18:14 PDT
Comment on attachment 647397 [details] [diff] [review]
give every script a ScriptSource

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

Looks good except for a few nits. r=me with those fixed.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +245,5 @@
>  {
>      if (!CheckLength(cx, length))
>          return false;
> +    ScriptSource *ss = cx->new_<ScriptSource>();
> +    AutoAttachToRuntime attacher(cx->runtime, ss);

CompileScript() checks for null ss. This should too.

::: js/src/jsscript.cpp
@@ +1243,3 @@
>      if (!ownSource) {
>          const size_t nbytes = length * sizeof(jschar);
> +        data.compressed = static_cast<unsigned char *>(cx->runtime->malloc_(nbytes));

Instead use cx->malloc_(nbytes) which reports the error on failure.

@@ +1363,4 @@
>              return false;
>      }
> +    if (!xdr->codeBytes(data.compressed, byteLen)) {
> +        xdr->cx()->free_(data.compressed);

This should only free this memory if we allocated it in the first place (that is, if mode is XDR_DECODE).

Also: if we are decoding, it should probably return length_, compressedLength_, and data.compressed to a consistent state before returning, right? (This ScriptSource is already attached to a script, I think, so it'll be destroyed eventually.) Maybe this can be rewritten to first decode to local variables, then call setSource() only on success.
Comment 4 User image Ed Morley [:emorley] 2012-08-01 02:51:50 PDT

Note You need to log in before you can comment on or make changes to this bug.