Last Comment Bug 402898 - JS_YieldRequest doesn't do scope-sharing
: JS_YieldRequest doesn't do scope-sharing
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla1.9beta4
Assigned To: Jason Orendorff [:jorendorff]
: Jason Orendorff [:jorendorff]
Depends on: 416470
Blocks: 404879
  Show dependency treegraph
Reported: 2007-11-07 11:43 PST by Jason Orendorff [:jorendorff]
Modified: 2008-02-22 06:49 PST (History)
7 users (show)
bob: in‑testsuite-
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (1.07 KB, patch)
2008-02-03 04:48 PST, Jason Orendorff [:jorendorff]
brendan: review+
brendan: approval1.9+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2007-11-07 11:43:40 PST
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 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-11-07 12:44:56 PST
I am a simple caveman, your scope-sharing ways frighten and confuse me.
Comment 2 User image Jason Orendorff [:jorendorff] 2008-02-03 04:48:59 PST
Created attachment 301109 [details] [diff] [review]

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 User image Jason Orendorff [:jorendorff] 2008-02-07 14:19:31 PST
Maybe peterv can help.
Comment 4 User image Peter Van der Beken [:peterv] 2008-02-08 01:55:40 PST
(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 User image Jason Orendorff [:jorendorff] 2008-02-08 17:09:52 PST
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 User image Jason Orendorff [:jorendorff] 2008-02-08 20:07:44 PST
The suspend/resume issue is bug 416470; patch is attached there.  After that patch, this one is (I think) ready to roll.
Comment 7 User image Brendan Eich [:brendan] 2008-02-12 13:45:13 PST
Comment on attachment 301109 [details] [diff] [review]

Neat, sweet, petite!

Comment 8 User image Reed Loden [:reed] (use needinfo?) 2008-02-14 03:14:42 PST
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.408; previous revision: 3.407
Comment 9 User image Jason Orendorff [:jorendorff] 2008-02-22 06:49:40 PST
(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.

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