Bug 120954 - Update Promises to the https://github.com/domenic/promises-unwrapping spec
Summary: Update Promises to the https://github.com/domenic/promises-unwrapping spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on: 121167
Blocks: 122679
  Show dependency treegraph
 
Reported: 2013-09-07 08:42 PDT by Sam Weinig
Modified: 2014-01-02 16:36 PST (History)
16 users (show)

See Also:


Attachments
Patch without tests (101.06 KB, patch)
2013-09-13 13:22 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (558.44 KB, application/zip)
2013-09-14 11:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (571.40 KB, application/zip)
2013-09-14 12:19 PDT, Build Bot
no flags Details
Patch (299.46 KB, patch)
2013-12-29 20:55 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (299.02 KB, patch)
2013-12-30 09:08 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (299.02 KB, patch)
2013-12-30 10:19 PST, Sam Weinig
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-09-07 08:42:17 PDT
The old (relative term) Promise spec from the DOM is being replaced by https://github.com/domenic/promises-unwrapping.  We should update.
Comment 1 Sam Weinig 2013-09-13 13:22:19 PDT
Created attachment 211580 [details]
Patch without tests
Comment 2 WebKit Commit Bot 2013-09-13 17:05:31 PDT
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 3 Early Warning System Bot 2013-09-13 17:08:57 PDT
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 4 Early Warning System Bot 2013-09-13 17:10:45 PDT
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 5 kov's GTK+ EWS bot 2013-09-13 18:10:58 PDT
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 6 Build Bot 2013-09-13 18:48:16 PDT
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 7 EFL EWS Bot 2013-09-13 22:57:20 PDT
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 8 EFL EWS Bot 2013-09-14 00:52:10 PDT
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 9 Build Bot 2013-09-14 11:17:32 PDT
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
Comment 10 Build Bot 2013-09-14 11:17:34 PDT
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 11 Build Bot 2013-09-14 12:19:00 PDT
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
Comment 12 Build Bot 2013-09-14 12:19:01 PDT
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
Comment 13 Sam Weinig 2013-12-29 20:55:00 PST
Created attachment 220098 [details]
Patch
Comment 14 EFL EWS Bot 2013-12-29 21:06:43 PST
Comment on attachment 220098 [details]
Patch

Attachment 220098 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4754670799028224
Comment 15 Build Bot 2013-12-29 21:31:08 PST
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 16 EFL EWS Bot 2013-12-29 21:38:49 PST
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 17 Darin Adler 2013-12-29 22:09:17 PST
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 18 Build Bot 2013-12-29 22:19:10 PST
Comment on attachment 220098 [details]
Patch

Attachment 220098 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5493989826363392
Comment 19 kov's GTK+ EWS bot 2013-12-29 22:24:03 PST
Comment on attachment 220098 [details]
Patch

Attachment 220098 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5423879048658944
Comment 20 Build Bot 2013-12-29 22:38:59 PST
Comment on attachment 220098 [details]
Patch

Attachment 220098 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5630655954157568
Comment 21 Sam Weinig 2013-12-30 09:08:56 PST
Created attachment 220113 [details]
Patch
Comment 22 Build Bot 2013-12-30 09:44:08 PST
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 23 Alexey Proskuryakov 2013-12-30 09:55:59 PST
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 24 Build Bot 2013-12-30 10:17:45 PST
Comment on attachment 220113 [details]
Patch

Attachment 220113 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4750212354539520
Comment 25 Sam Weinig 2013-12-30 10:19:53 PST
Created attachment 220116 [details]
Patch
Comment 26 Sam Weinig 2013-12-30 10:30:52 PST
(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 27 Filip Pizlo 2014-01-02 11:11:22 PST
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?
Comment 28 Filip Pizlo 2014-01-02 11:11:49 PST
R=me but you probably need that one DeferGC.
Comment 29 Sam Weinig 2014-01-02 16:36:29 PST
Committed r161241: <http://trac.webkit.org/changeset/161241>