[ABANDONED] Enable V8 Promises

Enable V8 Promises

Enable V8 Promises.
This CL sets --harmony-promises flag.
When V8 Promises are shipped, I will remove it.

BUG=352597, 352552
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170300

[yhirano, 2014-03-25 12:23:28]

As in the description, this CL changes the behavior of ScriptPromise and
ScriptPromiseResolver.

[yhirano, 2014-03-25 12:24:40]

ksakamoto, fast/css/fontface-methods.html fails because of this change.
Are you OK with it?

[jochen (gone - plz use gerrit), 2014-03-25 12:28:05]

As discussed offline, I don't see a way around the micro task issue with the
current api.

Since we have to switch the runtime enabled flag and v8 at the same time,
putting the switch here temporarily is OK.

lgtm

[yhirano, 2014-03-26 01:03:42]

+rafaelw

Remove RunMicrotasks.
https://src.chromium.org/viewvc/blink?revision=169978&view=revision runs
microtasks properly.

[yhirano, 2014-03-26 06:47:16]

+aandley (inspector), ksakamoto (font loading), junov (imagebitmap), eroman
(crypto)
Hi, I am switching the Promises implementation and it breaks some tests.

inspector:
Sorry I don't understand the async callstack and I can't fix it.
Can I just mark it to be skipped?

font loading:
As we discussed today, I fixed the test. PTAL.

imagebitmap:
I fixed one layout test (canvas-createImageBitmap-immutable). PTAL.
Though some more tests fail it seems they are caused by a Promises defect, I
don't fix them for now.

crypto:
Some tests fail because of the task execution order change.
I make the tests less timing dependent. PTAL.

I'm sorry to rush you, but I'm planning to land it before the coming branch
point.
It would be great if you could review this CL today.

Thanks,

[Kunihiko Sakamoto, 2014-03-26 06:53:57]

lgtm for LayoutTests/http/tests/webfont/
Thanks for taking care of the test!

[aandrey, 2014-03-26 08:30:40]

lgtm on skipping the debugger/async-callstack-promises.html test

[eroman, 2014-03-26 09:07:58]

lgtm for crypto tests

https://codereview.chromium.org/209853010/diff/380003/LayoutTests/crypto/reso...
File LayoutTests/crypto/resources/common.js (right):

https://codereview.chromium.org/209853010/diff/380003/LayoutTests/crypto/reso...
LayoutTests/crypto/resources/common.js:144: var promise = eval(code);
In general this change is not a good idea, since "code" may not evaluate the
same if deferred (consider if it depends on globals which have changed since it
was called).

In practice it looks like the existing call sites don't have such issues. And
since I am actively removing this from the tests I am OK with this change.

https://codereview.chromium.org/209853010/diff/380003/LayoutTests/crypto/reso...
LayoutTests/crypto/resources/common.js:162: var promise = eval(code);
Same comment as above.

https://codereview.chromium.org/209853010/diff/380003/LayoutTests/crypto/sign...
File LayoutTests/crypto/sign-verify.html (right):

