Closed Bug 1365564 Opened 7 years ago Closed 7 years ago

Intermittent toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js | application crashed [@ JS::Value::toObjectOrNull()] or [@ JSFunction::setExtendedSlot(unsigned long, JS::Value const&)]

Categories

(Core :: JavaScript: GC, defect)

Product:
Core
Component:
JavaScript: GC
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: jonco)

References

Details

(Keywords: crash, intermittent-failure, Whiteboard: [stockwell fixed:product])

Crash Data

Attachments

(2 files)

bug1365564-mark-stack-wrapper-check
958 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
bug1365635-mark-stack-wrapper-check-2
1.29 KB, patch
sfink
: review+
Details | Diff | Splinter Review
Component: JavaScript Engine → JavaScript: GC
https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/js/public/Value.h#651


11:29:39     INFO - GECKO(1989) | Assertion failure: isObjectOrNull(), at /home/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:651
11:30:13     INFO - GECKO(1989) | #01: js::GCMarker::stackContainsCrossZonePointerTo(js::gc::Cell const*) const [js/src/vm/ProxyObject.h:70]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #02: js::gc::detail::CellIsNotGray(js::gc::Cell const*) [js/src/jsgc.cpp:8148]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #03: JSFunction::setExtendedSlot(unsigned long, JS::Value const&) [js/src/gc/Barrier.h:251]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #04: EnqueuePromiseReactionJob(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::PromiseState) [js/src/builtin/Promise.cpp:563]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #05: ResolvePromise(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>, JS::PromiseState) [js/src/builtin/Promise.cpp:843]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #06: FulfillMaybeWrappedPromise(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) [js/src/builtin/Promise.cpp:658]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #07: ResolvePromiseInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) [js/src/builtin/Promise.cpp:418]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #08: ResolvePromiseFunction(JSContext*, unsigned int, JS::Value*) [js/src/builtin/Promise.cpp:465]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #09: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:294]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #10: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:470]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #11: <name omitted> [js/src/vm/Interpreter.cpp:534]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #12: js::fun_call(JSContext*, unsigned int, JS::Value*) [js/src/jsfun.cpp:1229]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #13: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:294]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #14: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:470]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #15: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:521]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #16: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:410]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #17: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #18: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:521]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #19: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:410]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #20: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #21: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:521]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #22: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:410]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #23: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #24: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:521]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #25: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:410]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #26: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #27: <name omitted> [js/src/vm/Interpreter.cpp:534]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #28: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2832]
11:30:13     INFO - 
11:30:13     INFO - GECKO(1989) | #29: nsXPCComponents_Utils::CallFunctionWithAsyncStack(JS::Handle<JS::Value>, nsIStackFrame*, nsAString const&, JSContext*, JS::MutableHandle<JS::Value>) [js/xpconnect/src/XPCComponents.cpp:2712]
11:30:13     INFO -
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=windows%20x64%20debug%20bc1%20e10s&tochange=159f82e6813c4dca0a72c1fa2a49ec99b087db32&fromchange=fb689ff5ec1955ea014734027457a95af1764208 suggests this failure started with https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=159f82e6813c4dca0a72c1fa2a49ec99b087db32, bug 1365362 / bug 1189822.

I don't see an obvious logical connection between those bugs and this failure. Also, there are other similar failures, like bug 1366083 that started around the same time but apparently have different regression ranges.
See Also: → 1366083
Hmm.  So GCMarker::stackContainsCrossZonePointerTo (which is debug-only) does:

        auto source = iter.peekPtr().as<JSObject>();
...
        if ((source->is<ProxyObject>() && source->as<ProxyObject>().target() == target) ||
            Debugger::isDebuggerCrossCompartmentEdge(source, target))


Now ProxyObject::target() does:

    JSObject* target() const {
        return const_cast<ProxyObject*>(this)->private_().toObjectOrNull();
    }

where:

    const Value& private_() {
        return GetProxyPrivate(this);
    }

but nothing says that the private slot of a ProxyObject has to be an object, or null, or indeed anything.  It's a _private_ slot.  For DOM proxies it can be an ObjectValue, or a PrivateValue, or an UndefinedValue.

The changes in bug 1189822 made the UndefinedValue case more common and the ObjectValue case less common.  But this code seems wrong a priori: it should be checking js::IsWrapper() before poking at target(), no?  Or do some other check for a ProxyObject whose private is really expected to be an object-or-null.

That said, there might be something wrong with the changes in bug 1189822 too, of course...
Flags: needinfo?(sphink)
Flags: needinfo?(jcoppeard)
(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #3)
> nothing says that the private slot of a ProxyObject has to be an object,
> or null, or indeed anything. 

Oh, I didn't realise that.
Assignee: nobody → jcoppeard
Flags: needinfo?(sphink)
Flags: needinfo?(jcoppeard)
Not all proxy objects have a target object and we only care about CCWs here anyway so call IsCrossCompartmentWrapper rather than is<ProxyObject>().
Attachment #8870769 - Flags: review?(sphink)
Attachment #8870769 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28459b0a0f07
Don't try and get target object from non-wrapper proxies r=sfink
Looks like this patch just morphed the signature.
https://treeherder.mozilla.org/logviewer.html#?job_id=101601246&repo=mozilla-inbound
Keywords: leave-open
Summary: Intermittent toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js | application crashed [@ JS::Value::toObjectOrNull()] → Intermittent toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js | application crashed [@ JS::Value::toObjectOrNull()] or [@ JSFunction::setExtendedSlot(unsigned long, JS::Value const&)]
See Also: → 1367815
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
Oh, it seems like non-CCWs can contain cross compartment pointers.  I filed bug 1367815 to investigate that.  In the mean time I'll change the condition again to check proxy targets, but only if they're objects.
Whiteboard: [stockwell needswork]
Comment on attachment 8871351 [details] [diff] [review]
bug1365635-mark-stack-wrapper-check-2

Can you review if the try run looks OK?
Attachment #8871351 - Flags: review?(sphink)
Comment on attachment 8871351 [details] [diff] [review]
bug1365635-mark-stack-wrapper-check-2

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

::: js/src/gc/Marking.cpp
@@ +2612,5 @@
> +        if (source->is<ProxyObject>()) {
> +            Value value = source->as<ProxyObject>().private_();
> +            if (value.isObject() && &value.toObject() == target)
> +                return sourceZone;
> +        }

I think this is good. ProxyObject::trace contains an unconditional

    TraceCrossCompartmentEdge(trc, obj, proxy->slotOfPrivate(), "private");

TraceCrossCompartmentEdge does the equivalent of

  Value value = source->as<ProxyObject>().private_();
  if (value.isGCThing()) {
    Cell* cell = value.toGCThing();
    if (cell->isTenured())
      /* check color */
    else
      /* assume black */
  }

so in this case you could do

  if (value.isGCThing() && value.toGCThing() == target)

but given that target is a JSObject*, they are functionally equivalent.
Attachment #8871351 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e64cc963fa
Fix GCMarker::stackContainsCrossZonePointerTo to check all proxies for cross compartment target objects r=sfink
The signature has shifted to bug 1368446 now. I don't see much value in having two bugs open to track essentially the same issue, so I'm duping this over.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
I don't think this is a duplicate.  They are different assertions failures.  The stacks are the same but I think this assertion failure was just masking the other one.
Resolution: DUPLICATE → FIXED
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → fixed
status-firefox-esr52: --- → unaffected
Keywords: leave-open
Target Milestone: --- → mozilla55
Whiteboard: [stockwell needswork] → [stockwell fixed:product]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: