402898 - JS_YieldRequest doesn't do scope-sharing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

JS_YieldRequest doesn't check rt->scopeSharingTodo the way JS_EndRequest and JS_SuspendRequest do.  This means that while JS_YieldRequest yields to GC, it doesn't yield up objects to other threads that need them. I think it should.

Cc-ing shaver in case he remembers the rationale for how this behaves.

Comment 1

[Mike Shaver (:shaver emeritus)]

I am a simple caveman, your scope-sharing ways frighten and confuse me.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

OK, the attached patch fixes this bug, but I'm uneasy about something.  I don't think this comment in jscntxt.h is correct:

    /* Same as requestDepth but ignoring JS_SuspendRequest/JS_ResumeRequest */
    jsrefcount          outstandingRequests;

To me it looks like JS_SuspendRequest and JS_ResumeRequest do modify outstandingRequests.  This field was introduced in bug 401687.  See comment 21 and 23.  The use is in xpconnect.

Igor, can you help me understand this?

Comment 3

[Jason Orendorff [:jorendorff]]

Maybe peterv can help.

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> To me it looks like JS_SuspendRequest and JS_ResumeRequest do modify
> outstandingRequests.

That does look wrong, I went with what Igor told me but should have looked closer.

Comment 5

[Jason Orendorff [:jorendorff]]

OK.  I would like to get this checked in, but allowing the bug in JS_{Suspend,Resume}Request to propagate to JS_YieldRequest is technically a regression.

I'll file a separate bug for the suspend/resume issue.

Comment 6

[Jason Orendorff [:jorendorff]]

The suspend/resume issue is bug 416470; patch is attached there.  After that patch, this one is (I think) ready to roll.

Comment 7

[Brendan Eich [:brendan]]

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

Neat, sweet, petite!

/be

Comment 8

[Reed Loden [:reed]]

Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.408; previous revision: 3.407
done

Comment 9

[Jason Orendorff [:jorendorff]]

(bc marked this "not in testsuite".)

This has some test coverage, although not a dedicated test.  Without this fix, all the new threading tests (js1_8/extensions/dekker.js and friends) deadlock.