Closed Bug 864957 Opened 7 years ago Closed 7 years ago

Consolidate locks used to avoid racing under JSRuntime::triggerOperationCallback

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bhackett, 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+
https://hg.mozilla.org/mozilla-central/rev/e478c0c1940f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.