585803 - JS_CompileFile and friends can return JSScript::emptyScript(), which cannot have JS_NewScriptObject applied to it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The JSAPI should allow callers to apply JS_NewScriptObject to any JSScript returned via the usual script-creation functions.  However, JS_CompileFile and friends will occasionally return JSScript::emptyScript(), which (naturally) must not be assigned to.

Fix seems similar to that done in bug 516827.

Comment 1

[Jim Blandy :jimb]

Attachment: Unit tests for existing JSScript and script object creation API.

In addition to the new JSAPI tests, this adds BEGIN/END_FIXTURE_TEST macros to jsapi-tests/tests.h.  "Fixture" is the terminology that seems to be used in the Google C++ Test Framework, and Junit, the Java testing framework that it is based on.

Comment 2

[Jim Blandy :jimb]

Attachment: Pass TCF_NEED_MUTABLE_SCRIPT when producing scripts returned via JSAPI functions.

All scripts produced using JSAPI functions should be able to have
JS_NewScriptObject applied to them. However, JS_CompileFile and
JS_CompileFileHandleForPrincipals fail to pass TCF_NEED_MUTABLE_SCRIPT, and
thus will occasionally return JSScript::emptyScript(); applying
JS_NewScriptObject to that causes a crash.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 464213 [details] [diff] [review]
Pass TCF_NEED_MUTABLE_SCRIPT when producing scripts returned via JSAPI functions.

Oops, sorry I missed those.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 464210 [details] [diff] [review]
Unit tests for existing JSScript and script object creation API.


>+        CHECK(!JSVAL_TO_OBJECT(script_object.value()));

JSVAL_IS_NULL

>+        CHECK(JSVAL_TO_OBJECT(second_script_object.value()));

!JSVAL_IS_PRIMITIVE

In both cases, my suggestion has almost the same semantics as what you
wrote but allows the jsapi-test assertion to fail (non-crashingly)
rather than flunk an assertion in jsapi.h or, which is much worse,
erroneously pass in non-DEBUG builds.

>+BEGIN_FIXTURE_TEST(ScriptObjectFixture, bug438633_JS_CompileUCScriptForPrincipals)
>+{
>+    return tryScript(JS_CompileUCScriptForPrincipals(cx, global, NULL,
>+                                                     uc_code, code_size,
>+                                                   __FILE__, __LINE__));

Indentation.

>+BEGIN_FIXTURE_TEST(ScriptObjectFixture, bug438633_JS_CompileFile)
>+{
>+    char script_filename[L_tmpnam];
>+    CHECK(tmpnam(script_filename) != NULL);

I doubt tmpnam is available everywhere. In fact I'm not even sure about
fopen and fseek. Better #ifdef these. Perhaps our configury already
provides the appropriate HAVE_TMPNAM macro?

Same for tmpfile.

>+BEGIN_TEST(testScriptObject_ScriptlessScriptObjects)
>+{
>+    /* JS_NewScriptObject(cx, NULL) should return a fresh object each time. */

...really? Fascinating.

>+    CHECK(JSVAL_TO_OBJECT(script_object1.value()));

Again, CHECK(!JSVAL_IS_PRIMITIVE(...

r=me with the nits picked.

Comment 5

[Jim Blandy :jimb]

(In reply to comment #4)
> Comment on attachment 464210 [details] [diff] [review]
> Unit tests for existing JSScript and script object creation API.
> 
> 
> >+        CHECK(!JSVAL_TO_OBJECT(script_object.value()));
> 
> JSVAL_IS_NULL
> 
> >+        CHECK(JSVAL_TO_OBJECT(second_script_object.value()));
> 
> !JSVAL_IS_PRIMITIVE

Ah, of course. I just corrected these same mistakes in dherman's AST patch.

> Indentation.

Thanks.

> >+BEGIN_FIXTURE_TEST(ScriptObjectFixture, bug438633_JS_CompileFile)
> >+{
> >+    char script_filename[L_tmpnam];
> >+    CHECK(tmpnam(script_filename) != NULL);
> 
> I doubt tmpnam is available everywhere. In fact I'm not even sure about
> fopen and fseek. Better #ifdef these. Perhaps our configury already
> provides the appropriate HAVE_TMPNAM macro?

tmpnam and tmpfile are in ISO C, and MSDN says they're available on Windows. I'll make sure.

Comment 6

[Jim Blandy :jimb]

It turns out that tmpnam and tmpfile try to create the files in the root directory, which fails unless you have Administrator privileges. I've rewritten the patch to just use fixed names, and provide a class any test can use.

Comment 7

[Jim Blandy :jimb]

Attachment: Unit tests for existing JSScript and script object creation API.

Revised per Jason's comments, and to avoid ISO C temporary file functions.

Comment 8

[Jim Blandy :jimb]

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

Comment 9

[Robert Sayre]

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

Comment 10

[Jim Blandy :jimb]

Tests landed:
http://hg.mozilla.org/tracemonkey/rev/5c45e81f1a14

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5c45e81f1a14