417377 - Missed SAVE_SP_AND_PC in JSOP_YIELD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

(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]

Attachment: 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]]

Comment on attachment 303171 [details] [diff] [review]
v1

Looks good, thanks.

/be

Comment 4

[Igor Bukanov]

I checked in the patch from comment 2 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1203003360&maxdate=1203003362&
who=igor%25mir2.org

Comment 5

[Daniel Veditz [:dveditz]]

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]

Attachment: 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]

Attachment: diff between trunk and branch versions

Comment 8

[Brendan Eich [:brendan]]

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]]

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]

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]

(In reply to comment #10)
> I checked in the patch from comment 6 to the trunk:

I meant MOZILLA_1_8_BRANCH, not trunk.