https://codereview.chromium.org/209853010/diff/380003/LayoutTests/crypto/sign...
LayoutTests/crypto/sign-verify.html:133: allTests = allTests.then(function() {
In fact I have a similar changelist pending which linearizes the completion
order:
https://codereview.chromium.org/199663006/

Feel free to commit yours first though.

Sorry that these caused you trouble. I have been cleaning up the promise
completion order this week but hadn't finished yet.

[Justin Novosad, 2014-03-27 15:23:44]

lgtm for canvas test

[rafaelw1, 2014-03-27 15:24:54]

Note that the v8 microtask patch got reverted last night, so this may need
to wait on that relanding.
On Mar 27, 2014 8:23 AM, <junov@chromium.org> wrote:

> lgtm for canvas test
>
> https://codereview.chromium.org/209853010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yhirano, 2014-03-28 07:29:12]

fast/canvas and fast/workers pass locally with PS16.
This PS includes Rafael's CL.

Waiting for the trybots results...

[yhirano, 2014-03-28 09:09:33]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-28 09:09:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/209853010/710001

[commit-bot: I haz the power, 2014-03-28 09:10:14]

Change committed as 170300

[adamk, 2014-03-28 17:55:05]

This patch looks wrong to me (and it bothers me a bit that it landed without any
apparent review of the changes to, and usages of,  V8RecursionScope).

https://codereview.chromium.org/209853010/diff/710001/Source/bindings/v8/V8Re...
File Source/bindings/v8/V8RecursionScope.cpp (right):

https://codereview.chromium.org/209853010/diff/710001/Source/bindings/v8/V8Re...
Source/bindings/v8/V8RecursionScope.cpp:56: if (!m_context ||
m_context->activeDOMObjectsAreStopped())
How do you know a null context indicates "stopped"?

https://codereview.chromium.org/209853010/diff/710001/Source/bindings/v8/V8Re...
File Source/bindings/v8/V8RecursionScope.h (right):

https://codereview.chromium.org/209853010/diff/710001/Source/bindings/v8/V8Re...
Source/bindings/v8/V8RecursionScope.h:106: ExecutionContext* m_context;
Can you explain why it's safe to store the context here?

https://codereview.chromium.org/209853010/diff/710001/Source/core/workers/Wor...
File Source/core/workers/WorkerRunLoop.cpp (right):

https://codereview.chromium.org/209853010/diff/710001/Source/core/workers/Wor...
Source/core/workers/WorkerRunLoop.cpp:60: V8RecursionScope
recursionScope(workerGlobalScope);
This seems like the wrong place to put this, V8RecursionScope is about script
execution, not general task running. Also, by wrapping the entire performTask
call, if the task dispatches events to multiple listeners, we'll get the wrong
end-of-microtask semantics (each event dispatch should end in performCheckpoint,
not just one for the whole task).

And it feels like layer violation for WorkerRunLoop to know about
V8RecursionScope.

https://codereview.chromium.org/209853010/diff/710001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/209853010/diff/710001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:118: Microtask::performCheckpoint();
I don't understand why this is here.

[adamk, 2014-03-28 17:58:06]

Regarding lack of review, I now see that jochen said "ship it" on
https://code.google.com/p/chromium/issues/detail?id=352552#c31, but it's not at
all clear from this issue that he looked at the code changes.

[jochen (gone - plz use gerrit), 2014-03-28 19:04:20]

On 2014/03/28 17:58:06, adamk wrote:
> Regarding lack of review, I now see that jochen said "ship it" on
> https://code.google.com/p/chromium/issues/detail?id=352552#c31, but it's not
at
> all clear from this issue that he looked at the code changes.

sorry for not leaving a proper audit trail and sorry for the mess.

[adamk, 2014-03-28 20:25:13]

This has been reverted in
http://src.chromium.org/viewvc/blink?view=revision&revision=170358

[rafaelw, 2014-03-29 01:21:53]

Note that the blink usage of the v8 microtask queue has been relanded in
https://codereview.chromium.org/217073004/ (along with the LSAN suppression).

[yhirano, 2014-03-31 02:10:24]

Reopen.

I'm sorry for the confusion.
Removed Rafael's CL and worker related changes.
It doesn't work now.

[jochen (gone - plz use gerrit), 2014-04-07 12:57:30]

Does this work meanwhile? What's missing?

[yhirano, 2014-04-07 13:02:39]

Adam and I are discussing at https://codereview.chromium.org/219003002/

[yhirano, 2014-04-07 13:02:40]

Adam and I are discussing at https://codereview.chromium.org/219003002/

[yhirano, 2014-04-15 08:45:05]

I've updated the CL.
Given that Microtask::performCheckPoint does no longer execute V8 tasks, I
propose the following solution.

When a ScriptPromiseResolverWithContext::resolve (or reject, same applies to the
following), it detects whether it is in a V8 context.
1. If it is inside a V8 context: just call v8::Resolver::resolve. Microtasks
will be executed by V8 automatically.
2. Otherwise: post a task that calls v8::Resolver::resolve and runs microtasks
manually.

I am rewriting callers' side code so that they use
ScriptPromiseResolverWithContext instead of a pair of ScriptPromiseResolver and
NewScriptState.

Jochen, Adam, Rafael, can you take a look at this CL again?

https://codereview.chromium.org/209853010/diff/860001/Source/bindings/v8/Scri...
File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right):

https://codereview.chromium.org/209853010/diff/860001/Source/bindings/v8/Scri...
Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:67:
getNoopFunction(isolate)->Call(v8::Undefined(isolate), 0, args);
I'm wondering if calling v8::V8::RunMicrotasks here works.
If it works, I like it more than the current code because it is more
straitforward.

https://codereview.chromium.org/209853010/diff/860001/Source/bindings/v8/Scri...
File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right):

