'arguments' behave differently in sloppy and strict mode. So if we cache HasOwnProperty('arguments') in strict mode, then try to call it again in sloppy mode (or vice-versa) the result will be wrong. The simplest solution is just not to ever cache HasOwnProperty('arguments'). Adding a bit for sloppy/strict in the keys would also work, but it would complicate things for the common case, and I don't expect HasOwnProperty('arguments') to be super common.
rdar://problem/50202206
Created attachment 369178 [details] Patch
(In reply to Robin Morisset from comment #0) > 'arguments' behave differently in sloppy and strict mode. So if we cache > HasOwnProperty('arguments') in strict mode, then try to call it again in > sloppy mode (or vice-versa) the result will be wrong. > The simplest solution is just not to ever cache HasOwnProperty('arguments'). > Adding a bit for sloppy/strict in the keys would also work, but it would > complicate things for the common case, and I don't expect > HasOwnProperty('arguments') to be super common. Why don't these have different structures?
Comment on attachment 369178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369178&action=review > JSTests/stress/has-own-property-arguments.js:3 > +class A extends Function {} > +new A("'use strict';").hasOwnProperty('arguments'); > +new A().hasOwnProperty('arguments'); I'm super confused what this is doing. This is just calling the constructor with a string as the first argument. Also, classes are always in strict mode. Can you explain what's going on? Can this also test what it expects the results to be? > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:95 > + // We don't cache accesses to arguments, because it depends on whether we are in strict or sloppy mode. > + if (propName == "arguments") > + return; This seems wrong. We should be reflecting this in Structure. It's unlikely the bug is just here and not also in Get
(In reply to Robin Morisset from comment #0) > 'arguments' behave differently in sloppy and strict mode. So if we cache > HasOwnProperty('arguments') in strict mode, then try to call it again in > sloppy mode (or vice-versa) the result will be wrong. > The simplest solution is just not to ever cache HasOwnProperty('arguments'). > Adding a bit for sloppy/strict in the keys would also work, but it would > complicate things for the common case, and I don't expect > HasOwnProperty('arguments') to be super common. Why wouldn't the sloppy-strict change be caught by the cache? A function can't change its mind about whether it is strict or sloppy. Is the issue that function objects for strict functions have the same structure as function objects for sloppy functions?
Function constructor should pick a correct structure, I think this is the issue.
(In reply to Yusuke Suzuki from comment #6) > Function constructor should pick a correct structure, I think this is the > issue. Ah, no. Investigating
(In reply to Yusuke Suzuki from comment #7) > (In reply to Yusuke Suzuki from comment #6) > > Function constructor should pick a correct structure, I think this is the > > issue. > > Ah, no. Investigating The issue is that InternalFunction::createSubclassStructureSlow caches the structure, and when using the previous structure, we are assuming that "If classInfo is the same, we should reuse the cached one". This is wrong for this case.
Created attachment 372671 [details] Patch
Created attachment 372672 [details] Patch
Comment on attachment 372672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372672&action=review r=me > JSTests/stress/has-own-property-arguments.js:8 > +shouldBe(new A().hasOwnProperty('arguments'), true); Can you also add tests that would break get by id inline caching?
Comment on attachment 372672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372672&action=review Thanks! >> JSTests/stress/has-own-property-arguments.js:8 >> +shouldBe(new A().hasOwnProperty('arguments'), true); > > Can you also add tests that would break get by id inline caching? Sure! Added.
Committed r246709: <https://trac.webkit.org/changeset/246709>