Closed Bug 895220 Opened 6 years ago Closed 6 years ago

New rooting hazards from ~AutoCompartment

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: sfink, Assigned: terrence)

References

Details

Attachments

(1 file)

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.
Flags: needinfo?(bhackett1024)
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.
Actually, it already is templatized on AllowGC, so I think this is just a bug. Here is a one-line "fix".
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #777904 - Flags: review?(bhackett1024)
Flags: needinfo?(bhackett1024)
Blocks: ExactRooting
Attachment #777904 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/480852219d50
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.