561444 - "Assertion failure: title->ownercx == cx || title->ownercx->thread == cx->thread, at ../jslock.cpp:1414" with multiple threads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

A new object, created in thread 2, shares an empty scope that was created in thread 1. An assertion in js_IsTitleLocked claims this shouldn't happen.

jsapi-test in the works.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: jsapi-test

The test.

I think the upshot of this is that the property tree must be per-compartment. This makes sense anyway since it must be GC'd and can contain references to objects (getters and setters).

The trace-tree code cache will be per-compartment, too, for similar reasons but also because it's so dependent on the global object.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1 - remove the assertions

Two bugs here.

1. The assertion we're hitting, in jslock.cpp, seems bogus to me. My understanding of the request model is that the JSAPI imposes no such restriction. An application is allowed to create same-shaped objects in two different threads; it seems to me JS_THREADSAFE aims to support that sort of thing.

2. The particular function, js_IsTitleLocked, where the failing assertion appears is only ever called when we assert a particular scope or object is locked--an assertion within an assertion. Once I fix the inner assertion, the outer one fails.

That assertion is in js_GetXMLObject, which claims that its caller should have locked xml->parent->object. There's a nice comment explaining why -- but js_GetXMLObject is called from 32 places, none of which lock anything. The assertion and the accompanying comment seem to be wishful thinking, or perhaps a design sketch for an XML thread safety scheme that was never implemented.

So I propose deleting this assertion too. It would be better to weaken it; we would like to assert whenever the application actually shares XML objects among racing threads. I don't see a way to do that, though.

Comment 3

[Igor Bukanov]

Comment on attachment 441638 [details] [diff] [review]
v1 - remove the assertions

>@@ -1402,23 +1402,21 @@ js_IsTitleLocked(JSContext *cx, JSTitle 
>     /*
>-     * General case: the title is either exclusively owned (by cx), or it has
>-     * a thin or fat lock to cope with shared (concurrent) ownership.
>+     * General case: the title is either exclusively owned by some context, or
>+     * it has a thin or fat lock to cope with shared (concurrent) ownership.
>      */
>-    if (title->ownercx) {
>-        JS_ASSERT(title->ownercx == cx || title->ownercx->thread == cx->thread);
>-        return JS_TRUE;
>-    }
>+    if (title->ownercx)
>+        return title->ownercx == cx;
>     return js_CurrentThreadId() ==
>            ((JSThread *)Thin_RemoveWait(ReadWord(title->lock.owner)))->id;
> }

The assert is really bogus and the changes are right on the target. But we need comments that js_LockObj(cx, obj) must set ownercx to the cx if the current thread can own the object. 

>-    /*
>-     * A JSXML cannot be shared among threads unless it has an object.
>-     * A JSXML cannot be given an object unless:
>-     * (a) it has no parent; or
>-     * (b) its parent has no object (therefore is thread-private); or
>-     * (c) its parent's object is locked.
>-     *
>-     * Once given an object, a JSXML is immutable.
>-     */
>-    JS_ASSERT(!xml->parent ||
>-              !xml->parent->object ||
>-              JS_IS_OBJ_LOCKED(cx, xml->parent->object));
>-

XML objects are not thread safe indeed.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Created an attachment (id=441638) [details]
> That assertion is in js_GetXMLObject, which claims that its caller should have
> locked xml->parent->object. There's a nice comment explaining why -- but
> js_GetXMLObject is called from 32 places, none of which lock anything. The
> assertion and the accompanying comment seem to be wishful thinking, or perhaps
> a design sketch for an XML thread safety scheme that was never implemented.

The latter, FWIW. That comment goes way back.

> So I propose deleting this assertion too. It would be better to weaken it; we
> would like to assert whenever the application actually shares XML objects among
> racing threads. I don't see a way to do that, though.

There's no good way, we should just delete as you propose. E4X is ST-only, too bad. The only bug here is lack of enforcement on MT embeddings.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/394cdeb4fc38

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/394cdeb4fc38

Comment 7

[Jason Orendorff [:jorendorff]]

These follow-ups need to go in m-c as well. I apologize.

http://hg.mozilla.org/tracemonkey/rev/f462a8e1fe38
http://hg.mozilla.org/tracemonkey/rev/633d4ea174a8