325521 - JS_YieldRequest() allows the garbage collector to race

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Michael Daumling]

JS_YieldRequest() is IMHO a quick replacement for a JS_SuspendRequest/JS_ResumeRequest combo. If used, a garbage collector on a separate thread races. It appears that the function does not wait for any garbage collector to succeed.

I do not have the latest Trucnk code installed, so please forgive me if I submit my proposed fix here. This fix appears to work well.

JS_PUBLIC_API(void)
JS_YieldRequest(JSContext *cx)
{
    JSRuntime *rt;

    JS_ASSERT(cx->thread);
    CHECK_REQUEST(cx);

    rt = cx->runtime;
    JS_LOCK_GC(rt);
    JS_ASSERT(rt->requestCount > 0);
    rt->requestCount--;
    if (rt->requestCount == 0)
        JS_NOTIFY_REQUEST_DONE(rt);
    JS_UNLOCK_GC(rt);
    /* XXXbe give the GC or another request calling it a chance to run here?
             Assumes FIFO scheduling */

    JS_LOCK_GC(rt);
    /* BEGIN FIX */
    if (rt->gcThread != cx->thread) {
        while (rt->gcLevel > 0)
            JS_AWAIT_GC_DONE(rt);
    }
    /* END FIX */
    rt->requestCount++;
    JS_UNLOCK_GC(rt);
}

Brendan, please review if this fix is correct.

Comment 1

[Brendan Eich [:brendan]]

shaver, what do you think?  Looks like the right fix to me.

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Yeah, it does.

Comment 3

[Michael Daumling]

Since you agree that this is the fix, I'll take the bug.

Comment 4

[Michael Daumling]

Attachment: Patch as desrbied in bug.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 210984 [details] [diff] [review]
Patch as desrbied in bug.

Checking in, and I took the opportunity to trim trailing spaces from unrelated lines in this file.

/be

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's a blocking1.8.1? nomination on this bug.  Should it be landed on the 1.8 branch for 1.8.1?

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this part of the JS 1.7 landing? If not, can we get it checked in to the 1.8.1 branch?

Comment 8

[Brendan Eich [:brendan]]

Fixed with JS1.7 landing.

/be

Comment 9

[Bob Clary [:bc] (inactive)]

not on 1.8.0, not making js16