Bug 120954 – Update Promises to the https://github.com/domenic/promises-unwrapping spec

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Sam Weinig]

The old (relative term) Promise spec from the DOM is being replaced by https://github.com/domenic/promises-unwrapping.  We should update.

[Sam Weinig]

Created attachment 211580 [details]
Patch without tests

[WebKit Commit Bot]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[kov's GTK+ EWS bot]

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

[Build Bot]

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

[EFL EWS Bot]

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

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

Created attachment 220098 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220098 [details]
Patch

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

[Build Bot]

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

[EFL EWS Bot]

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

[Darin Adler]

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.

[Build Bot]

Comment on attachment 220098 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 220098 [details]
Patch

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

[Build Bot]

Comment on attachment 220098 [details]
Patch

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

[Sam Weinig]

Created attachment 220113 [details]
Patch

[Build Bot]

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

[Alexey Proskuryakov]

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?

[Build Bot]

Comment on attachment 220113 [details]
Patch

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

[Sam Weinig]

Created attachment 220116 [details]
Patch

[Sam Weinig]

(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.

[Filip Pizlo]

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?

[Filip Pizlo]

R=me but you probably need that one DeferGC.

[Sam Weinig]

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