Bug 120954

Created attachment 211580 [details] Patch without tests

Attachment 211580 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSPromise.cpp', u'Source/JavaScriptCore/runtime/JSPromise.h', u'Source/JavaScriptCore/runtime/JSPromiseCallback.cpp', u'Source/JavaScriptCore/runtime/JSPromiseCallback.h', u'Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromisePrototype.h', u'Source/JavaScriptCore/runtime/JSPromiseResolver.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolver.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverConstructor.h', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.cpp', u'Source/JavaScriptCore/runtime/JSPromiseResolverPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h']" exit_code: 1 Source/JavaScriptCore/runtime/JSPromise.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Source/JavaScriptCore/runtime/JSPromise.cpp:196: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/JavaScriptCore/runtime/JSPromise.cpp:301: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSPromise.cpp:384: This { should be at the end of the previous line [whitespace/braces] [4] Source/JavaScriptCore/runtime/JSPromise.cpp:453: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1801303

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1877171

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1896125

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1808183

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1860388

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1808258

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1861462 New failing tests: js/Promise-catch.html js/Promise-then-without-callbacks-in-workers.html js/Promise-already-resolved.html js/Promise-reject.html js/Promise-simple-fulfill-inside-callback.html js/Promise-simple-in-workers.html js/Promise-fulfill-in-workers.html js/Promise-fulfill.html js/Promise-resolve-with-then-fulfill.html js/Promise-resolve-with-then-reject.html js/Promise-init.html js/Promise-then.html js/Promise-static-fulfill.html js/Promise-resolve-with-then-exception.html js/Promise-then-without-callbacks.html js/Promise-resolve-in-workers.html js/Promise-static-resolve.html js/Promise-chain.html js/Promise-static-reject.html js/Promise-simple-fulfill.html js/Promise-then-in-workers.html js/Promise-already-fulfilled.html js/Promise-simple.html js/Promise-resolve.html js/Promise-init-in-workers.html js/Promise-catch-in-workers.html js/Promise-resolve-chain.html js/Promise-exception.html js/Promise-already-rejected.html js/Promise-reject-in-workers.html

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

Comment on attachment 211580 [details] Patch without tests Attachment 211580 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1848401 New failing tests: js/Promise-catch.html js/Promise-then-without-callbacks-in-workers.html js/Promise-already-resolved.html js/Promise-reject.html js/Promise-simple-fulfill-inside-callback.html js/Promise-simple-in-workers.html js/Promise-fulfill-in-workers.html js/Promise-fulfill.html js/Promise-resolve-with-then-fulfill.html js/Promise-resolve-with-then-reject.html js/Promise-init.html js/Promise-then.html js/Promise-static-fulfill.html js/Promise-resolve-with-then-exception.html js/Promise-then-without-callbacks.html js/Promise-resolve-in-workers.html js/Promise-static-resolve.html js/Promise-chain.html js/Promise-static-reject.html js/Promise-simple-fulfill.html js/Promise-then-in-workers.html js/Promise-already-fulfilled.html js/Promise-simple.html js/Promise-resolve.html js/Promise-init-in-workers.html js/Promise-catch-in-workers.html js/Promise-resolve-chain.html js/Promise-exception.html js/Promise-already-rejected.html js/Promise-reject-in-workers.html

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

Created attachment 220098 [details] Patch

Comment on attachment 220098 [details] Patch Attachment 220098 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/4754670799028224

Comment on attachment 220098 [details] Patch Attachment 220098 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6181006084145152

Comment on attachment 220098 [details] Patch Attachment 220098 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/5264018486853632

Comment on attachment 220098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220098&action=review > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:-32 > -#include "JSCJSValueInlines.h" Looks like this needs to be added back. As far as I can tell, removal of this is why the builds are failing.

Comment on attachment 220098 [details] Patch Attachment 220098 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5493989826363392

Comment on attachment 220098 [details] Patch Attachment 220098 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5423879048658944

Comment on attachment 220098 [details] Patch Attachment 220098 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5630655954157568

Created attachment 220113 [details] Patch

Comment on attachment 220113 [details] Patch Attachment 220113 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5914771530448896

Comment on attachment 220113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220113&action=review > LayoutTests/crypto/subtle/resources/common.js:116 > - resolver.fulfill(results); > + resolver(results); Can we just delete this whole Promise.all polyfill now?

