New rooting hazards from ~AutoCompartment

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: terrence)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 1

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

Comment 2

6 years ago
Created attachment 777904 [details] [diff] [review]
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".
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #777904 - Flags: review?(bhackett1024)
Flags: needinfo?(bhackett1024)
(Assignee)

Updated

6 years ago
Blocks: 753203
Attachment #777904 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/480852219d50
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.