https://codereview.chromium.org/209853010/diff/860001/Source/bindings/v8/Scri...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:101: ref();
I don't like the manual ref counting, but I couldn't think of a better solution.

[yhirano, 2014-04-15 10:33:39]

https://codereview.chromium.org/209853010/diff/860001/Source/bindings/v8/Scri...
File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right):

https://codereview.chromium.org/209853010/diff/860001/Source/bindings/v8/Scri...
Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:67:
getNoopFunction(isolate)->Call(v8::Undefined(isolate), 0, args);
On 2014/04/15 08:45:06, yhirano wrote:
> I'm wondering if calling v8::V8::RunMicrotasks here works.
> If it works, I like it more than the current code because it is more
> straitforward.
Calling an empty function violates some assertions[1]. now I call
v8::V8::RunMicrotasks directly.

1:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_dbg/...

[adamk, 2014-04-15 20:46:17]

With respect to the v8::V8::RunMicrotasks, this now seems acceptable to me (at
least in the short term). ScriptResolverWithContext now encapsulates enough of
the right state that its call to resolve() + spinning the microtask queue should
get the correct behavior.

I think it might be useful to move this patch to a new Rietveld issue with a
longer change description containing much of the information in your last
comment, and leaving this issue around for history's sake.

https://codereview.chromium.org/209853010/diff/880001/LayoutTests/virtual/sta...
File
LayoutTests/virtual/stable/webexposed/global-constructors-listing-dedicated-worker-expected.txt
(right):

https://codereview.chromium.org/209853010/diff/880001/LayoutTests/virtual/sta...
LayoutTests/virtual/stable/webexposed/global-constructors-listing-dedicated-worker-expected.txt:57:
[Worker] WeakMap
These seem unrelated...(same in the other places where WeakMap/WeakSet show up)

[yhirano, 2014-04-16 01:45:20]

https://codereview.chromium.org/209853010/diff/880001/LayoutTests/virtual/sta...
File
LayoutTests/virtual/stable/webexposed/global-constructors-listing-dedicated-worker-expected.txt
(right):

https://codereview.chromium.org/209853010/diff/880001/LayoutTests/virtual/sta...
LayoutTests/virtual/stable/webexposed/global-constructors-listing-dedicated-worker-expected.txt:57:
[Worker] WeakMap
On 2014/04/15 20:46:18, adamk wrote:
> These seem unrelated...(same in the other places where WeakMap/WeakSet show
up)

V8 Promises require weak collections and that's why they are introduced.
The intent to ship[1] is also for weak collections, so I think it's OK.
I will note that this CL introduces weak collections as well in the description.

1: https://groups.google.com/forum/#!topic/v8-users/PFOLFuqT3SQ

[yhirano, 2014-04-22 07:49:37]

The new CL is https://codereview.chromium.org/243703007/ .