Closed
Bug 1329113
Opened 7 years ago
Closed 7 years ago
AutoCloseIterator::~AutoCloseIterator ignores the return value of CloseIterator
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: arai, Assigned: arai)
Details
Attachments
(1 file)
2.02 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
https://dxr.mozilla.org/mozilla-central/rev/b548da4e16f067e5b69349376e37b2db97983cf7/js/src/proxy/CrossCompartmentWrapper.cpp#253 > ~AutoCloseIterator() { if (obj) CloseIterator(cx, obj); } If it never fails, it needs MOZ_ALWAYS_TRUE or something. if it can fail, we should handle the case.
Comment 1•7 years ago
|
||
It can fail. CloseIterator can call LegacyGeneratorObject::close can call GlobalObject::getIntrinsicValue which can fail. It can probably fail when it Calls the LegacyGeneratorCloseInternal intrinsic, whatever that is.
Assignee | ||
Comment 2•7 years ago
|
||
https://dxr.mozilla.org/mozilla-central/rev/a08ec245fa24d573fc99e81210ecc09de734cdd3/js/src/proxy/CrossCompartmentWrapper.cpp#286 > static bool > Reify(JSContext* cx, JSCompartment* origin, MutableHandleObject objp) > { > Rooted<PropertyIteratorObject*> iterObj(cx, &objp->as<PropertyIteratorObject>()); > NativeIterator* ni = iterObj->getNativeIterator(); > > AutoCloseIterator close(cx, iterObj); AutoCloseIterator is used only here, so `iterObj` is PropertyIteratorObject, and CloseIterator uses infallible path: https://dxr.mozilla.org/mozilla-central/rev/a08ec245fa24d573fc99e81210ecc09de734cdd3/js/src/jsiter.cpp#1195-1210 > bool > js::CloseIterator(JSContext* cx, HandleObject obj) > { > if (obj->is<PropertyIteratorObject>()) { > /* Remove enumerators from the active list, which is a stack. */ > NativeIterator* ni = obj->as<PropertyIteratorObject>().getNativeIterator(); > > if (ni->flags & JSITER_ENUMERATE) { > ni->unlink(); > > MOZ_ASSERT(ni->flags & JSITER_ACTIVE); > ni->flags &= ~JSITER_ACTIVE; > > /* > * Reset the enumerator; it may still be in the cached iterators > * for this thread, and can be reused. > */ > ni->props_cursor = ni->props_array; > } So, maybe we should just change AutoCloseIterator to receive PropertyIteratorObject*, and then use MOZ_ALWAYS_TRUE for CloseIterator call. so that the fallible path for LegacyGeneratorObject won't be taken.
Assignee | ||
Comment 3•7 years ago
|
||
Changed AutoCloseIterator to receive PropertyIteratorObject*, and put CloseIterator into MOZ_ALWAYS_TRUE, in AutoCloseIterator and Reify, where object is always PropertyIteratorObject.
Comment 4•7 years ago
|
||
Comment on attachment 8841304 [details] [diff] [review] Use PropertyIteratorObject* in AutoCloseIterator. Review of attachment 8841304 [details] [diff] [review]: ----------------------------------------------------------------- Oh, I see. That seems fine.
Attachment #8841304 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 5•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/81c5898574dcd877c0d42b5554ee3f941d6ba55e Bug 1329113 - Use PropertyIteratorObject* in AutoCloseIterator. r=sfink
Comment 6•7 years ago
|
||
The last inbound -> central merge didn't get the bugs marked as fixed (this one was the last bug from that merge)
Flags: needinfo?(cbook)
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81c5898574dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 8•7 years ago
|
||
(In reply to Guilherme Lima from comment #6) > The last inbound -> central merge didn't get the bugs marked as fixed (this > one was the last bug from that merge) Hi Guilherme yeah that was because there was a problem with treeherder and i waited till this was fixed. Should be ok now :)
Flags: needinfo?(cbook)
You need to log in
before you can comment on or make changes to this bug.
Description
•