Last Comment Bug 431489 - js_DecompileCode should not print if Decompile failed
: js_DecompileCode should not print if Decompile failed
: fixed1.8.1.15
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2008-04-29 23:55 PDT by Igor Bukanov
Modified: 2008-07-02 02:15 PDT (History)
9 users (show)
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (982 bytes, patch)
2008-04-30 00:01 PDT, Igor Bukanov
brendan: review+
shaver: approval1.9+
Details | Diff | Splinter Review
v1 for 1.8 branch (936 bytes, patch)
2008-05-30 00:21 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description Igor Bukanov 2008-04-29 23:55:14 PDT
As the bug 431248 shows, a failed LOCAL_ASSERT does not save the decompiler and leads to crashes without DEBUG. The reason for this is that DecompileCode does not check that Decompile returns true before trying to print the last result:

    ok = Decompile(&ss, pc, len, JSOP_NOP) != NULL;
    if (code != oldcode) {
        JS_free(cx, jp->script->code);
        jp->script->code = oldcode;
        jp->script->main = oldmain;
    jp->script = oldscript;

    /* If the given code didn't empty the stack, do it now. */
    if ( {
^^^^^^^^^^^^^^^^^^ - the missing ok check
        do {
            last = OFF2STR(&ss.sprinter, PopOff(&ss, JSOP_POP));
        } while ( > pcdepth);
        js_printf(jp, "%s", last);
Note that ok can be false here not only due to bugs in the code but also due to oom errors.
Comment 1 Igor Bukanov 2008-04-30 00:01:22 PDT
Created attachment 318556 [details] [diff] [review]

Adding the missed ok check.
Comment 2 Igor Bukanov 2008-04-30 00:03:11 PDT
To test the patch, one can use the test case from the bug 431248 without the fix there.
Comment 3 Brendan Eich [:brendan] 2008-04-30 00:08:47 PDT
Comment on attachment 318556 [details] [diff] [review]

This is totally safe to ride along into 1.9. You can see ok set unconditionally above the added test, in the context.

Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-04-30 05:56:30 PDT
Comment on attachment 318556 [details] [diff] [review]

a=shaver: protection against other crashes in cases of LOCAL_ASSERT failure is righteous.
Comment 6 Bob Clary [:bc:] 2008-04-30 14:01:27 PDT
see bug 431248 js1_7/extensions/regress-431248.js
Comment 7 Bob Clary [:bc:] 2008-05-03 05:34:49 PDT
v 1.9.0
Comment 8 Igor Bukanov 2008-05-30 00:21:05 PDT
Created attachment 323035 [details] [diff] [review]
v1 for 1.8 branch

The patch is a trivial backport of one-liner from the trunk.
Comment 9 Daniel Veditz [:dveditz] 2008-06-04 11:23:04 PDT
Comment on attachment 323035 [details] [diff] [review]
v1 for 1.8 branch

Approved for, a=dveditz for release-drivers
Comment 10 Daniel Veditz [:dveditz] 2008-06-04 11:26:35 PDT
Is there a testcase for this on the branch? this bug refers to bug 431248 which appears to be a trunk-only regression.
Comment 11 Igor Bukanov 2008-06-04 11:59:31 PDT
(In reply to comment #10)
> Is there a testcase for this on the branch? 

I am not aware of a test case that shows the problem on the branch.

Comment 12 Igor Bukanov 2008-06-06 14:45:27 PDT
I checked in the patch from the comment 8 to the MOZILLA_1_8_BRANCH:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision:; previous revision:

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