Last Comment Bug 417377 - Missed SAVE_SP_AND_PC in JSOP_YIELD
: Missed SAVE_SP_AND_PC in JSOP_YIELD
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-13 16:51 PST by Igor Bukanov
Modified: 2008-03-25 18:31 PDT (History)
3 users (show)
brendan: blocking1.9+
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
bob: in‑testsuite-
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.62 KB, patch)
2008-02-13 17:38 PST, Igor Bukanov
brendan: review+
brendan: approval1.9+
Details | Diff | Review
fix for 1.8 (3.63 KB, patch)
2008-02-29 13:36 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.13+
Details | Diff | Review
diff between trunk and branch versions (3.93 KB, text/plain)
2008-02-29 13:38 PST, Igor Bukanov
no flags Details

Description Igor Bukanov 2008-02-13 16:51:24 PST
Here is the JSOP_YIELD case from js_Interpret:

          BEGIN_CASE(JSOP_YIELD)
            ASSERT_NOT_THROWING(cx);
            if (FRAME_TO_GENERATOR(fp)->state == JSGEN_CLOSING) {
                js_ReportValueError(cx, JSMSG_BAD_GENERATOR_YIELD,
                                    JSDVG_SEARCH_STACK, fp->argv[-2], NULL);
                ok = JS_FALSE;
                goto out;
            }
            fp->rval = FETCH_OPND(-1);
            fp->flags |= JSFRAME_YIELDING;
            pc += JSOP_YIELD_LENGTH;
            SAVE_SP_AND_PC(fp);

Here SAVE_SP_AND_PC(fp) is not called before reporting an error. As such a freshly allocated GC thing on the stack may become unrooted leading to a GC hazard if a GC is triggered twice during js_ReportValueError.
Comment 1 Igor Bukanov 2008-02-13 17:23:41 PST
(In reply to comment #0)
> Here SAVE_SP_AND_PC(fp) is not called before reporting an error. As such a
> freshly allocated GC thing on the stack may become unrooted leading to a GC
> hazard if a GC is triggered twice during js_ReportValueError.

This is wrong. The hazard can only happen if the collected value is accessed. With the current it is only possible when a debugger installs a call handler and the handler executes a GC. Only in this case the stack slot with a collected during js_ReportValueError value can be accessed.
Comment 2 Igor Bukanov 2008-02-13 17:38:26 PST
Created attachment 303171 [details] [diff] [review]
v1

There is another missing SAVE_SP_AND_PC in JSOP_ARRAYPUSH with the same consequences: the hazard is possible only if a debugger installs a handler that runs a GC.

Plus in the patch I removed default: case in the getter switch to save the code and less time to check that SAVE_SP_AND_PC is not missed there.
Comment 3 Brendan Eich [:brendan] 2008-02-13 17:52:21 PST
Comment on attachment 303171 [details] [diff] [review]
v1

Looks good, thanks.

/be
Comment 5 Daniel Veditz [:dveditz] 2008-02-29 12:19:32 PST
Is there a chance of getting a 1.8 patch by the March 7 code-freeze? If not I suppose we can wait another cycle for a "moderate" bug.
Comment 6 Igor Bukanov 2008-02-29 13:36:35 PST
Created attachment 306590 [details] [diff] [review]
fix for 1.8

Compared with the trunk version the version for 1.8 the patch does not need to fix trunk-only JSOP_ARRAYPUSH. In addition there is no ASSERT_SAVED_SP_AND_PC on the branch so the patch backports it.
Comment 7 Igor Bukanov 2008-02-29 13:38:17 PST
Created attachment 306591 [details]
diff between trunk and branch versions
Comment 8 Brendan Eich [:brendan] 2008-02-29 19:26:53 PST
Comment on attachment 306590 [details] [diff] [review]
fix for 1.8

>-              case JSOP_INITELEM:
>+              default:
>+                JS_ASSERT(op2 == JSOP_INITELEM);

FYI, see bug 356378 and bug 380432.

This patch is good, but we want further fixing of one or both of the above.

/be
Comment 9 Daniel Veditz [:dveditz] 2008-03-03 11:19:32 PST
Comment on attachment 306590 [details] [diff] [review]
fix for 1.8

approved for 1.8.1.13, a=dveditz for release-drivers
Comment 10 Igor Bukanov 2008-03-03 12:03:57 PST
I checked in the patch from comment 6 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.97; previous revision: 3.181.2.96
done
Comment 11 Igor Bukanov 2008-03-03 12:19:08 PST
(In reply to comment #10)
> I checked in the patch from comment 6 to the trunk:

I meant MOZILLA_1_8_BRANCH, not trunk.

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