Closed Bug 1329113 Opened 3 years ago Closed 3 years ago

AutoCloseIterator::~AutoCloseIterator ignores the return value of CloseIterator

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

Details

Attachments

(1 file)

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.
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.
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.
Changed AutoCloseIterator to receive PropertyIteratorObject*, and put CloseIterator into MOZ_ALWAYS_TRUE, in AutoCloseIterator and Reify, where object is always PropertyIteratorObject.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8841304 - Flags: review?(sphink)
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+
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)
https://hg.mozilla.org/mozilla-central/rev/81c5898574dc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(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.