Last Comment Bug 680755 - Missing goto out in CompileUCFunctionForPrincipalsCommon
: Missing goto out in CompileUCFunctionForPrincipalsCommon
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks: 679879
  Show dependency treegraph
 
Reported: 2011-08-21 06:40 PDT by Igor Bukanov
Modified: 2011-08-25 04:39 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v 0.1 (4.75 KB, patch)
2011-08-21 06:49 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v1 (19.93 KB, patch)
2011-08-24 14:39 PDT, Igor Bukanov
jorendorff: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-08-21 06:40:25 PDT
Since the bug 679879 CompileUCFunctionForPrincipalsCommon from jsapi.cpp contains http://hg.mozilla.org/mozilla-central/annotate/6dc468c41136/js/src/jsapi.cpp#l4724 :

        EmptyShape *emptyCallShape = EmptyShape::getEmptyCallShape(cx);
        if (!emptyCallShape)
            fun = NULL;
        AutoShapeRooter shapeRoot(cx, emptyCallShape);

        AutoObjectRooter tvr(cx, fun);

That is, there is a missing goto out here. This bug leads to a NULL pointer deref in case EmptyShape::getEmptyCallShape fails to allocate the shape.
Comment 1 Igor Bukanov 2011-08-21 06:49:29 PDT
Created attachment 554714 [details] [diff] [review]
v 0.1

The patch adds an auto class to do the last-frame-checks and uses it in CompileUCFunctionForPrincipalsCommon to replace replace LAST_FRAME_CHECKS macro. This way on errors the code can immediately return null.

If the proposed usage looks OK, I will replace all LAST_FRAME_CHECKS with the new class.
Comment 2 Jason Orendorff [:jorendorff] 2011-08-22 12:26:23 PDT
Comment on attachment 554714 [details] [diff] [review]
v 0.1

This is a fine idea. I'll be happy to review, but the patch has a bug: LAST_FRAME_CHECKS only reports an exception if no JS code is running on cx (hence the words "last frame" in the name). This is by design. It means that JSNatives and other callbacks can call public JSAPI functions like so:

    if (!JS_CallFunctionName(cx, obj, "foo", 0, NULL, &v))
        return false;

and any exceptions thrown from obj.foo() will be propagated to the caller, not squelched or converted to uncatchable errors. (If we don't have a test for this, consider writing a jsapi-test.)

I think the success() method is unnecessary. It's harmless to check for an exception on success.
Comment 3 Igor Bukanov 2011-08-22 14:36:12 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> This is a fine idea. I'll be happy to review, but the patch has a bug:
> LAST_FRAME_CHECKS only reports an exception if no JS code is running on cx
> (hence the words "last frame" in the name). 

Thanks for catching this, the destructor were supposed to look like:

if (cx && JS_IsRunning(cx) && !cx->hasRunOption(JSOPTION_DONT_REPORT_UNCAUGHT))
    js_ReportUncaughtException(cx);

> I think the success() method is unnecessary. It's harmless to check for an
> exception on success.

Do you suggest to check for cx->throwing instead? That would indeed remove the need for the success method.
Comment 4 Igor Bukanov 2011-08-22 14:46:42 PDT
(In reply to Igor Bukanov from comment #3)
> (In reply to Jason Orendorff [:jorendorff] from comment #2)
> > I think the success() method is unnecessary. It's harmless to check for an
> > exception on success.
> 
> Do you suggest to check for cx->throwing instead? That would indeed remove
> the need for the success method.

The drawback of checking for cx->throwing is an extra branch per API call. With the explicit success() the compiler can optimize away the exception check at places where the code checks for errors in any case. But I presume that would not affect the benchmarks...
Comment 5 Jason Orendorff [:jorendorff] 2011-08-23 04:49:47 PDT
(In reply to Igor Bukanov from comment #3)
> Do you suggest to check for cx->throwing instead? That would indeed remove
> the need for the success method.

Yes.

(In reply to Igor Bukanov from comment #4)
> The drawback of checking for cx->throwing is an extra branch per API call.
> With the explicit success() the compiler can optimize away the exception
> check at places where the code checks for errors in any case.

I actually find this fairly convincing. Keep it if you prefer.
Comment 6 Igor Bukanov 2011-08-24 14:39:13 PDT
Created attachment 555548 [details] [diff] [review]
v1

The new version removes the success method and checks for cx->throwing instead. In most places where the last frame checks are used we do not check for the error explicitly and just return the result of the internal api call directly.  So the issues from the comment 4 is not common to justify extra code complexity.

The patch also removes unnecessary anchoring and rooting around js_NewFunction in couple of places via rearranging the sequence of calls so compileFunctionBody immediately follows js_NewFunction.
Comment 7 Jason Orendorff [:jorendorff] 2011-08-24 16:37:59 PDT
Comment on attachment 555548 [details] [diff] [review]
v1

Nice cleanup.
Comment 9 Marco Bonardo [::mak] 2011-08-25 04:39:18 PDT
http://hg.mozilla.org/mozilla-central/rev/62fd8154f8de

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