Objects emulating undefined not handled properly in BCE::emitAsyncIterator

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment)

obj[@@asyncIterator] is compared == to |undefined|, which lets document.all-alikes slip through.  This needs to be compared strictly against null and undefined both, as in bug 1408740 (during which bug-fixing I found this one).
No test upstreamed for this yet.  Depends on that other patch landing first, natch.
Attachment #8919555 - Flags: review?(andrebargull)
Comment on attachment 8919555 [details] [diff] [review]
Probable patch

Review of attachment 8919555 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7023,4 @@
>          return false;
>  
>      IfThenElseEmitter ifAsyncIterIsUndefined(this);
> +    if (!emitPushNotUndefinedOrNull())                            // OBJ ITERFN !UNDEF-OR-NULL

Hmm, I understand why you've chosen to use "!UNDEF-OR-NULL" here instead of "NOT-UNDEF-OR-NULL", but nonetheless I'm wondering if it's preferable to use "NOT-UNDEF-OR-NULL" for consistency...
Attachment #8919555 - Flags: review?(andrebargull) → review+
Yeah.  An arguably over-engineered solution would be to make the function *configurably* return undef-or-null or !undef-or-null by spewing stricteq/strictne and and/or as appropriate, but it seems probably overkill.

Upstream PR for a test is https://github.com/tc39/test262/pull/1299 which I tested (for future reference) using

  ./test262-update.py --local /home/jwalden/moz/test262/ --branch getiterator-async-funky

and which failed pre-patch and succeeds post-patch.

I'll push these two emulate-undefined patches once this bug's PR is merged.
Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/adf632d02408a8434d08a48bbb9776a975d1b830

There are some further quirks to this, that don't cast shade on this patch or the upstream test PR.  See bug 1408740 comment 8.  As regards this specific test, |iter[@@asyncIterator]| is called with no arguments passed, so should return null per specified HTML semantics that we don't implement, which triggers a very-slightly-later thrown TypeError.  However, the test also accepts that thrown TypeError as a success case, so the upstream PR is still a perfectly correct test, and will remain so when we fix Gecko semantics for calling |document.all|.  With that in mind, we're done here.
https://hg.mozilla.org/mozilla-central/rev/adf632d02408
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.