Handle rejected promises better for workers.

Handle rejected promises better for workers.

Rework how the rejected promises that v8 reports are processed in
a worker context, attaching the queue containing them to
WorkerScriptController, which is then drained by the worker thread's
task observer.

Along with that, make the individual rejected promises Oilpan friendly.

R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189785

[sof, 2015-02-02 21:44:58]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-02-02 21:44:58]

Please take a look.

Has better GC plugin support been considered for these kinds of off-heap
collections?

[haraken, 2015-02-02 23:38:07]

LGTM.

> Has better GC plugin support been considered for these kinds of off-heap
> collections?

Yes, I do hope it.

[sof, 2015-02-03 06:39:17]

On 2015/02/02 23:38:07, haraken wrote:
> LGTM.
> 
> > Has better GC plugin support been considered for these kinds of off-heap
> > collections?
> 
> Yes, I do hope it.

Will look into a bit, I would have thought there was some coverage for this
already, but perhaps not.

Anyway, more work needed here, as this thread-specific persistent leads to
leakage on shutdown.

[sof, 2015-02-03 10:16:13]

On 2015/02/03 06:39:17, sof wrote:
> On 2015/02/02 23:38:07, haraken wrote:
> > LGTM.
> > 
> > > Has better GC plugin support been considered for these kinds of off-heap
> > > collections?
> > 
> > Yes, I do hope it.
> 
> Will look into a bit, I would have thought there was some coverage for this
> already, but perhaps not.
> 
> Anyway, more work needed here, as this thread-specific persistent leads to
> leakage on shutdown.

Maintaining the invariant that worker thread heaps should not have any
Persistent<>s left on detach required some extra work & another shutdown step to
handle these promise queues.

Behaves better now (but checking bots for completeness); pt(another)l?

[sof, 2015-02-03 19:48:14]

On 2015/02/03 10:16:13, sof wrote:
> On 2015/02/03 06:39:17, sof wrote:
> > On 2015/02/02 23:38:07, haraken wrote:
> > > LGTM.
> > > 
> > > > Has better GC plugin support been considered for these kinds of off-heap
> > > > collections?
> > > 
> > > Yes, I do hope it.
> > 
> > Will look into a bit, I would have thought there was some coverage for this
> > already, but perhaps not.
> > 
> > Anyway, more work needed here, as this thread-specific persistent leads to
> > leakage on shutdown.
> 
> Maintaining the invariant that worker thread heaps should not have any
> Persistent<>s left on detach required some extra work & another shutdown step
to
> handle these promise queues.
> 
> Behaves better now (but checking bots for completeness); pt(another)l?

Bots all green.

[sof, 2015-02-04 10:31:56]

As ps#2 made substantial changes over the initial one, it needs a re-review, I
reckon.

(If it's already in a long queue, all good :) )

[sof, 2015-02-06 06:27:19]

ping

[haraken, 2015-02-06 06:49:14]

On 2015/02/06 06:27:19, sof wrote:
> ping

Sorry for the delay.

Can we probably just make OwnPtrWillBePersistent<PromiseRejectMessageQueue> be
owned by ScriptController and WorkerScriptController (or some similar class per
thread)?

[sof, 2015-02-06 11:21:18]

On 2015/02/06 06:49:14, haraken wrote:
> On 2015/02/06 06:27:19, sof wrote:
> > ping
> 
> Sorry for the delay.
> 
> Can we probably just make OwnPtrWillBePersistent<PromiseRejectMessageQueue> be
> owned by ScriptController and WorkerScriptController (or some similar class
per
> thread)?

How would the registered v8 promise reject callback get at that
ScriptController-owned object when invoked?

[haraken, 2015-02-06 13:32:34]

On 2015/02/06 11:21:18, sof wrote:
> On 2015/02/06 06:49:14, haraken wrote:
> > On 2015/02/06 06:27:19, sof wrote:
> > > ping
> > 
> > Sorry for the delay.
> > 
> > Can we probably just make OwnPtrWillBePersistent<PromiseRejectMessageQueue>
be
> > owned by ScriptController and WorkerScriptController (or some similar class
> per
> > thread)?
> 
> How would the registered v8 promise reject callback get at that
> ScriptController-owned object when invoked?

The callback should know ScriptState, so you can get to the ScriptController via
something like scriptState->executionContext()->... (We might need to add some
helper method to ScriptState though.)

[sof, 2015-02-07 17:12:52]

On 2015/02/06 06:49:14, haraken wrote:
> On 2015/02/06 06:27:19, sof wrote:
> > ping
> 
> Sorry for the delay.
> 
> Can we probably just make OwnPtrWillBePersistent<PromiseRejectMessageQueue> be
> owned by ScriptController and WorkerScriptController (or some similar class
per
> thread)?

Reworked this layer a bit; please take another look?

(And if you're wondering why ScriptController doesn't have one of these rejected
promises queues, look at ps#4.)

[haraken, 2015-02-08 01:53:53]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-02-08 01:53:55]

Mostly looks good.

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/RejectedPromises.cpp:73: void
RejectedPromises::dispose()

Who calls dispose() when the main thread shuts down?

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Initializer.cpp:192:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<RejectedPromises>, rejectedPromises,
(adoptPtrWillBeNoop (new RejectedPromises())));

Nit: Unnecessary space after Noop.

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Initializer.cpp:283: ScriptState* scriptState =
ScriptState::from(context);

Slightly better:

ScriptState* scriptState = ScriptState::current(isolate);
if (!scriptState->contextIsValid())
    return;

and remove line 279 and 280.

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:144:
m_rejectedPromises->dispose();

Won't ~WorkerScriptController() be called in some destructor of an on-heap
object?

I guess it would be safe to call dispose() in WorkerGlobalScope::dispose().

[sof, 2015-02-08 06:57:39]

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/RejectedPromises.cpp:73: void
RejectedPromises::dispose()
On 2015/02/08 01:53:54, haraken wrote:
> 
> Who calls dispose() when the main thread shuts down?

Like on trunk, no one drains it should there be any items added to the queue
after the last running of EndOfTaskRunner. Is that a real possibility?

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.cpp:144:
m_rejectedPromises->dispose();
On 2015/02/08 01:53:55, haraken wrote:
> 
> Won't ~WorkerScriptController() be called in some destructor of an on-heap
> object?
> 
> I guess it would be safe to call dispose() in WorkerGlobalScope::dispose().

It is called from there; the OwnPtr<WorkerScriptController> is cleared via
clearScript().

[sof, 2015-02-08 07:26:48]

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Initializer.cpp:192:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<RejectedPromises>, rejectedPromises,
(adoptPtrWillBeNoop (new RejectedPromises())));
On 2015/02/08 01:53:55, haraken wrote:
> 
> Nit: Unnecessary space after Noop.

Removed

https://codereview.chromium.org/897523002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Initializer.cpp:283: ScriptState* scriptState =
ScriptState::from(context);
On 2015/02/08 01:53:54, haraken wrote:
> 
> Slightly better:
> 
> ScriptState* scriptState = ScriptState::current(isolate);
> if (!scriptState->contextIsValid())
>     return;
> 
> and remove line 279 and 280.

That looks a bit tidier, thanks. Switched.

[haraken, 2015-02-09 00:53:06]

LGTM

[sof, 2015-02-09 06:39:01]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-02-09 06:39:20]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2015-02-09 07:09:49]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-09 07:10:06]

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

[commit-bot: I haz the power, 2015-02-09 07:13:32]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189785