Summary: Crash generating object preview for ArrayIterator Test: <script> (function() { try { var iter = [7][Symbol.iterator](); debugger; iter['return'] = function() {}; Array.from(iter, function(){ throw 2; }); } catch(e) { /* empty */ } })(); </script> Steps to reproduce: 1. Inspect test page 2. Reload (to trigger debugger) 3. Step through code => Crash after the `iter["return"]` line. Notes: - JSInjectedScriptHost::iteratorEntries clones the ArrayIterator and reuses the structure but doesn't fill in all properties that are important for the iterable operations (namely the `return` function it claims to implement).
<rdar://problem/32859012>
Created attachment 313688 [details] [PATCH] Proposed Fix
Comment on attachment 313688 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313688&action=review > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:530 > + clone->putDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName(), iteratorObject->getDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName())); When is this called? What if the iterator object gets mutated by user code? What I’m worried about is a user adds random properties to this object, and then we still end up with properties that are zero filled that we didn’t expect to exist. If this is even possible, I think the best way to proceed would be to just start off the result with the empty structure, and add the properties allowing the object’s structure to transition.
Comment on attachment 313688 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313688&action=review >> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:530 >> + clone->putDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName(), iteratorObject->getDirect(vm, vm.propertyNames->builtinNames().arrayIteratorKindPrivateName())); > > When is this called? What if the iterator object gets mutated by user code? What I’m worried about is a user adds random properties to this object, and then we still end up with properties that are zero filled that we didn’t expect to exist. If this is even possible, I think the best way to proceed would be to just start off the result with the empty structure, and add the properties allowing the object’s structure to transition. This is called only in this file. This object itself should not be exposed to user code. It is created here, iterated up to 5 times, and closed. The idea here is that Web Inspector wants to peek and show the next 5 elements in an iterator. Since "Array Iterator" and other native iterators are guaranteed to have side-effect free iteration we can peek upcoming values by cloning the iterator instance, walk the clone forward a few values, and then getting rid of the clone. Array Iterator, unlike Map Iterator, Set Iterator, etc, is different in that it is implemented in JavaScript built-ins. But we can identify one with PrivateNames. This is our implementation of cloning an Array Iterator. If you see a way that user code can access this object then we might have to abandon this approach or seek an alternative. Maybe that is possible if someone mutates the ArrayIterator prototype? Namely `[].values().__proto__.next`? Starting off with an empty structure won't get us the necessary `next` function.
Comment on attachment 313688 [details] [PATCH] Proposed Fix After talking with Saam going to go a safer route: - Only peek iterators when not observable (no user code) - Do a better job cloning ArrayIterators
Created attachment 313767 [details] [PATCH] Proposed Fix
Created attachment 313768 [details] [PATCH] Proposed Fix
Comment on attachment 313768 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313768&action=review > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:557 > + // FIXME: Reflect.enumerate iteration can be observable, but it is also removed from ECMAScript. I am removing this in: <https://webkit.org/b/173806> Remove Reflect.enumerate
Comment on attachment 313768 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313768&action=review r=me > Source/JavaScriptCore/ChangeLog:9 > + When Inspector generates an object preview for an ArrayIterator instance it made I like how this patch turned out > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:527 > + JSObject* clone = constructEmptyObject(exec, ArrayIteratorPrototype::create(vm, globalObject, ArrayIteratorPrototype::createStructure(vm, globalObject, globalObject->iteratorPrototype()))); Does the global object not cache this structure?
> > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:527 > > + JSObject* clone = constructEmptyObject(exec, ArrayIteratorPrototype::create(vm, globalObject, ArrayIteratorPrototype::createStructure(vm, globalObject, globalObject->iteratorPrototype()))); > > Does the global object not cache this structure? It doesn't. Want me to add it?
<https://trac.webkit.org/r218836>
(In reply to Joseph Pecoraro from comment #10) > > > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:527 > > > + JSObject* clone = constructEmptyObject(exec, ArrayIteratorPrototype::create(vm, globalObject, ArrayIteratorPrototype::createStructure(vm, globalObject, globalObject->iteratorPrototype()))); > > > > Does the global object not cache this structure? > > It doesn't. Want me to add it? I'm not sure what you went with. It's probably better in the long run, but not essential for this code.