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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

8 years ago
Created attachment 441147 [details] [diff] [review]
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.
(Assignee)

Comment 2

8 years ago
Created attachment 441638 [details] [diff] [review]
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.
Assignee: general → jorendorff
Attachment #441147 - Attachment is obsolete: true
Attachment #441638 - Flags: review?(igor)

Comment 3

8 years ago
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.
Attachment #441638 - Flags: review?(igor) → review+
(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
(Assignee)

Comment 5

8 years ago
http://hg.mozilla.org/tracemonkey/rev/394cdeb4fc38
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

8 years ago
Depends on: 562909

Comment 6

8 years ago
http://hg.mozilla.org/mozilla-central/rev/394cdeb4fc38
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

8 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.