Don't report promise rejection events during the microtask checkpoint

Don't report promise rejection events during the microtask checkpoint

instead post them to the message loop.

BUG=495801
Committed: https://crrev.com/31bc5867e1112af8b82d780b47fcb3a6f337a8a0
Cr-Commit-Position: refs/heads/master@{#352251}

[jochen (gone - plz use gerrit), 2015-09-30 14:31:41]

jochen@chromium.org changed reviewers:
+ domenic@chromium.org, haraken@chromium.org, hiroshige@chromium.org

[jochen (gone - plz use gerrit), 2015-09-30 14:31:42]

https://codereview.chromium.org/1381693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1381693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:231:
Platform::current()->currentThread()->taskRunner()->postTask(FROM_HERE, new
Task(bind(&RejectedPromises::processQueueNow, this, queue)));
hiroshige@

this line doesn't even compile, so I guess it's wrong :)

any suggestion what to do here?

[hiroshige, 2015-10-01 03:59:25]

https://codereview.chromium.org/1381693002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1381693002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:231:
Platform::current()->currentThread()->taskRunner()->postTask(FROM_HERE, new
Task(bind(&RejectedPromises::processQueueNow, this, queue)));
On 2015/09/30 14:31:42, jochen wrote:
> hiroshige@
> 
> this line doesn't even compile, so I guess it's wrong :)
> 
> any suggestion what to do here?

WTF::bind()'s argument should be:
- Something copyable,
- PassOwnPtr<>,
- T* or RawPtr<T> if T is GCed,
- (or so)

Non-Oilpan:
Compilation fails because Deque<OwnPtr<Message>> is not copyable.
PassOwnPtr<Deque<OwnPtr<Message>>> can be used.

Oilpan (not so sure though):
HeapDeque<Member<Message>> is a GCed object, so
RawPtr<HeapDeque<Member<Message>>> should be used.

(Background info: This is because HeapDeque<Member<Message>> should be stored as
Persistent<HeapDeque<Member<Message>>> in PartBoundFunctionImpl, because
PartBoundFunctionImpl is not GCed object. WTF::bind() converts RawPtr<...> to
CrossThreadPersistent automatically via  ParamStorageTraits<RawPtr<T>> -- in
this case Persistent is sufficient but CrossThreadPersistent is used because the
conversion code can be used for cross-thread cases.)


So, OwnPtrWillBeRawPtr<WillBeHeapDeque<OwnPtrWillBeMember<Message>>> can be
used??
However,
- I'm still not sure how to move |m_queue| into
OwnPtrWillBeRawPtr<WillBeHeapDeque<OwnPtrWillBeMember<Message>>>.
- Quite long type name.

haraken@, could you double check for oilpan issues?

[haraken, 2015-10-01 04:09:16]

On 2015/10/01 03:59:25, hiroshige wrote:
>
https://codereview.chromium.org/1381693002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):
> 
>
https://codereview.chromium.org/1381693002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:231:
> Platform::current()->currentThread()->taskRunner()->postTask(FROM_HERE, new
> Task(bind(&RejectedPromises::processQueueNow, this, queue)));
> On 2015/09/30 14:31:42, jochen wrote:
> > hiroshige@
> > 
> > this line doesn't even compile, so I guess it's wrong :)
> > 
> > any suggestion what to do here?
> 
> WTF::bind()'s argument should be:
> - Something copyable,
> - PassOwnPtr<>,
> - T* or RawPtr<T> if T is GCed,
> - (or so)
> 
> Non-Oilpan:
> Compilation fails because Deque<OwnPtr<Message>> is not copyable.
> PassOwnPtr<Deque<OwnPtr<Message>>> can be used.
> 
> Oilpan (not so sure though):
> HeapDeque<Member<Message>> is a GCed object, so
> RawPtr<HeapDeque<Member<Message>>> should be used.
> 
> (Background info: This is because HeapDeque<Member<Message>> should be stored
as
> Persistent<HeapDeque<Member<Message>>> in PartBoundFunctionImpl, because
> PartBoundFunctionImpl is not GCed object. WTF::bind() converts RawPtr<...> to
> CrossThreadPersistent automatically via  ParamStorageTraits<RawPtr<T>> -- in
> this case Persistent is sufficient but CrossThreadPersistent is used because
the
> conversion code can be used for cross-thread cases.)
> 

