<rdar://problem/41646336>
Why?
(In reply to Saam Barati from comment #1) > Why? Because normal JSFunctions can start out with the same structure as a builtin. When in strict mode, the structure for a builtin function will be the same as a normal empty function. If the program invokes hasOwnProperty("prototype") on the builtin first and we cache the result, we will end up caching a false value. However, when the program subsequently invokes hasOwnProperty("prototype") on a normal empty function (e.g. foo = function() {}), it will see the same structure and return the false result in the cache. However, the expected result in this case should be true.
(In reply to Mark Lam from comment #2) > (In reply to Saam Barati from comment #1) > > Why? > > Because normal JSFunctions can start out with the same structure as a > builtin. When in strict mode, the structure for a builtin function will be > the same as a normal empty function. > > If the program invokes hasOwnProperty("prototype") on the builtin first and > we cache the result, we will end up caching a false value. However, when > the program subsequently invokes hasOwnProperty("prototype") on a normal > empty function (e.g. foo = function() {}), it will see the same structure > and return the false result in the cache. However, the expected result in > this case should be true. they shouldn't have the same structure then.
What you're describing is a bug that is not just in this cache, but all our ICs
(In reply to Saam Barati from comment #4) > What you're describing is a bug that is not just in this cache, but all our > ICs I'll investigate further.
Created attachment 344016 [details] proposed patch.
Comment on attachment 344016 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=344016&action=review Nice. r=me > JSTests/stress/regress-187211.js:76 > +} Can we also add a test for getById ICs? I’m guessing they will run into the same problem of producing the wrong result > Source/JavaScriptCore/runtime/JSFunction.cpp:73 > + if (executable->isBuiltinFunction()) { Might be worth an assert above that arrow functions aren’t Builtins? Or can they be if they’re nested in a builtin? Or maybe it’s moot because they don’t have a “prototype” property?
(In reply to Saam Barati from comment #7) > Comment on attachment 344016 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344016&action=review > > Nice. r=me > > > JSTests/stress/regress-187211.js:76 > > +} > > Can we also add a test for getById ICs? I’m guessing they will run into the > same problem of producing the wrong result I think getById is not vulnerable, and I have not been able to write a test to get it to fail. Looking at the code, getById caches an offset only if PropertySlot.isCacheable() is true. In the case of the builtin function's prototype property, isCacheable() is false. However, the HasOwnPropertyCache caches a result value if !(!slot.isCacheable() && !slot.isUnset()) i.e. it caches if slot.isCacheable() || slot.isUnset(). The second condition is what allow the HasOwnProperty result to be cached for the builtin function, and this is the source of the discrepancy that led to this bug. > > Source/JavaScriptCore/runtime/JSFunction.cpp:73 > > + if (executable->isBuiltinFunction()) { > > Might be worth an assert above that arrow functions aren’t Builtins? Or can > they be if they’re nested in a builtin? Or maybe it’s moot because they > don’t have a “prototype” property? My testing shows that there are builtin arrow functions. I think you are right that arrow functions not having the "prototype" property makes the immediate issue moot. However, I think it is fragile to leave it in this state: if anyone adds another lazily reified property, there's a chance that it can become an issue. Hence, I've re-written the code to give builtin arrow functions a different structure as well. I'll upload a patch shortly.
Created attachment 344079 [details] proposed patch.
Comment on attachment 344079 [details] proposed patch. Attachment 344079 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8409883 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html
Created attachment 344085 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344079 [details] proposed patch. Attachment 344079 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8410411 New failing tests: http/tests/security/local-video-source-from-remote.html
Created attachment 344091 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344016 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=344016&action=review >>> JSTests/stress/regress-187211.js:76 >>> +} >> >> Can we also add a test for getById ICs? I’m guessing they will run into the same problem of producing the wrong result > > I think getById is not vulnerable, and I have not been able to write a test to get it to fail. Looking at the code, getById caches an offset only if PropertySlot.isCacheable() is true. In the case of the builtin function's prototype property, isCacheable() is false. > > However, the HasOwnPropertyCache caches a result value if !(!slot.isCacheable() && !slot.isUnset()) i.e. it caches if slot.isCacheable() || slot.isUnset(). The second condition is what allow the HasOwnProperty result to be cached for the builtin function, and this is the source of the discrepancy that led to this bug. Do Builtins have a prototype property or not? It seems like the GetById cache would break when given an unset slot This is what GetById does, which I believe is the same as the own property cache if (!slot.isCacheable() && !slot.isUnset()) return GiveUpOnCache;
(In reply to Saam Barati from comment #14) > Comment on attachment 344016 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344016&action=review > > >>> JSTests/stress/regress-187211.js:76 > >>> +} > >> > >> Can we also add a test for getById ICs? I’m guessing they will run into the same problem of producing the wrong result > > > > I think getById is not vulnerable, and I have not been able to write a test to get it to fail. Looking at the code, getById caches an offset only if PropertySlot.isCacheable() is true. In the case of the builtin function's prototype property, isCacheable() is false. > > > > However, the HasOwnPropertyCache caches a result value if !(!slot.isCacheable() && !slot.isUnset()) i.e. it caches if slot.isCacheable() || slot.isUnset(). The second condition is what allow the HasOwnProperty result to be cached for the builtin function, and this is the source of the discrepancy that led to this bug. > > Do Builtins have a prototype property or not? It seems like the GetById > cache would break when given an unset slot > > This is what GetById does, which I believe is the same as the own property > cache > > if (!slot.isCacheable() && !slot.isUnset()) > return GiveUpOnCache; Thanks for the review. I'll dig further to see what I was missing when I attempted to make the GetById test and failed. Since we know that this patch is fundamentally correct, I'll move forward and land it first. If I can come up with a test for GetById, I'll land that in a follow up. Thanks.
Fix landed in r233426: <http://trac.webkit.org/r233426>.