Comment on attachment 220113 [details] Patch Attachment 220113 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4750212354539520

Created attachment 220116 [details] Patch

(In reply to comment #23) > (From update of attachment 220113 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220113&action=review > > > LayoutTests/crypto/subtle/resources/common.js:116 > > - resolver.fulfill(results); > > + resolver(results); > > Can we just delete this whole Promise.all polyfill now? No, Promise.all() and Promise.race() are still not implemented (an attempt to keep things small that has not worked out all that well). I will do them in a follow up.

Comment on attachment 220116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220116&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.h:113 > -class TaskContext : public RefCounted<TaskContext> { > +class MicroTask : public RefCounted<MicroTask> { > public: > - virtual ~TaskContext() > + virtual ~MicroTask() > { > } > + > + virtual void run(ExecState*) = 0; > }; You could be a bro and move this to a separate file while you're at it. > Source/JavaScriptCore/runtime/JSPromise.cpp:91 > + for (size_t i = 0; i < thisObject->m_resolveReactions.size(); ++i) > + visitor.append(&thisObject->m_resolveReactions[i]); > + for (size_t i = 0; i < thisObject->m_rejectReactions.size(); ++i) > + visitor.append(&thisObject->m_rejectReactions[i]); You could be a bro and give SlotVisitor an STL-style append(beginIterator, endIterator) helper. And then use that here. > Source/JavaScriptCore/runtime/JSPromise.cpp:161 > + // 1. Repeat for each reaction in reactions, in original insertion order > + for (auto& reaction : reactions) { > + // i. Call QueueMicrotask(ExecutePromiseReaction, (reaction, argument)). > + globalObject->queueMicroTask(createExecutePromiseReactionMicroTask(vm, reaction.get(), argument)); This is super scary. You've swapped the reactions array, which holds pointers to the heap, into a stack-allocated vector, which then points to a C-heap-allocated buffer of GC-invisible pointers to the GC heap. And then you proceed to loop over something using some C++ hipsterishness and repeatedly call JSGlobalObject methods. The following will likely eventually happen if it hasn't happened already: - Whatever methods the C++ compiler secretly calls as part of the for loop iteration will accidentally trigger a GC. - JSGlobalObject::queueMicroTask() will trigger a GC. - createExecutePromiseReactionMicroTask() will trigger a GC. I think that you probably already have a bug here, and even if you don't, you're creating a high risk of buggishness in the future. I think that it's good practice to either: - Ensure that all of your heap pointers are always in something that is reachable from roots. This means that Vector<WriteBarrier<>> is a terrible idea unless that vector could at any instant be traced from some visitChildren method on some object that is visible from the stack. This isn't the case here since you're swapping into a local vector. - Ensure that during the span of time where the Vector<WriteBarrier<>> is live, you're only doing obviously-not-GCing things. To me this means sticking to the C subset of C++ and putting yourself into the same kind of mentality that one would have when writing a kernel interrupt handler. This isn't the case here since you're doing a lot of exotic methods. To flip this around, if you're hacking some function inside JSC you probably want to be able to assume that you can allocate objects. This is almost always true, so it's common to see patches that add new allocations, and that's OK. But this little bit of code makes that not the case: if you add an allocation to JSGlobalObject::queueMicroTask(), createExecutePromiseReactionMicroTask(), or whatever C++ secretly calls for that for loop, then you're dead. - Use my best friend, DeferGC. It's a RAII-style guard that defers GC around GC-unsafe regions. You could slap that bad boy around JSPromise::reject() or something, and then everything would be fine. The DeferGC approach is kind of dirty so we don't want to be using it everywhere, but it's probably fine here, unless you can find obvious ways to either make the vector reachable in GC or to make it obvious that a GC cannot ever happen here. > Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:88 > +JSValue getDeferred(ExecState* exec, JSValue C) Can we change the name of this method to reflect the fact that it's got something to do with promises? Right now it's got a very generic-looking signature: JSValue getDeferred(ExecState*, JSValue) Could mean anything... > Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:169 > + // ii. Call QueueMicrotask(ExecutePromiseReaction, (resolveReaction, resolution)). > + globalObject->queueMicroTask(createExecutePromiseReactionMicroTask(vm, resolveReaction, resolution)); Is it MicroTask or Microtask?

R=me but you probably need that one DeferGC.

Committed r161241: <http://trac.webkit.org/changeset/161241>