Thanks hiroshige-san, this makes sense.

> 
> So, OwnPtrWillBeRawPtr<WillBeHeapDeque<OwnPtrWillBeMember<Message>>> can be
> used??
> However,
> - I'm still not sure how to move |m_queue| into
> OwnPtrWillBeRawPtr<WillBeHeapDeque<OwnPtrWillBeMember<Message>>>.

We need to change m_queue's typ to
OwnPtrWillBeRawPtr<WillBeHeapDeque<OwnPtrWillBeMember<Message>>>.

> - Quite long type name.

Let's use a 'using' :)

> 
> haraken@, could you double check for oilpan issues?

[jochen (gone - plz use gerrit), 2015-10-01 09:48:54]

the types are all subtly different (OwnPtr vs PassOwnPtr for example), so I just
wrote them out.

still updating the tests, as the ones you (Domenic) wrote don't work :) - I left
some comments on the pull request

[jochen (gone - plz use gerrit), 2015-10-02 07:57:24]

ptal

Domenic, overall

Kentaro, bindings/

https://codereview.chromium.org/1381693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (left):

https://codereview.chromium.org/1381693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:62:
ASSERT(!v8::Local<v8::Promise>::Cast(value)->HasHandler());
that's no longer true, as we post a task to report the unhandled rejection, and
by the time it's reported there might already be a handler.

[haraken, 2015-10-02 07:59:47]

https://codereview.chromium.org/1381693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.h (right):

