864957 - Consolidate locks used to avoid racing under JSRuntime::triggerOperationCallback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Kannan Vijayan [:djvj]]

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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 741075 [details] [diff] [review]
patch

Review of attachment 741075 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 741075 [details] [diff] [review]
patch

Hah, nice catch.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e478c0c1940f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e478c0c1940f