JS_YieldRequest doesn't do scope-sharing

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Trunk
mozilla1.9beta4
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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.
I am a simple caveman, your scope-sharing ways frighten and confuse me.
(Assignee)

Comment 2

10 years ago
Created attachment 301109 [details] [diff] [review]
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?
Assignee: general → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 3

10 years ago
Maybe peterv can help.
(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.
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Updated

10 years ago
Blocks: 404879
(Assignee)

Updated

10 years ago
Depends on: 416470
(Assignee)

Updated

10 years ago
Attachment #301109 - Flags: review?(brendan)
(Assignee)

Comment 6

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

Updated

10 years ago
Attachment #301109 - Attachment description: v1 - apply after the patch in bug 412985 → v1
Comment on attachment 301109 [details] [diff] [review]
v1

Neat, sweet, petite!

/be
Attachment #301109 - Flags: review?(brendan)
Attachment #301109 - Flags: review+
Attachment #301109 - Flags: approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.408; previous revision: 3.407
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
(Assignee)

Comment 9

10 years ago
(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.
You need to log in before you can comment on or make changes to this bug.