https://codereview.chromium.org/1381693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.h:19: class
RejectedPromises final : public
RefCountedWillBeGarbageCollected<RejectedPromises> {

Why do you need to change OwnPtr to RefPtr?

[jochen (gone - plz use gerrit), 2015-10-02 08:02:04]

https://codereview.chromium.org/1381693002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.h (right):

https://codereview.chromium.org/1381693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.h:19: class
RejectedPromises final : public
RefCountedWillBeGarbageCollected<RejectedPromises> {
On 2015/10/02 at 07:59:47, haraken wrote:
> Why do you need to change OwnPtr to RefPtr?

So the class doesn't die while a task is on the message loop. Workers actually
destroy the RejectedPromises when they shut down.

[haraken, 2015-10-02 09:19:42]

LGTM on my side.

But keep in mind that the current implementation of m_promise is wrong -- since
visitDOMWrapper is not called in a minor GC, the minor GC can collect the
(weakened) m_promise prematurely.

[jochen (gone - plz use gerrit), 2015-10-02 09:58:31]

On 2015/10/02 at 09:19:42, haraken wrote:
> LGTM on my side.
> 
> But keep in mind that the current implementation of m_promise is wrong --
since visitDOMWrapper is not called in a minor GC, the minor GC can collect the
(weakened) m_promise prematurely.

That's the m_promise on the PromiseRejectionEvent, right? that's not touched in
this CL anyway

[haraken, 2015-10-02 10:03:25]

On 2015/10/02 09:58:31, jochen wrote:
> On 2015/10/02 at 09:19:42, haraken wrote:
> > LGTM on my side.
> > 
> > But keep in mind that the current implementation of m_promise is wrong --
> since visitDOMWrapper is not called in a minor GC, the minor GC can collect
the
> (weakened) m_promise prematurely.
> 
> That's the m_promise on the PromiseRejectionEvent, right? that's not touched
in
> this CL anyway

Ah, you're right.

But doesn't the Message::m_promise have the same issue? How is it guaranteed to
survive a minor GC when it's weakened?

[hiroshige, 2015-10-02 10:25:29]

lgtm (for the bind() issue).

[jochen (gone - plz use gerrit), 2015-10-02 10:36:23]

On 2015/10/02 at 10:03:25, haraken wrote:
> On 2015/10/02 09:58:31, jochen wrote:
> > On 2015/10/02 at 09:19:42, haraken wrote:
> > > LGTM on my side.
> > > 
> > > But keep in mind that the current implementation of m_promise is wrong --
> > since visitDOMWrapper is not called in a minor GC, the minor GC can collect
the
> > (weakened) m_promise prematurely.
> > 
> > That's the m_promise on the PromiseRejectionEvent, right? that's not touched
in
> > this CL anyway
> 
> Ah, you're right.
> 
> But doesn't the Message::m_promise have the same issue? How is it guaranteed
to survive a minor GC when it's weakened?

it's initially created as a strong reference, and only made weak after it was
reported (line 251). If it's then gc'd before a handler was attached, we don't
need the message anymore and discard it (line 211/242). If a handler is
attached, it's made strong again (line 212)

[jochen (gone - plz use gerrit), 2015-10-02 12:33:00]

updated to reflect spec changes

[domenic, 2015-10-02 20:11:47]

On 2015/10/02 at 12:33:00, jochen wrote:
> updated to reflect spec changes

lgtm. https://github.com/domenic/unhandled-rejections-browser-spec is updated
with the test changes here and the spec matches the implementation.

[jochen (gone - plz use gerrit), 2015-10-02 20:12:46]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-10-02 20:12:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/1381693002/#ps100001
(title: "updates")

[commit-bot: I haz the power, 2015-10-02 20:15:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381693002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381693002/100001

[commit-bot: I haz the power, 2015-10-02 22:39:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 22:39:31]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jochen (gone - plz use gerrit), 2015-10-03 06:03:06]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2015-10-03 06:03:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381693002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381693002/100001

[commit-bot: I haz the power, 2015-10-03 08:59:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-03 09:00:09]

Patchset 6 (id:??) landed as
https://crrev.com/31bc5867e1112af8b82d780b47fcb3a6f337a8a0
Cr-Commit-Position: refs/heads/master@{#352251}

[sof, 2015-10-04 15:48:15]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-10-04 15:48:16]

Introduced some leaks,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/bu...

[sof, 2015-10-04 20:06:22]

https://codereview.chromium.org/1381693002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1381693002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:245:
Platform::current()->currentThread()->scheduler()->timerTaskRunner()->postTask(FROM_HERE,
new Task(bind(&RejectedPromises::processQueueNow, this, queue.release())));
To have the closure retain a RefPtr<> to the RejectedPromises object(*), you
need to explicitly convert it on construction to make that clear --

  s/this/PassRefPtrWillBeRawPtr<RejectedPromises>(this)/

As-is, the closure retains a bare pointer to the object, which results in things
like


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

* - is it required to use a RefPtr<> to implement the semantics? (the underlying
question being.. do the pending tasks here for a shut down worker need to run?)

[yosin_UTC9, 2015-10-05 04:06:18]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1384913002/ by yosin@chromium.org.

The reason for reverting is: Causes leaks in following tests:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...


fast/dom/promise-rejection-events-isolated-worlds.html
web-animations-api/player-ready-promise.html
http/tests/fetch/window/fetch.html
inspector/sources/debugger-pause/debugger-pause-on-promise-rejection.html
inspector-enabled/console/console-uncaught-promise-no-inspector.html
http/tests/serviceworker/fetch-request-resources.html
inspector/console/console-revoke-error.html
inspector/console/console-uncaught-promise.html
fast/dom/promise-rejection-events-attached-in-event.html
inspector/sources/debugger/debugger-uncaught-promise-on-pause.html
imported/web-platform-tests/screen-orientation/lock-bad-argument.html
web-animations-api/player-finished-promise.html
http/tests/dom/promise-rejection-events.html
fast/css/fontfaceset-add-remove-while-loading.html.