Bug 154475

Summary: Builtins that should not rely on iteration do.
Product: WebKit Reporter: Mark S. Miller <erights>
Component: JavaScriptCoreAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, erights, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Open this web page in Safari and Webkit Nightly, and open the console
none
Screenshot showing Safari working and WebKit failing the badbind.html test for this bug
none
Patch
none
Benchmark results none

Description Mark S. Miller 2016-02-19 14:52:11 PST
Created attachment 271806 [details]
Open this web page in Safari and Webkit Nightly, and open the console

Now that the latest WebKit seems to mostly work again on the SES selftest page
https://rawgit.com/tvcutsem/es-lab/master/src/ses/contract.html
I was seeing some new failures. But this time I was able to debug (thanks!)
and isolate the problem.

The new problem is that the builtins used by
http://wiki.ecmascript.org/doku.php?id=conventions:safe_meta_programming
all of which redate ES6 now depend on iterator.next behavior in violation of the ES6 spec.
You can see it yourself by opening the badbind.html in Safari and Webkit Nightly, opening
the console, and seeing what's there. Works fine on Safari and on every other browser I
tested. Fails on the latest WebKit nightly.

Note that I'll be updating the SES selftest page to stop removing .next, because of this
bug, so the SES selftest page will stop being a good test.
Comment 1 Mark S. Miller 2016-02-19 14:53:57 PST
Created attachment 271807 [details]
Screenshot showing Safari working and WebKit failing the badbind.html test for this bug
Comment 2 Keith Miller 2016-02-19 15:09:14 PST
I think this is because of my patch (http://trac.webkit.org/changeset/196734). I didn't realize that arguments iterator was observable from outside it's scope. Working on this now.
Comment 3 Mark S. Miller 2016-02-19 15:58:36 PST
Although this specific test passes on all other browsers, note that Chrome50 Canary has a similar but different problem: 
https://bugs.chromium.org/p/v8/issues/detail?id=4769
Comment 4 Keith Miller 2016-02-22 09:41:37 PST
Created attachment 271926 [details]
Patch
Comment 5 Keith Miller 2016-02-22 11:08:36 PST
Created attachment 271931 [details]
Benchmark results
Comment 6 Geoffrey Garen 2016-02-22 11:38:07 PST
Comment on attachment 271926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271926&action=review

r=me

> Source/JavaScriptCore/builtins/FunctionPrototype.js:33
> +        @putByValDirect(argumentValues, i-1, arguments[i]);

Spacing around - 1.
Comment 7 WebKit Commit Bot 2016-02-22 11:42:58 PST
Comment on attachment 271926 [details]
Patch

Clearing flags on attachment: 271926

Committed r196949: <http://trac.webkit.org/changeset/196949>
Comment 8 WebKit Commit Bot 2016-02-22 11:43:03 PST
All reviewed patches have been landed.  Closing bug.