Bug 136056

Created attachment 236794 [details] the wrong approach I initially tried to do a direct fix to the alleged bug by having dynamic checks to see if the base matched. Then I realized that this is not needed. But, I'm saving this approach here just in case.

Created attachment 236795 [details] the patch

Comment on attachment 236795 [details] the patch r=me

This patch was landed in r172741: <http://trac.webkit.org/r172741>. And then rolled out in r172742: <http://trac.webkit.org/r172742> due to https://bugs.webkit.org/show_bug.cgi?id=136058.

> It's actually perfectly safe to issue a get_direct_pname for the wrong base because of the structure check. It's true that it's safe, but it's probably not desirable.

(In reply to comment #5) > > It's actually perfectly safe to issue a get_direct_pname for the wrong base because of the structure check. > > It's true that it's safe, but it's probably not desirable. It is desirable. For starters, it's completely sound. Given some structure S and some string foo, we know that there exists an offset such that any access o[foo] where o has structure S can be performed by just using that offset. Also, it is very much desirable because there does not exist any other alternative for how to make for-in fast. There is no point in trying to statically prove whether the base matches. In no realistic situation will we have an efficient mechanism for detecting if the base matches, other than doing a dynamic base check, which would obviously be worse than the structure check that we are doing anyway. Consider some examples: FOR-IN WITH LOCAL VARIABLE BASE for (var s in o) { o[s] } Except for very trivial cases, we cannot be sure that the body of the for loop has a reassignment of o, or that this reassignment actually interferes with the optimization: for (var s in o) { o[s] o = foo; } In SSA form - which our bytecode doesn't have - we could implement a cheap check for this. In our bytecode's 3AC form we would have to do some additional analysis to ensure that no such statement is present. This additional analysis would have no positive effect on the system because it would not increase soundness or performance. It would cost additional time during bytecode parsing, but it would achieve nothing, since the structure check was already adequate. FOR-IN WITH NON-LOCAL-VARIABLE BASE for (var s in o.f.f.f) { o.f.f.f[s] } You have two options when bytecompiling o.f.f.f[s]: - Do what we have always done, which is emit a get_direct_pname. This is obviously the right choice for this program. - Emit a raw get_by_val. This is never better. get_by_val with a string base is horribly expensive; it's guaranteed to be an out-of-line call. get_direct_pname will make that out-of-line call, but only after doing a structure check. That structure check is essentially free if you consider the relative expense of the out-of-line get_by_val that we would do anyway. Hence, get_direct_pname is always the better choice, if you know that the subscript of the access is the iterator variable of a for-in loop and the other conditions for get_direct_pname hold. Finally, in the higher tiers, when we have additional profiling to tell us what is going on, it is always beneficial to have a get_by_val with a for-in iterator subscript to be able to refer to the for-in loop's index and enumerator object. That's all that get_direct_pname means: it's semantically a get_by_val with a hint that says "by the way, I know statically that the subscript string emerged from an enumerator during some enumeration index". The compiler can still turn it into a direct out-of-line get_by_val call if it believes that the base object will never have the right structure.

Created attachment 236836 [details] the patch

Comment on attachment 236836 [details] the patch r=me

Comment on attachment 236836 [details] the patch Attachment 236836 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5342671258255360 New failing tests: http/tests/security/cross-frame-access-enumeration.html

Created attachment 236844 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5

(In reply to comment #9) > (From update of attachment 236836 [details]) > Attachment 236836 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/5342671258255360 > > New failing tests: > http/tests/security/cross-frame-access-enumeration.html Looks like this just needs a rebase because the number of calls into the DOM has changed and so the number of console messages about security stuff has now changed.

Comment on attachment 236836 [details] the patch Attachment 236836 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4688001472921600 New failing tests: http/tests/security/cross-frame-access-enumeration.html

Created attachment 236848 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Landed in http://trac.webkit.org/changeset/172794

(In reply to comment #14) > Landed in http://trac.webkit.org/changeset/172794 It broke zillion tests on the Apple Mac 32 bit bot:http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/3680/steps/webkit-32bit-jsc-test/logs/stdio

(In reply to comment #15) > (In reply to comment #14) > > Landed in http://trac.webkit.org/changeset/172794 > > It broke zillion tests on the Apple Mac 32 bit bot:http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/3680/steps/webkit-32bit-jsc-test/logs/stdio And by "zillion", you of course mean "two newly added tests in this revision".

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > Landed in http://trac.webkit.org/changeset/172794 > > > > It broke zillion tests on the Apple Mac 32 bit bot:http://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/3680/steps/webkit-32bit-jsc-test/logs/stdio > > And by "zillion", you of course mean "two newly added tests in this revision". :) I checked only the number: "Rsults for JSC stress tests: 22 failures found.", but you are true, zillion means _now_ the only two new tests in every config. But it doesn't mean if they aren't fail and you aren't responsible to fix them.

The new bug report to the track the new failures: https://bugs.webkit.org/show_bug.cgi?id=136124