Closed Bug 864957 Opened 12 years ago Closed 12 years ago

Consolidate locks used to avoid racing under JSRuntime::triggerOperationCallback

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bhackett1024, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
JSRuntime::triggerOperationCallback touches several bits of data from off thread which are protected by two different locks. Both of these locks are used only to avoid races between the interrupting and interrupted threads. Bug 864220 needs more data to be protected against such races, and rather than add a third lock for this purpose it would be better if the existing two were consolidated in a similar manner to the GC lock. The attached patch makes this change.
Attachment #740989 - Flags: review?(luke)
Attachment #740989 - Flags: review?(kvijayan)
Comment on attachment 740989 [details] [diff] [review] patch Review of attachment 740989 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review because of syntax error. Otherwise, most comments are stylistic or debug notes. ::: js/src/jsapi.cpp @@ +995,5 @@ > #ifdef JS_THREADSAFE > clearOwnerThread(); > + > + if (operationCallbackLock) > + PR_DestroyLock(operationCallbackLock); JS_ASSERT(!operationCallbackOwner) here. ::: js/src/jscntxt.h @@ +645,5 @@ > + * Lock taken when triggering the operation callback from another thread. > + * Protects all data that is touched in this process. > + */ > + PRLock *operationCallbackLock; > + PRThread *operationCallbackOwner; Wrap |operationCallbackOwner| field with #ifdef DEBUG, it's only used in debug contexts. @@ +651,5 @@ > + void setOperationCallbackOwner(PRThread *thread) { > +#ifdef DEBUG > + operationCallbackOwner = thread; > + } > +#endif // DEBUG This seems to be a syntax error with --disable-debug. @@ +662,5 @@ > + public: > + AutoLockForOperationCallback(JSRuntime *rt MOZ_GUARD_OBJECT_NOTIFIER_PARAM) : rt(rt) { > + MOZ_GUARD_OBJECT_NOTIFIER_INIT; > + PR_Lock(rt->operationCallbackLock); > + rt->setOperationCallbackOwner(PR_GetCurrentThread()); This needs to be wrapped with #ifdef DEBUG, because PR_GetCurrentThread will be run even if |setOperationCallbackOwner| does nothing with it. Given that this code needs to be wrapped with DEBUG-ifdefs anyway, having a separate |setOperationCallbackOwner| method seems like overkill. Just an #ifdef DEBUG-wrapped line setting the field directly here (and below when setting it to NULL) should be enough. @@ +666,5 @@ > + rt->setOperationCallbackOwner(PR_GetCurrentThread()); > + } > + ~AutoLockForOperationCallback() { > + JS_ASSERT(rt->operationCallbackOwner == PR_GetCurrentThread()); > + rt->setOperationCallbackOwner(NULL); See above comment.
Attachment #740989 - Flags: review?(kvijayan)
Attached patch patchSplinter Review
Attachment #740989 - Attachment is obsolete: true
Attachment #740989 - Flags: review?(luke)
Attachment #741075 - Flags: review?(luke)
Attachment #741075 - Flags: review?(kvijayan)
Comment on attachment 741075 [details] [diff] [review] patch Review of attachment 741075 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #741075 - Flags: review?(kvijayan) → review+
Comment on attachment 741075 [details] [diff] [review] patch Hah, nice catch.
Attachment #741075 - Flags: review?(luke) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: