704258 - "Assertion failure: static_cast<Cell *>(thing)->isMarked()" with adoptNode, gczeal(4)

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension) (asserts fatally when loaded)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

Result:

Assertion failure: static_cast<Cell *>(thing)->isMarked(), at js/src/jsgc.cpp:3572

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

Excellent test! I strongly suspected that ReparentWrapperIfFound was not being barriered correctly, but I didn't know how to test it. This is exactly what I was looking for. Jesse, is there a way we could add this to the test suite (maybe Mochitests)?

The problem was that the trace hook for a wrapped native works pretty much as follows:
  TraceScopeJSObjects(trc, wrapper->GetScope());
What I didn't realize is that GetScope() doesn't directly access a field. Instead, it often does GetProto()->GetScope(). This means that if the proto of a wrapped native changes, then its scope is often changing as well. This patch marks the scope as well as the proto whenever the proto is changed.

Comment 3

[Jesse Ruderman]

I'm not very familiar with Mochitest, but it has something called "SpecialPowers" that you can probably use. It's similar to my extension's "fuzzPriv".

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

The previous patch was based off the incremental GC branch. This one should apply to m-c.

Bobby, this is the write barrier patch we talked about. It makes the write barrier code a little more explicit.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 581018 [details] [diff] [review]
patch v2

>+    void writeBarrierPre(JSRuntime *rt) {
>+        JS_ModifyReference((void **) &value, value);
>+    }

It's not immediately clear to me why this is called "writeBarrierPre". Why not writeBarrier?

>-    if (JS_GetIncrementalGCTracer(rt))
>+    if (JS_IsWriteBarrierNeeded(rt))
>         mScriptableInfo->Mark();

>     // Write barrier for incremental GC.
>     if (HasProto()) {
>         JSRuntime* rt = GetRuntime()->GetJSRuntime();
>-        if (JS_GetIncrementalGCTracer(rt))
>-            GetProto()->TraceJS(JS_GetIncrementalGCTracer(rt));
>+        GetProto()->WriteBarrier(rt);

It would be nice if we could be consistent about whether we want to do this kind of "arms-length" check. Is the situation such that we don't want to call Mark() on an XPCNativeScriptable when we're not in an incremental marking period? If so, it looks like the call to GetProto->WriteBarrier() is going to do just that (see the bottom of the patch).

>+    void WriteBarrier(JSRuntime *rt)
>+    {
>+        mGlobalJSObject.writeBarrierPre(rt);
>+        if (mPrototypeJSObject.get())
>+            mPrototypeJSObject.writeBarrierPre(rt);
>+        if (mPrototypeJSFunction.get())
>+            mPrototypeJSFunction.writeBarrierPre(rt);
>+    }
>+

It seems silly to have to check whether the pointer is null all the time. Can we make writeBarrier a no-op on null heap pointers and remove the checks?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This no longer reproduces with incremental GC landing. I remember realizing a while ago that this bug was actually a problem where the write barrier verifier asserted even though nothing was actually wrong. The verifier has changed a lot since then, so hopefully this is fixed.