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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Attachment #464210 - Flags: review?(jorendorff)
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.
Attachment #464213 - Flags: review?(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
Attachment #464213 - Flags: review?(brendan) → review+
blocking2.0: --- → beta5+
Assignee: general → jim
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.
Attachment #464210 - Flags: review?(jorendorff) → review+
(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.
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.
Revised per Jason's comments, and to avoid ISO C temporary file functions.
Attachment #464210 - Attachment is obsolete: true
Attachment #467128 - Flags: review?(jorendorff)
http://hg.mozilla.org/tracemonkey/rev/afa9d15fb8b6
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/afa9d15fb8b6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #467128 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.