895220 - New rooting hazards from ~AutoCompartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

GC Function: int32 JSObject::getElementNoGC(JSContext*, JSObject*, JSObject*, uint32, JS::Value*)
    uint8 js::IndexToIdNoGC(JSContext*, uint32, jsid*)
    uint8 js::IndexToIdSlow(js::ExclusiveContext*, uint32, class js::FakeMutableHandle<jsid>) [with js::AllowGC allowGC = (js::AllowGC)0u; uint32_t = unsigned int; typename js::MaybeRooted<jsid, allowGC>::MutableHandleType = js::FakeMutableHandle<jsid>]
    JSAtom* js::AtomizeChars(js::ExclusiveContext*, uint16*, uint64, uint32) [with js::AllowGC allowGC = (js::AllowGC)0u; jschar = short unsigned int; size_t = long unsigned int]
    jsatom.cpp:JSAtom* AtomizeAndCopyChars(js::ExclusiveContext*, uint16*, uint64, uint32) [with js::AllowGC allowGC = (js::AllowGC)0u; jschar = short unsigned int; size_t = long unsigned int]
    void js::AutoCompartment::~AutoCompartment(int32)
    void JSContext::leaveCompartment(JSCompartment*)
    void JSContext::wrapPendingException()
    uint8 JSCompartment::wrap(JSContext*, class JS::MutableHandle<JS::Value>, class JS::Handle<JSObject*>)
    FieldCall: JSRuntime.preWrapObjectCallback

This is a result of the change:

-    AutoEnterAtomsCompartment ac(cx);
+    AutoCompartment ac(cx, cx->runtime()->atomsCompartment);

AutoEnterAtomsCompartment just called cx->privateSetCompartment(oldCompartment) in its destructor, ignoring any pending exception. ~AutoCompartment wraps a pending exception. An exception is clearly possible, since AtomizeAndCopyChars can directly call js_ReportOutOfMemory, which calls cx->setPendingException(), so it seems like it was just wrong already.

The question is, what should be done in NoGC code in this case? Should a new ~AutoCompartment<NoGC> just clear the pending exception, under the assumption that the user will be returning false/NULL in that case anyway, so the operation will be retried? I'm not familiar with the semantics of this NoGC stuff.

Comment 1

[Terrence Cole [:terrence]]

Last night I was not able to think of anything better than templatizing AtomizeAndCopyChars on AllowGC and making a version of it that returns false instead of OOM.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0 - Fix AtomizeAndCopyChars NoGC version

Actually, it already is templatized on AllowGC, so I think this is just a bug. Here is a one-line "fix".

Comment 3

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/480852219d50

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/480852219d50