[WIP] Migrate hasPendingActivity from ActiveDOMObject to ScriptWrappable.

[WIP] Migrate hasPendingActivity from ActiveDOMObject to ScriptWrappable.

R=haraken@chromium.org

BUG=483722

[vivekg, 2015-05-15 04:50:30]

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

[vivekg, 2015-05-15 04:50:30]

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

[vivekg, 2015-05-15 04:50:30]

This CL is mostly a mechanical change in which I have just introduced
hasPendingActivity to ScriptWrappable object. 

This results in some ambiguous name resolution with some of the classes as they
have been derived directly/indirectly using both ScriptWrappable and
ActiveDOMObject.

I have replaced these places with correct form of the call with explicit naming.
But this somehow doesn't seem right in that we still end up having
ActiveDOMObject::hasPendingActivity.

Please suggest.

[vivekg, 2015-05-15 04:50:31]

This CL is mostly a mechanical change in which I have just introduced
hasPendingActivity to ScriptWrappable object. 

This results in some ambiguous name resolution with some of the classes as they
have been derived directly/indirectly using both ScriptWrappable and
ActiveDOMObject.

I have replaced these places with correct form of the call with explicit naming.
But this somehow doesn't seem right in that we still end up having
ActiveDOMObject::hasPendingActivity.

Please suggest.

[haraken, 2015-05-15 12:14:24]

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

[haraken, 2015-05-15 12:14:25]

+kinuko-san

https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
File Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to handle
this usage of ActiveDOMObject::hasPendingActivity?

It seems that ContextLifecycleNotifier::hasPendingActivity() is used only for
counting the number of pending activities in a worker thread (for some purpose I
don't fully understand).

- If we can remove it, it would be the best.

- If we cannot remove it, I think we can move the logic to
WorkerScriptController (or something like that). Since we're just interested in
the number of pending activities of the worker thread, we can manage the number
per isolate (i.e., we don't need to manage the number per context).

Maybe kinuko-san has an idea on this.

[vivekg, 2015-05-18 16:27:05]

On 2015/05/15 at 12:14:25, haraken wrote:
> +kinuko-san
> 
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> File Source/core/dom/ContextLifecycleNotifier.cpp (right):
> 
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to handle
this usage of ActiveDOMObject::hasPendingActivity?
> 
> It seems that ContextLifecycleNotifier::hasPendingActivity() is used only for
counting the number of pending activities in a worker thread (for some purpose I
don't fully understand).
> 
> - If we can remove it, it would be the best.
> 
> - If we cannot remove it, I think we can move the logic to
WorkerScriptController (or something like that). Since we're just interested in
the number of pending activities of the worker thread, we can manage the number
per isolate (i.e., we don't need to manage the number per context).
> 
> Maybe kinuko-san has an idea on this.

Thank you haraken@ for the review. Lets wait for kinuko-san@ for his take on
this migration.

[kinuko, 2015-05-19 16:02:30]

https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
File Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to handle
this usage of ActiveDOMObject::hasPendingActivity?
On 2015/05/15 12:14:25, haraken wrote:
> It seems that ContextLifecycleNotifier::hasPendingActivity() is used only for
> counting the number of pending activities in a worker thread (for some purpose
I
> don't fully understand).

My old memory tells me that this code is (was?) used to keep a parent execution
context (i.e. Document) alive while any of its child DOM objects have pending
async operations.  I'm a bit unfamiliar with the background concept of the
'document lifetime', how do we do so for Document today?  Worker's basically
trying to do same as Document does (but doing so across threads as worker's
execution context and worker's DOM object live on different threads), if we
don't need this for document we can probably remove it for worker too.

> - If we can remove it, it would be the best.
> 
> - If we cannot remove it, I think we can move the logic to
> WorkerScriptController (or something like that). Since we're just interested
in
> the number of pending activities of the worker thread, we can manage the
number
> per isolate (i.e., we don't need to manage the number per context).
>
> Maybe kinuko-san has an idea on this.

[kinuko, 2015-05-19 16:09:59]

On 2015/05/19 16:02:30, kinuko wrote:
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> File Source/core/dom/ContextLifecycleNotifier.cpp (right):
> 
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to handle
> this usage of ActiveDOMObject::hasPendingActivity?
> On 2015/05/15 12:14:25, haraken wrote:
> > It seems that ContextLifecycleNotifier::hasPendingActivity() is used only
for
> > counting the number of pending activities in a worker thread (for some
purpose
> I
> > don't fully understand).
> 
> My old memory tells me that this code is (was?) used to keep a parent
execution
> context (i.e. Document) alive while any of its child DOM objects have pending
> async operations.  I'm a bit unfamiliar with the background concept of the
> 'document lifetime', how do we do so for Document today?  Worker's basically
> trying to do same as Document does (but doing so across threads as worker's
> execution context and worker's DOM object live on different threads), if we
> don't need this for document we can probably remove it for worker too.

I imagine we could possibly just return true for a worker object (to keep its
wrapper alive) while the corresponding thread's running.  I need to check if
that's the correct behavior, but it feels ok... to me.

> > - If we can remove it, it would be the best.
> > 
> > - If we cannot remove it, I think we can move the logic to
> > WorkerScriptController (or something like that). Since we're just interested
> in
> > the number of pending activities of the worker thread, we can manage the
> number
> > per isolate (i.e., we don't need to manage the number per context).
> >
> > Maybe kinuko-san has an idea on this.

[haraken, 2015-07-08 02:07:31]

On 2015/05/19 16:09:59, kinuko wrote:
> On 2015/05/19 16:02:30, kinuko wrote:
> >
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> > File Source/core/dom/ContextLifecycleNotifier.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> > Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to
handle
> > this usage of ActiveDOMObject::hasPendingActivity?
> > On 2015/05/15 12:14:25, haraken wrote:
> > > It seems that ContextLifecycleNotifier::hasPendingActivity() is used only
> for
> > > counting the number of pending activities in a worker thread (for some
> purpose
> > I
> > > don't fully understand).
> > 
> > My old memory tells me that this code is (was?) used to keep a parent
> execution
> > context (i.e. Document) alive while any of its child DOM objects have
pending
> > async operations.  I'm a bit unfamiliar with the background concept of the
> > 'document lifetime', how do we do so for Document today?  Worker's basically
> > trying to do same as Document does (but doing so across threads as worker's
> > execution context and worker's DOM object live on different threads), if we
> > don't need this for document we can probably remove it for worker too.
> 
> I imagine we could possibly just return true for a worker object (to keep its
> wrapper alive) while the corresponding thread's running.  I need to check if
> that's the correct behavior, but it feels ok... to me.
> 
> > > - If we can remove it, it would be the best.
> > > 
> > > - If we cannot remove it, I think we can move the logic to
> > > WorkerScriptController (or something like that). Since we're just
interested
> > in
> > > the number of pending activities of the worker thread, we can manage the
> > number
> > > per isolate (i.e., we don't need to manage the number per context).
> > >
> > > Maybe kinuko-san has an idea on this.

Any progress on this?

Moving hasPendingActivity to ScriptWrappable is a key to reduce # of
ActiveDOMObjects and various overhead associated with it.

[vivekg, 2015-07-09 08:51:20]

On 2015/07/08 at 02:07:31, haraken wrote:
> On 2015/05/19 16:09:59, kinuko wrote:
> > On 2015/05/19 16:02:30, kinuko wrote:
> > >
> >
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> > > File Source/core/dom/ContextLifecycleNotifier.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> > > Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to
handle
> > > this usage of ActiveDOMObject::hasPendingActivity?
> > > On 2015/05/15 12:14:25, haraken wrote:
> > > > It seems that ContextLifecycleNotifier::hasPendingActivity() is used
only
> > for
> > > > counting the number of pending activities in a worker thread (for some
> > purpose
> > > I
> > > > don't fully understand).
> > > 
> > > My old memory tells me that this code is (was?) used to keep a parent
> > execution
> > > context (i.e. Document) alive while any of its child DOM objects have
pending
> > > async operations.  I'm a bit unfamiliar with the background concept of the
> > > 'document lifetime', how do we do so for Document today?  Worker's
basically
> > > trying to do same as Document does (but doing so across threads as
worker's
> > > execution context and worker's DOM object live on different threads), if
we
> > > don't need this for document we can probably remove it for worker too.
> > 
> > I imagine we could possibly just return true for a worker object (to keep
its
> > wrapper alive) while the corresponding thread's running.  I need to check if
> > that's the correct behavior, but it feels ok... to me.
> > 
> > > > - If we can remove it, it would be the best.
> > > > 
> > > > - If we cannot remove it, I think we can move the logic to
> > > > WorkerScriptController (or something like that). Since we're just
interested
> > > in
> > > > the number of pending activities of the worker thread, we can manage the
> > > number
> > > > per isolate (i.e., we don't need to manage the number per context).
> > > >
> > > > Maybe kinuko-san has an idea on this.
> 
> Any progress on this?
> 
> Moving hasPendingActivity to ScriptWrappable is a key to reduce # of
ActiveDOMObjects and various overhead associated with it.

Sorry haraken for not updating this. The email notification got slipped thr' the
mail churn. 
I will be busy with some internal tasks happening for couple of weeks. In the
meanwhile, if anybody has some cycles available can pick this up.
Or I can resume once I am done with internal tasks. WDYT?

[haraken, 2015-07-09 09:12:38]

On 2015/07/09 08:51:20, vivekg_ wrote:
> On 2015/07/08 at 02:07:31, haraken wrote:
> > On 2015/05/19 16:09:59, kinuko wrote:
> > > On 2015/05/19 16:02:30, kinuko wrote:
> > > >
> > >
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> > > > File Source/core/dom/ContextLifecycleNotifier.cpp (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1133713008/diff/1/Source/core/dom/ContextLife...
> > > > Source/core/dom/ContextLifecycleNotifier.cpp:110: // Any idea on how to
> handle
> > > > this usage of ActiveDOMObject::hasPendingActivity?
> > > > On 2015/05/15 12:14:25, haraken wrote:
> > > > > It seems that ContextLifecycleNotifier::hasPendingActivity() is used
> only
> > > for
> > > > > counting the number of pending activities in a worker thread (for some
> > > purpose
> > > > I
> > > > > don't fully understand).
> > > > 
> > > > My old memory tells me that this code is (was?) used to keep a parent
> > > execution
> > > > context (i.e. Document) alive while any of its child DOM objects have
> pending
> > > > async operations.  I'm a bit unfamiliar with the background concept of
the
> > > > 'document lifetime', how do we do so for Document today?  Worker's
> basically
> > > > trying to do same as Document does (but doing so across threads as
> worker's
> > > > execution context and worker's DOM object live on different threads), if
> we
> > > > don't need this for document we can probably remove it for worker too.
> > > 
> > > I imagine we could possibly just return true for a worker object (to keep
> its
> > > wrapper alive) while the corresponding thread's running.  I need to check
if
> > > that's the correct behavior, but it feels ok... to me.
> > > 
> > > > > - If we can remove it, it would be the best.
> > > > > 
> > > > > - If we cannot remove it, I think we can move the logic to
> > > > > WorkerScriptController (or something like that). Since we're just
> interested
> > > > in
> > > > > the number of pending activities of the worker thread, we can manage
the
> > > > number
> > > > > per isolate (i.e., we don't need to manage the number per context).
> > > > >
> > > > > Maybe kinuko-san has an idea on this.
> > 
> > Any progress on this?
> > 
> > Moving hasPendingActivity to ScriptWrappable is a key to reduce # of
> ActiveDOMObjects and various overhead associated with it.
> 
> Sorry haraken for not updating this. The email notification got slipped thr'
the
> mail churn. 
> I will be busy with some internal tasks happening for couple of weeks. In the
> meanwhile, if anybody has some cycles available can pick this up.
> Or I can resume once I am done with internal tasks. WDYT?

Thanks for the update -- I hope min(you, someone) :) This is not an urgent task
but is something we want to finish in a reasonable timeline.

[vivekg, 2015-10-09 06:52:16]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 06:52:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713008/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133713008/20001

[commit-bot: I haz the power, 2015-10-09 08:30:09]

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

[commit-bot: I haz the power, 2015-10-09 08:30:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vivekg, 2015-10-09 09:38:37]

The windows and mac have reported about the below test failure with ASSERTION.

plugins/refcount-leaks.html

Whereas linux passes all the tests.

[haraken, 2015-10-09 09:41:06]

On 2015/10/09 09:38:37, vivekg_ wrote:
> The windows and mac have reported about the below test failure with ASSERTION.
> 
> plugins/refcount-leaks.html
> 
> Whereas linux passes all the tests.

Is the crash really related to this CL? The crash is happening at "ASSERTION
FAILED: npObject", which seems unrelated to hasPendingActivity.

[vivekg, 2015-10-09 09:43:32]

On 2015/10/09 09:41:06, haraken wrote:
> On 2015/10/09 09:38:37, vivekg_ wrote:
> > The windows and mac have reported about the below test failure with
ASSERTION.
> > 
> > plugins/refcount-leaks.html
> > 
> > Whereas linux passes all the tests.
> 
> Is the crash really related to this CL? The crash is happening at "ASSERTION
> FAILED: npObject", which seems unrelated to hasPendingActivity.

I feel so but then if you see the call stack on windows [1], there is
V8GCController::gcPrologue call hence got a doubt. 

[1]
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

[haraken, 2015-10-09 09:48:09]

https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:265:
ScriptWrappable* scriptwrappable = getInternalField<ScriptWrappable,
v8DOMWrapperObjectIndex>(wrapper);

Is it guaranteed that NPObject has the internal field at
v8DOMWrapperObjectIndex?

[haraken, 2015-10-09 09:56:59]

On 2015/10/09 09:48:09, haraken wrote:
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:265:
> ScriptWrappable* scriptwrappable = getInternalField<ScriptWrappable,
> v8DOMWrapperObjectIndex>(wrapper);
> 
> Is it guaranteed that NPObject has the internal field at
> v8DOMWrapperObjectIndex?

It is unsafe to use getInternalField directly. You can use toScriptWrappable.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[vivekg, 2015-10-09 10:42:48]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 10:43:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133713008/40001

[vivekg, 2015-10-09 10:52:34]

On 2015/10/09 09:56:59, haraken wrote:
> On 2015/10/09 09:48:09, haraken wrote:
> >
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:265:
> > ScriptWrappable* scriptwrappable = getInternalField<ScriptWrappable,
> > v8DOMWrapperObjectIndex>(wrapper);
> > 
> > Is it guaranteed that NPObject has the internal field at
> > v8DOMWrapperObjectIndex?
> 
> It is unsafe to use getInternalField directly. You can use toScriptWrappable.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

[haraken, 2015-10-09 10:56:58]

On 2015/10/09 10:52:34, vivekg_ wrote:
> On 2015/10/09 09:56:59, haraken wrote:
> > On 2015/10/09 09:48:09, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:265:
> > > ScriptWrappable* scriptwrappable = getInternalField<ScriptWrappable,
> > > v8DOMWrapperObjectIndex>(wrapper);
> > > 
> > > Is it guaranteed that NPObject has the internal field at
> > > v8DOMWrapperObjectIndex?
> > 
> > It is unsafe to use getInternalField directly. You can use
toScriptWrappable.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Done.

Let's see if tests pass.

[vivekg, 2015-10-09 11:54:57]

On 2015/10/09 10:56:58, haraken wrote:
> On 2015/10/09 10:52:34, vivekg_ wrote:
> > On 2015/10/09 09:56:59, haraken wrote:
> > > On 2015/10/09 09:48:09, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1133713008/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:265:
> > > > ScriptWrappable* scriptwrappable = getInternalField<ScriptWrappable,
> > > > v8DOMWrapperObjectIndex>(wrapper);
> > > > 
> > > > Is it guaranteed that NPObject has the internal field at
> > > > v8DOMWrapperObjectIndex?
> > > 
> > > It is unsafe to use getInternalField directly. You can use
> toScriptWrappable.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Done.
> 
> Let's see if tests pass.

The failure still exist again on Windows and Mac.

[commit-bot: I haz the power, 2015-10-09 12:12:37]

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

[commit-bot: I haz the power, 2015-10-09 12:12:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2015-10-09 12:23:03]

Maybe you want to insert RELEASE_ASSERT around toScriptWrappable in
V8GCController.cpp to identify where you're crashing?

[vivekg, 2015-10-09 12:47:45]

On 2015/10/09 12:23:03, haraken wrote:
> Maybe you want to insert RELEASE_ASSERT around toScriptWrappable in
> V8GCController.cpp to identify where you're crashing?

Are you suggesting RELEASE_ASSERT(false) around toScriptWrappable?
In this case, most of the layout tests would fail and the layout test runner
will come out of running the tests.

I tried running it locally on my linux machine but the test is failing not due
to crash.

[haraken, 2015-10-09 13:26:26]

https://codereview.chromium.org/1133713008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/1133713008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:266: if
(scriptwrappable && scriptwrappable->hasPendingActivity()) {

What happens if you change this to:

  if (strcmp(type->interfaceName, "NPObject") && scriptwrappable &&
scriptwrappable->hasPendingActivity())

?

If the crash is gone, we can know that the crash is caused by calling
hasPendingActivity for NPObject.

[vivekg, 2015-10-09 13:43:00]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-09 13:43:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133713008/60001

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

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

[commit-bot: I haz the power, 2015-10-09 15:12:01]

Dry run: This issue passed the CQ dry run.

[vivekg, 2015-10-09 16:59:21]

On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Yes the crash is gone with this additional check. So now we know for sure that
the problem is with the hasPendingActivity for NPObject.

[haraken, 2015-10-10 14:20:56]

On 2015/10/09 16:59:21, vivekg_ wrote:
> On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Yes the crash is gone with this additional check. So now we know for sure that
> the problem is with the hasPendingActivity for NPObject.

Is the crash happening when calling hasPendingActivity? Or after calling
hasPendingActivity (i.e., somewhere in a if clause of the hasPendingActivity
check)?

[vivekg, 2015-10-12 05:42:41]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-12 05:42:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133713008/80001

[commit-bot: I haz the power, 2015-10-12 07:08:40]

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

[commit-bot: I haz the power, 2015-10-12 07:08:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vivekg, 2015-10-12 08:16:38]

On 2015/10/10 14:20:56, haraken wrote:
> On 2015/10/09 16:59:21, vivekg_ wrote:
> > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > Dry run: This issue passed the CQ dry run.
> > 
> > Yes the crash is gone with this additional check. So now we know for sure
that
> > the problem is with the hasPendingActivity for NPObject.
> 
> Is the crash happening when calling hasPendingActivity? Or after calling
> hasPendingActivity (i.e., somewhere in a if clause of the hasPendingActivity
> check)?

Its failing inside the if block as per the latest patch.

[haraken, 2015-10-12 09:14:50]

On 2015/10/12 08:16:38, vivekg_ wrote:
> On 2015/10/10 14:20:56, haraken wrote:
> > On 2015/10/09 16:59:21, vivekg_ wrote:
> > > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > > Dry run: This issue passed the CQ dry run.
> > > 
> > > Yes the crash is gone with this additional check. So now we know for sure
> that
> > > the problem is with the hasPendingActivity for NPObject.
> > 
> > Is the crash happening when calling hasPendingActivity? Or after calling
> > hasPendingActivity (i.e., somewhere in a if clause of the hasPendingActivity
> > check)?
> 
> Its failing inside the if block as per the latest patch.

NPObject shouldn't have a pending activity. Would it be possible to make
toScriptWrappable(object)->hasPendingActivity return false if the object is a
NPObject? Maybe we can tweak a wrapperTypeInfo of NPV8Object somehow...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[vivekg, 2015-10-13 06:08:35]

On 2015/10/12 09:14:50, haraken wrote:
> On 2015/10/12 08:16:38, vivekg_ wrote:
> > On 2015/10/10 14:20:56, haraken wrote:
> > > On 2015/10/09 16:59:21, vivekg_ wrote:
> > > > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > > > Dry run: This issue passed the CQ dry run.
> > > > 
> > > > Yes the crash is gone with this additional check. So now we know for
sure
> > that
> > > > the problem is with the hasPendingActivity for NPObject.
> > > 
> > > Is the crash happening when calling hasPendingActivity? Or after calling
> > > hasPendingActivity (i.e., somewhere in a if clause of the
hasPendingActivity
> > > check)?
> > 
> > Its failing inside the if block as per the latest patch.
> 
> NPObject shouldn't have a pending activity. Would it be possible to make
> toScriptWrappable(object)->hasPendingActivity return false if the object is a
> NPObject? Maybe we can tweak a wrapperTypeInfo of NPV8Object somehow...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Is this conversion safe? I mean by default ScriptWrappable::hasPendingActivity
returns false.

ScriptWrappable* npObjectToScriptWrappable(NPObject* npObject)
{
    return reinterpret_cast<ScriptWrappable*>(npObject);
}

[haraken, 2015-10-13 06:16:23]

On 2015/10/13 06:08:35, vivekg_ wrote:
> On 2015/10/12 09:14:50, haraken wrote:
> > On 2015/10/12 08:16:38, vivekg_ wrote:
> > > On 2015/10/10 14:20:56, haraken wrote:
> > > > On 2015/10/09 16:59:21, vivekg_ wrote:
> > > > > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > > > > Dry run: This issue passed the CQ dry run.
> > > > > 
> > > > > Yes the crash is gone with this additional check. So now we know for
> sure
> > > that
> > > > > the problem is with the hasPendingActivity for NPObject.
> > > > 
> > > > Is the crash happening when calling hasPendingActivity? Or after calling
> > > > hasPendingActivity (i.e., somewhere in a if clause of the
> hasPendingActivity
> > > > check)?
> > > 
> > > Its failing inside the if block as per the latest patch.
> > 
> > NPObject shouldn't have a pending activity. Would it be possible to make
> > toScriptWrappable(object)->hasPendingActivity return false if the object is
a
> > NPObject? Maybe we can tweak a wrapperTypeInfo of NPV8Object somehow...
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Is this conversion safe? I mean by default ScriptWrappable::hasPendingActivity
> returns false.
> 
> ScriptWrappable* npObjectToScriptWrappable(NPObject* npObject)
> {
>     return reinterpret_cast<ScriptWrappable*>(npObject);
> }


I'm not sure if the conversion is safe or not, but if
npObject->hasPendingActivity() always returns false, why do we crash inside the
if(hasPendingActivity()) block? If npObject->hasPendingActivity()always returns
false, NPObject should not enter the if block.

[vivekg, 2015-10-14 06:33:01]

On 2015/10/13 06:16:23, haraken wrote:
> On 2015/10/13 06:08:35, vivekg_ wrote:
> > On 2015/10/12 09:14:50, haraken wrote:
> > > On 2015/10/12 08:16:38, vivekg_ wrote:
> > > > On 2015/10/10 14:20:56, haraken wrote:
> > > > > On 2015/10/09 16:59:21, vivekg_ wrote:
> > > > > > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > > > > > Dry run: This issue passed the CQ dry run.
> > > > > > 
> > > > > > Yes the crash is gone with this additional check. So now we know for
> > sure
> > > > that
> > > > > > the problem is with the hasPendingActivity for NPObject.
> > > > > 
> > > > > Is the crash happening when calling hasPendingActivity? Or after
calling
> > > > > hasPendingActivity (i.e., somewhere in a if clause of the
> > hasPendingActivity
> > > > > check)?
> > > > 
> > > > Its failing inside the if block as per the latest patch.
> > > 
> > > NPObject shouldn't have a pending activity. Would it be possible to make
> > > toScriptWrappable(object)->hasPendingActivity return false if the object
is
> a
> > > NPObject? Maybe we can tweak a wrapperTypeInfo of NPV8Object somehow...
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Is this conversion safe? I mean by default
ScriptWrappable::hasPendingActivity
> > returns false.
> > 
> > ScriptWrappable* npObjectToScriptWrappable(NPObject* npObject)
> > {
> >     return reinterpret_cast<ScriptWrappable*>(npObject);
> > }
> 
> 
> I'm not sure if the conversion is safe or not, but if
> npObject->hasPendingActivity() always returns false, why do we crash inside
the
> if(hasPendingActivity()) block? If npObject->hasPendingActivity()always
returns
> false, NPObject should not enter the if block.

I tried to run the tests on windows machine but still unable to reproduce this
locally. I am not sure why hasPendingActivity is returning true whereas the
default return is false.

[haraken, 2015-10-14 07:56:10]

On 2015/10/14 06:33:01, vivekg_ wrote:
> On 2015/10/13 06:16:23, haraken wrote:
> > On 2015/10/13 06:08:35, vivekg_ wrote:
> > > On 2015/10/12 09:14:50, haraken wrote:
> > > > On 2015/10/12 08:16:38, vivekg_ wrote:
> > > > > On 2015/10/10 14:20:56, haraken wrote:
> > > > > > On 2015/10/09 16:59:21, vivekg_ wrote:
> > > > > > > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > > > > > > Dry run: This issue passed the CQ dry run.
> > > > > > > 
> > > > > > > Yes the crash is gone with this additional check. So now we know
for
> > > sure
> > > > > that
> > > > > > > the problem is with the hasPendingActivity for NPObject.
> > > > > > 
> > > > > > Is the crash happening when calling hasPendingActivity? Or after
> calling
> > > > > > hasPendingActivity (i.e., somewhere in a if clause of the
> > > hasPendingActivity
> > > > > > check)?
> > > > > 
> > > > > Its failing inside the if block as per the latest patch.
> > > > 
> > > > NPObject shouldn't have a pending activity. Would it be possible to make
> > > > toScriptWrappable(object)->hasPendingActivity return false if the object
> is
> > a
> > > > NPObject? Maybe we can tweak a wrapperTypeInfo of NPV8Object somehow...
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > Is this conversion safe? I mean by default
> ScriptWrappable::hasPendingActivity
> > > returns false.
> > > 
> > > ScriptWrappable* npObjectToScriptWrappable(NPObject* npObject)
> > > {
> > >     return reinterpret_cast<ScriptWrappable*>(npObject);
> > > }
> > 
> > 
> > I'm not sure if the conversion is safe or not, but if
> > npObject->hasPendingActivity() always returns false, why do we crash inside
> the
> > if(hasPendingActivity()) block? If npObject->hasPendingActivity()always
> returns
> > false, NPObject should not enter the if block.
> 
> I tried to run the tests on windows machine but still unable to reproduce this
> locally. I am not sure why hasPendingActivity is returning true whereas the
> default return is false.

Are you sure that we hit the following assert?

  ASSERT(strcmp(type->interfaceName, "NPObject") ||
!toScriptWrappable(object)->hasPendingActivity());

[vivekg, 2015-10-14 09:44:55]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-14 09:45:45]

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

[vivekg, 2015-10-14 10:34:23]

On 2015/10/14 07:56:10, haraken wrote:
> On 2015/10/14 06:33:01, vivekg_ wrote:
> > On 2015/10/13 06:16:23, haraken wrote:
> > > On 2015/10/13 06:08:35, vivekg_ wrote:
> > > > On 2015/10/12 09:14:50, haraken wrote:
> > > > > On 2015/10/12 08:16:38, vivekg_ wrote:
> > > > > > On 2015/10/10 14:20:56, haraken wrote:
> > > > > > > On 2015/10/09 16:59:21, vivekg_ wrote:
> > > > > > > > On 2015/10/09 15:12:01, commit-bot: I haz the power wrote:
> > > > > > > > > Dry run: This issue passed the CQ dry run.
> > > > > > > > 
> > > > > > > > Yes the crash is gone with this additional check. So now we know
> for
> > > > sure
> > > > > > that
> > > > > > > > the problem is with the hasPendingActivity for NPObject.
> > > > > > > 
> > > > > > > Is the crash happening when calling hasPendingActivity? Or after
> > calling
> > > > > > > hasPendingActivity (i.e., somewhere in a if clause of the
> > > > hasPendingActivity
> > > > > > > check)?
> > > > > > 
> > > > > > Its failing inside the if block as per the latest patch.
> > > > > 
> > > > > NPObject shouldn't have a pending activity. Would it be possible to
make
> > > > > toScriptWrappable(object)->hasPendingActivity return false if the
object
> > is
> > > a
> > > > > NPObject? Maybe we can tweak a wrapperTypeInfo of NPV8Object
somehow...
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > 
> > > > Is this conversion safe? I mean by default
> > ScriptWrappable::hasPendingActivity
> > > > returns false.
> > > > 
> > > > ScriptWrappable* npObjectToScriptWrappable(NPObject* npObject)
> > > > {
> > > >     return reinterpret_cast<ScriptWrappable*>(npObject);
> > > > }
> > > 
> > > 
> > > I'm not sure if the conversion is safe or not, but if
> > > npObject->hasPendingActivity() always returns false, why do we crash
inside
> > the
> > > if(hasPendingActivity()) block? If npObject->hasPendingActivity()always
> > returns
> > > false, NPObject should not enter the if block.
> > 
> > I tried to run the tests on windows machine but still unable to reproduce
this
> > locally. I am not sure why hasPendingActivity is returning true whereas the
> > default return is false.
> 
> Are you sure that we hit the following assert?
> 
>   ASSERT(strcmp(type->interfaceName, "NPObject") ||
> !toScriptWrappable(object)->hasPendingActivity());

Yes here is the crash log
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

[haraken, 2015-10-14 11:12:10]

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

[haraken, 2015-10-14 11:12:11]

+yukishiino for help

In short: If we insert the following line to V8GCController (look at the CL), we
hit the ASSERT. We wonder why.

  ASSERT(strcmp(type->interfaceName, "NPObject") ||
!toScriptWrappable(object)->hasPendingActivity());

If the type is "NPObject", toScriptWrappable(object)->hasPendingActivity()
should return false because ScriptWrappable::hasPendingActivity should be
used... This crash happens only on win and mac try bots.

[commit-bot: I haz the power, 2015-10-14 11:17:24]

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

[commit-bot: I haz the power, 2015-10-14 11:17:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2015-10-15 10:19:28]

On 2015/10/14 11:12:11, haraken wrote:
> +yukishiino for help
> 
> In short: If we insert the following line to V8GCController (look at the CL),
we
> hit the ASSERT. We wonder why.
> 
>   ASSERT(strcmp(type->interfaceName, "NPObject") ||
> !toScriptWrappable(object)->hasPendingActivity());
> 
> If the type is "NPObject", toScriptWrappable(object)->hasPendingActivity()
> should return false because ScriptWrappable::hasPendingActivity should be
> used... This crash happens only on win and mac try bots.

This code is unsafe.  Do not do this.

>   ASSERT(strcmp(type->interfaceName, "NPObject") ||
> !toScriptWrappable(object)->hasPendingActivity());

Note that a) ScriptWrappable::hasPendingActivity() is a virtual function, and b)
NPObject does NOT inherit from ScriptWrappable, thus there is no vtbl in an
NPObject, or even if it exists, there is no entry to hasPendingActivity in the
vtbl.  So, you MUST NOT call any virtual functions of ScriptWrappable for an
NPObject.  You shouldn't call any (including non-virtual) member functions of
ScriptWrappable, either.

Plus, strcmp() in this style is confusing.  I'd recommend to write strcmp(a, b)
!= 0 explicitly in this case.  Actually, I'm not sure if you're confused with
strcmp(a, b) == 0 or not.

The ASSERT statement reads
    ASSERT( if not NPObject || !hasPendingActivity() );
Then, if you're sure that NPObject never be a pending activity, this assert
looks almost meaningless.  Did you mean the following?
    ASSERT( if NPObject || !hasPendingActivity() );

The current code (Patch Set 6) is:
    if (strcmp(type->interfaceName, "NPObject") && scriptwrappable &&
scriptwrappable->hasPendingActivity())
        return;
    RELEASE_ASSERT(strcmp(type->interfaceName, "NPObject") ||
!scriptwrappable->hasPendingActivity());
This code looks strange to me.  In pseudo code,
    if ( not NPObject && hasPendingActivity )
        return;
    // else, NPObject || not hasPendingActivity
    RELEASE_ASSERT( ****NOT**** NPObject || not hasPendingActivity);

Why?

[haraken, 2015-10-15 10:30:25]

On 2015/10/15 10:19:28, Yuki wrote:
> On 2015/10/14 11:12:11, haraken wrote:
> > +yukishiino for help
> > 
> > In short: If we insert the following line to V8GCController (look at the
CL),
> we
> > hit the ASSERT. We wonder why.
> > 
> >   ASSERT(strcmp(type->interfaceName, "NPObject") ||
> > !toScriptWrappable(object)->hasPendingActivity());
> > 
> > If the type is "NPObject", toScriptWrappable(object)->hasPendingActivity()
> > should return false because ScriptWrappable::hasPendingActivity should be
> > used... This crash happens only on win and mac try bots.
> 
> This code is unsafe.  Do not do this.

ahhhh, good point! I was assuming NPObject is ScriptWrappable.

Then what we want to do would be:

  if (not NPObject && scriptWrappable->hasPendingActivity)
    ...;

strcmp is not nice since it's heavy. Maybe can we use some other entries in
WrapperTypeInfo? (I don't really care about the implementation since NPObject is
going to be deprecated very soon.)


> 
> >   ASSERT(strcmp(type->interfaceName, "NPObject") ||
> > !toScriptWrappable(object)->hasPendingActivity());
> 
> Note that a) ScriptWrappable::hasPendingActivity() is a virtual function, and
b)
> NPObject does NOT inherit from ScriptWrappable, thus there is no vtbl in an
> NPObject, or even if it exists, there is no entry to hasPendingActivity in the
> vtbl.  So, you MUST NOT call any virtual functions of ScriptWrappable for an
> NPObject.  You shouldn't call any (including non-virtual) member functions of
> ScriptWrappable, either.
> 
> Plus, strcmp() in this style is confusing.  I'd recommend to write strcmp(a,
b)
> != 0 explicitly in this case.  Actually, I'm not sure if you're confused with
> strcmp(a, b) == 0 or not.
> 
> The ASSERT statement reads
>     ASSERT( if not NPObject || !hasPendingActivity() );
> Then, if you're sure that NPObject never be a pending activity, this assert
> looks almost meaningless.  Did you mean the following?
>     ASSERT( if NPObject || !hasPendingActivity() );
> 
> The current code (Patch Set 6) is:
>     if (strcmp(type->interfaceName, "NPObject") && scriptwrappable &&
> scriptwrappable->hasPendingActivity())
>         return;
>     RELEASE_ASSERT(strcmp(type->interfaceName, "NPObject") ||
> !scriptwrappable->hasPendingActivity());
> This code looks strange to me.  In pseudo code,
>     if ( not NPObject && hasPendingActivity )
>         return;
>     // else, NPObject || not hasPendingActivity
>     RELEASE_ASSERT( ****NOT**** NPObject || not hasPendingActivity);
> 
> Why?

[Yuki, 2015-10-15 10:43:06]

On 2015/10/15 10:30:25, haraken wrote:
> strcmp is not nice since it's heavy. Maybe can we use some other entries in
> WrapperTypeInfo? (I don't really care about the implementation since NPObject
is
> going to be deprecated very soon.)

type == npObjectTypeInfo()
should be enough.

[haraken, 2015-10-15 10:43:53]

On 2015/10/15 10:43:06, Yuki wrote:
> On 2015/10/15 10:30:25, haraken wrote:
> > strcmp is not nice since it's heavy. Maybe can we use some other entries in
> > WrapperTypeInfo? (I don't really care about the implementation since
NPObject
> is
> > going to be deprecated very soon.)
> 
> type == npObjectTypeInfo()
> should be enough.

Sounds nice to me.

[haraken, 2015-10-15 10:45:02]

On 2015/10/15 10:43:53, haraken wrote:
> On 2015/10/15 10:43:06, Yuki wrote:
> > On 2015/10/15 10:30:25, haraken wrote:
> > > strcmp is not nice since it's heavy. Maybe can we use some other entries
in
> > > WrapperTypeInfo? (I don't really care about the implementation since
> NPObject
> > is
> > > going to be deprecated very soon.)
> > 
> > type == npObjectTypeInfo()
> > should be enough.
> 
> Sounds nice to me.

Also we should implement toScriptWrappable(NPObject*) { static_assert(0, "should
not reach here") } (in a follow-up CL).

[Yuki, 2015-10-15 10:48:52]

On 2015/10/15 10:45:02, haraken wrote:
> Also we should implement toScriptWrappable(NPObject*) { static_assert(0,
"should
> not reach here") } (in a follow-up CL).

AFAIK, we don't have such a function.  We have
    toScriptWrappable(v8::Local<v8::Value>)
and in this case, we don't know if it's an NPObject or not at compile time.

Since NPObject does not inherit from ScriptWrappable, it's not that easy to
convert them each other.

[haraken, 2015-10-15 11:11:33]

On 2015/10/15 10:48:52, Yuki wrote:
> On 2015/10/15 10:45:02, haraken wrote:
> > Also we should implement toScriptWrappable(NPObject*) { static_assert(0,
> "should
> > not reach here") } (in a follow-up CL).
> 
> AFAIK, we don't have such a function.  We have
>     toScriptWrappable(v8::Local<v8::Value>)
> and in this case, we don't know if it's an NPObject or not at compile time.
> 
> Since NPObject does not inherit from ScriptWrappable, it's not that easy to
> convert them each other.

Uploaded a CL to add an ASSERT:
https://codereview.chromium.org/1399763004/

[vivekg, 2015-10-15 13:05:15]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 13:05:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713008/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133713008/120001

[vivekg, 2015-10-15 13:14:01]

So anyone having familiarity with this logic can help?

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp:113: //
ActiveDOMObject* activeDOMObject = static_cast<ActiveDOMObject*>(observer);
Just to revive the context, quoting the text from our previous discussion here:

On 2015/05/15 12:14:25, haraken wrote:
> It seems that ContextLifecycleNotifier::hasPendingActivity() is used only for
> counting the number of pending activities in a worker thread (for some purpose
I don't fully understand).

My old memory tells me that this code is (was?) used to keep a parent execution
context (i.e. Document) alive while any of its child DOM objects have pending
async operations.  I'm a bit unfamiliar with the background concept of the
'document lifetime', how do we do so for Document today?  Worker's basically
trying to do same as Document does (but doing so across threads as worker's
execution context and worker's DOM object live on different threads), if we
don't need this for document we can probably remove it for worker too.

[haraken, 2015-10-15 13:41:41]

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

[haraken, 2015-10-15 13:41:42]

On 2015/10/15 13:14:01, vivekg_ wrote:
> So anyone having familiarity with this logic can help?
> 
>
https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp (right):
> 
>
https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp:113: //
> ActiveDOMObject* activeDOMObject = static_cast<ActiveDOMObject*>(observer);
> Just to revive the context, quoting the text from our previous discussion
here:
> 
> On 2015/05/15 12:14:25, haraken wrote:
> > It seems that ContextLifecycleNotifier::hasPendingActivity() is used only
for
> > counting the number of pending activities in a worker thread (for some
purpose
> I don't fully understand).
> 
> My old memory tells me that this code is (was?) used to keep a parent
execution
> context (i.e. Document) alive while any of its child DOM objects have pending
> async operations.  I'm a bit unfamiliar with the background concept of the
> 'document lifetime', how do we do so for Document today?  Worker's basically
> trying to do same as Document does (but doing so across threads as worker's
> execution context and worker's DOM object live on different threads), if we
> don't need this for document we can probably remove it for worker too.

A worker does not have a Document. Worker's 'context' is a WorkerGlobalScope
(which derives ExecutionContext).

As far as I see:

- ContextLifecycleNotifier::hasPendingActivity() is used only for counting the
number of pending activities in a worker thread.
- The result is passed into various classes, and finally used as a result of
InProcessWorkerBase::hasPendingActivity(). In other words, the result of
ContextLifecycleNotifier::hasPendingActivity() is used to keep
InProcessWorkerBase's wrapper alive while the corresponding worker thread has a
pending activity.

I don't understand why such a logic is needed.

kinuko-san, toyoshima-san: Do you have any idea on this? Or can we just try to
remove ContextLifecycleNotifier::hasPendingActivity()?

[commit-bot: I haz the power, 2015-10-15 15:30:48]

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

[commit-bot: I haz the power, 2015-10-15 15:30:49]

Dry run: This issue passed the CQ dry run.

[vivekg, 2015-10-16 05:59:37]

I will be OOO for the next week. Will be back on 26th October.

[Yuki, 2015-10-16 06:43:16]

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:900:
ScriptWrappable::hasPendingActivity() ? " ScriptWrappable::hasPendingActivity()"
: "",
The direct super class (on the path to ScriptWrappable) of this class is
    RefCountedGarbageCollectedEventTargetWithInlineData<MediaKeySession>,
so you should call
    RefCountedGarbageCollectedEventTargetWithInlineData<MediaKeySession>::
    hasPendingActivity()
instead of ScriptWrappable::hasPendingActivity().

You may or may not want to define an alias in .h (just after inheritance) like
    typedef RefCountedGarbageCollectedEventTargetWithInlineData<MediaKeySession>
ParentClassToScriptWrappable;
or something like that.

If we think that it's granted that super class' hasPendingActivity() always
returns false, then we can remove this code.

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:905: return
ScriptWrappable::hasPendingActivity()
Ditto.

[vivekg, 2015-10-16 06:59:33]

Thanks for the review. Removed the code.

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
(right):

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:900:
ScriptWrappable::hasPendingActivity() ? " ScriptWrappable::hasPendingActivity()"
: "",
On 2015/10/16 06:43:15, Yuki wrote:
> The direct super class (on the path to ScriptWrappable) of this class is
>     RefCountedGarbageCollectedEventTargetWithInlineData<MediaKeySession>,
> so you should call
>     RefCountedGarbageCollectedEventTargetWithInlineData<MediaKeySession>::
>     hasPendingActivity()
> instead of ScriptWrappable::hasPendingActivity().
> 
> You may or may not want to define an alias in .h (just after inheritance) like
>     typedef
RefCountedGarbageCollectedEventTargetWithInlineData<MediaKeySession>
> ParentClassToScriptWrappable;
> or something like that.
> 
> If we think that it's granted that super class' hasPendingActivity() always
> returns false, then we can remove this code.


Done. Removed the code as it would always return false here.

https://codereview.chromium.org/1133713008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp:905: return
ScriptWrappable::hasPendingActivity()
On 2015/10/16 06:43:15, Yuki wrote:
> Ditto.

Done.

[vivekg, 2015-10-16 06:59:41]

The CQ bit was checked by vivekg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 07:01:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133713008/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133713008/140001

[commit-bot: I haz the power, 2015-10-16 08:12:50]

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

[commit-bot: I haz the power, 2015-10-16 08:12:51]

Dry run: This issue passed the CQ dry run.

[vivekg, 2015-10-27 03:21:08]

On 2015/10/15 13:41:42, haraken wrote:
> A worker does not have a Document. Worker's 'context' is a WorkerGlobalScope
> (which derives ExecutionContext).
> 
> As far as I see:
> 
> - ContextLifecycleNotifier::hasPendingActivity() is used only for counting the
> number of pending activities in a worker thread.
> - The result is passed into various classes, and finally used as a result of
> InProcessWorkerBase::hasPendingActivity(). In other words, the result of
> ContextLifecycleNotifier::hasPendingActivity() is used to keep
> InProcessWorkerBase's wrapper alive while the corresponding worker thread has
a
> pending activity.
> 
> I don't understand why such a logic is needed.
> 
> kinuko-san, toyoshima-san: Do you have any idea on this? Or can we just try to
> remove ContextLifecycleNotifier::hasPendingActivity()?

kinuko@, toyoshima@: Any comment on the fate of
ContextLifecycleNotifier::hasPendingActivity() ?

[kinuko (google), 2015-10-27 05:28:03]

Description was changed from

==========
[WIP] Migrate hasPendingActivity from ActiveDOMObject to ScriptWrappable.

R=haraken@chromium.org

BUG=483722
==========

to

==========
[WIP] Migrate hasPendingActivity from ActiveDOMObject to ScriptWrappable.

R=haraken@chromium.org

BUG=483722
==========

[kinuko (google), 2015-10-27 05:28:03]

kinuko@google.com changed reviewers:
+ yurys@chromium.org

[kinuko (google), 2015-10-27 05:51:50]

On 2015/10/27 03:21:08, vivekg_ wrote:
> On 2015/10/15 13:41:42, haraken wrote:
> > A worker does not have a Document. Worker's 'context' is a WorkerGlobalScope
> > (which derives ExecutionContext).
> > 
> > As far as I see:
> > 
> > - ContextLifecycleNotifier::hasPendingActivity() is used only for counting
the
> > number of pending activities in a worker thread.
> > - The result is passed into various classes, and finally used as a result of
> > InProcessWorkerBase::hasPendingActivity(). In other words, the result of
> > ContextLifecycleNotifier::hasPendingActivity() is used to keep
> > InProcessWorkerBase's wrapper alive while the corresponding worker thread
has
> a
> > pending activity.
> > 
> > I don't understand why such a logic is needed.
> > 
> > kinuko-san, toyoshima-san: Do you have any idea on this? Or can we just try
to
> > remove ContextLifecycleNotifier::hasPendingActivity()?
> 
> kinuko@, toyoshima@: Any comment on the fate of
> ContextLifecycleNotifier::hasPendingActivity() ?

Sorry for slow response again, wasn't really tracking this one.  Looks like
https://bugs.webkit.org/show_bug.cgi?id=62292 is where we introduced this.  It
has some repro code and discussion, but in short we seem to have introduced this
mechanism not to have a worker get collected when the worker has started
outstanding activity in the message handler. I'm still not fully sure if
checking the pending activity only at the end of message handler is really
reasonable and necessary, but while I'm looking into these bits further let me
cc yurys@ who implemented this part.  (Yury, might you be able to give some
comments on this? ^^^)

[vivekg, 2015-10-29 10:54:13]

yurys@ any comments/pointers?

[kinuko, 2015-10-29 16:06:10]

Did a little more tests using the 'repro' code attached to the original bug
(https://bugs.webkit.org/show_bug.cgi?id=62292).  I also inserted some
artificial gc() in the code.  Looks like if we completely disregard the pending
activity on the worker context (e.g. just let
ContextLifecycleNotifier::hasPendingActivity() always return false) it looks
Worker object could get GC'ed even when worker context has pending activity
(e.g. outstanding timer), which seems to be unexpected behavior per spec as
having pending activity must keep the worker as 'protected'.  So... probably the
code could be cleaned up further but we seem to need something like the current
code.

[haraken, 2015-10-29 16:13:56]

Thanks for investigating the weird bit!

On 2015/10/29 16:06:10, kinuko wrote:
> Did a little more tests using the 'repro' code attached to the original bug
> (https://bugs.webkit.org/show_bug.cgi?id=62292).  I also inserted some
> artificial gc() in the code.  Looks like if we completely disregard the
pending
> activity on the worker context (e.g. just let
> ContextLifecycleNotifier::hasPendingActivity() always return false) it looks
> Worker object could get GC'ed even when worker context has pending activity
> (e.g. outstanding timer), which seems to be unexpected behavior per spec as
> having pending activity must keep the worker as 'protected'.  So... probably
the
> code could be cleaned up further but we seem to need something like the
current
> code.

Per the spec, when should the worker object get GCed? I think there should be a
more explicit timing we should collect the worker object (rather than observing
pending activities of the worker context).

[yurys, 2015-10-29 16:15:40]

yurys@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org, pfeldman@chromium.org

[yurys, 2015-10-29 16:28:26]

I'm not sure I understand the value of moving the method from ActiveDOMObject to
ScriptWrappable. Can you elaborate on this? In particular, how will it help
eliminate inheritance from ContextLifecycleObserver for the objects with pending
activities?

https://codereview.chromium.org/1133713008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/1133713008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:154: virtual bool
hasPendingActivity() const;
It looks strange that we have only this method on ScriptWrappable while all
activity control methods (suspend/resume etc.) are on ActiveDOMObject. Is there
a plan on changing that?

https://codereview.chromium.org/1133713008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/1133713008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp:114: // if
(activeDOMObject->hasPendingActivity())
Revert this?

[haraken, 2015-10-29 16:35:43]

On 2015/10/29 16:28:26, yurys wrote:
> I'm not sure I understand the value of moving the method from ActiveDOMObject
to
> ScriptWrappable. Can you elaborate on this? In particular, how will it help
> eliminate inheritance from ContextLifecycleObserver for the objects with
pending
> activities?

Currently a lot of DOM objects are made ActiveDOMObjects just because they want
to keep their wrappers alive (using hasPendingAcitivity). This adds a lot of
overhead because ContextLifecycleNotifier needs to keep track of all
ActiveDOMObjects and send signals when something happens (e.g., contextStopped).
In the particular case of HTMLImageElement, the overhead of making
HTMLImageElement ActiveDOMObject is unacceptable, and thus we've added a hack to
add HTMLImageElement::hasPendingActivity without making it an ActiveDOMObject
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

In short, ActiveDOMObject is heavy. So we want to reduce the number of
ActiveDOMObjects.

[kinuko, 2015-10-30 06:05:40]

On 2015/10/29 16:13:56, haraken wrote:
> Thanks for investigating the weird bit!
> 
> On 2015/10/29 16:06:10, kinuko wrote:
> > Did a little more tests using the 'repro' code attached to the original bug
> > (https://bugs.webkit.org/show_bug.cgi?id=62292).  I also inserted some
> > artificial gc() in the code.  Looks like if we completely disregard the
> pending
> > activity on the worker context (e.g. just let
> > ContextLifecycleNotifier::hasPendingActivity() always return false) it looks
> > Worker object could get GC'ed even when worker context has pending activity
> > (e.g. outstanding timer), which seems to be unexpected behavior per spec as
> > having pending activity must keep the worker as 'protected'.  So... probably
> the
> > code could be cleaned up further but we seem to need something like the
> current
> > code.
> 
> Per the spec, when should the worker object get GCed? I think there should be
a
> more explicit timing we should collect the worker object (rather than
observing
> pending activities of the worker context).

I had the same question for some time now so I've revisited spec a few times
before, but basically there doesn't seem no clear text about when a worker
object should get GC'ed, but there're some texts about when a worker gets closed
or can be killed.  In my reading in short:

- Worker will be closed and eventually killed once it stops being a protected
worker
- UA can also actually kill a worker at anytime, say, for CPU quota management
or in response to a user request.

And a protected worker's definition is:
- Any of its responsible document(s) is fully active, and
- Either it has outstanding timers, database transactions, or network
connections, or its list of worker's port is not empty

The spec also has text like "Start monitoring the worker such that no sooner
than it stops being a protected worker...", which feels that monitoring if a
worker context has pending activity or not is an understandable implementation
for the spec text.

[haraken, 2015-10-30 18:06:21]

On 2015/10/30 06:05:40, kinuko wrote:
> On 2015/10/29 16:13:56, haraken wrote:
> > Thanks for investigating the weird bit!
> > 
> > On 2015/10/29 16:06:10, kinuko wrote:
> > > Did a little more tests using the 'repro' code attached to the original
bug
> > > (https://bugs.webkit.org/show_bug.cgi?id=62292).  I also inserted some
> > > artificial gc() in the code.  Looks like if we completely disregard the
> > pending
> > > activity on the worker context (e.g. just let
> > > ContextLifecycleNotifier::hasPendingActivity() always return false) it
looks
> > > Worker object could get GC'ed even when worker context has pending
activity
> > > (e.g. outstanding timer), which seems to be unexpected behavior per spec
as
> > > having pending activity must keep the worker as 'protected'.  So...
probably
> > the
> > > code could be cleaned up further but we seem to need something like the
> > current
> > > code.
> > 
> > Per the spec, when should the worker object get GCed? I think there should
be
> a
> > more explicit timing we should collect the worker object (rather than
> observing
> > pending activities of the worker context).
> 
> I had the same question for some time now so I've revisited spec a few times
> before, but basically there doesn't seem no clear text about when a worker
> object should get GC'ed, but there're some texts about when a worker gets
closed
> or can be killed.  In my reading in short:
> 
> - Worker will be closed and eventually killed once it stops being a protected
> worker
> - UA can also actually kill a worker at anytime, say, for CPU quota management
> or in response to a user request.
> 
> And a protected worker's definition is:
> - Any of its responsible document(s) is fully active, and
> - Either it has outstanding timers, database transactions, or network
> connections, or its list of worker's port is not empty
> 
> The spec also has text like "Start monitoring the worker such that no sooner
> than it stops being a protected worker...", which feels that monitoring if a
> worker context has pending activity or not is an understandable implementation
> for the spec text.

Thanks kinuko-san! I also investigated the spec and the implementation.

The spec requires that the worker object is kept alive while the worker has any
pending activity. ContextLifecycleNotifier::hasPendingActivity() is needed to
realize the behavior. However, the current implementation has the following
issues:

- Whether the worker has any pending activity or not is judged when the main
thread sent the last postMessage
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
not when the main thread performs a GC. This can lead to a wrong behavior if
some pending activity is initiated after the main thread sent the last
postMessage (and before the main thread performs a GC).

- Whenever the main thread sends a postMessage, the worker's
ContextLifecycleNotifer needs to iterate all ActiveDOMObjects and check their
pending activities. This sounds heavy.

So I'd propose the following implementation:

- Move hasPendingActivity from ActiveDOMObject to ScriptWrappable.

- When the worker thread performs a GC, the worker thread iterates all
ScriptWrappables (we're already doing this) and check their
hasPendingActivities. The worker thread reports the result to
Worker::m_hasPendingActivity (in the main thread).

- When the main thread performs a GC, do not collect the worker object while
Worker::m_hasPendingActivity returns true.

In this implementation, the worker object will be collected after 1) the worker
thread lost all pending activities *and* 2) the worker thread performs a GC
*and* 3) the main thread performs a GC. This will extend the lifetime of the
worker object a bit longer than today though (because the worker object won't be
collected until both the worker thread and the main thread perform GCs).

What do you think?

[kinuko, 2015-11-01 15:04:11]

On 2015/10/30 18:06:21, haraken wrote:
> On 2015/10/30 06:05:40, kinuko wrote:
> > On 2015/10/29 16:13:56, haraken wrote:
> > > Thanks for investigating the weird bit!
> > > 
> > > On 2015/10/29 16:06:10, kinuko wrote:
> > > > Did a little more tests using the 'repro' code attached to the original
> bug
> > > > (https://bugs.webkit.org/show_bug.cgi?id=62292).  I also inserted some
> > > > artificial gc() in the code.  Looks like if we completely disregard the
> > > pending
> > > > activity on the worker context (e.g. just let
> > > > ContextLifecycleNotifier::hasPendingActivity() always return false) it
> looks
> > > > Worker object could get GC'ed even when worker context has pending
> activity
> > > > (e.g. outstanding timer), which seems to be unexpected behavior per spec
> as
> > > > having pending activity must keep the worker as 'protected'.  So...
> probably
> > > the
> > > > code could be cleaned up further but we seem to need something like the
> > > current
> > > > code.
> > > 
> > > Per the spec, when should the worker object get GCed? I think there should
> be
> > a
> > > more explicit timing we should collect the worker object (rather than
> > observing
> > > pending activities of the worker context).
> > 
> > I had the same question for some time now so I've revisited spec a few times
> > before, but basically there doesn't seem no clear text about when a worker
> > object should get GC'ed, but there're some texts about when a worker gets
> closed
> > or can be killed.  In my reading in short:
> > 
> > - Worker will be closed and eventually killed once it stops being a
protected
> > worker
> > - UA can also actually kill a worker at anytime, say, for CPU quota
management
> > or in response to a user request.
> > 
> > And a protected worker's definition is:
> > - Any of its responsible document(s) is fully active, and
> > - Either it has outstanding timers, database transactions, or network
> > connections, or its list of worker's port is not empty
> > 
> > The spec also has text like "Start monitoring the worker such that no sooner
> > than it stops being a protected worker...", which feels that monitoring if a
> > worker context has pending activity or not is an understandable
implementation
> > for the spec text.
> 
> Thanks kinuko-san! I also investigated the spec and the implementation.
> 
> The spec requires that the worker object is kept alive while the worker has
any
> pending activity. ContextLifecycleNotifier::hasPendingActivity() is needed to
> realize the behavior. However, the current implementation has the following
> issues:
> 
> - Whether the worker has any pending activity or not is judged when the main
> thread sent the last postMessage
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> not when the main thread performs a GC. This can lead to a wrong behavior if
> some pending activity is initiated after the main thread sent the last
> postMessage (and before the main thread performs a GC).

During worker context initialization (and while top-level script is executed)
it's considered that the context has a pending activity, and if the top-level
script initiated a new pending activity it is also reported in
DedicatedWorkerGlobalScope::postInitialize, so I think it would be kept as far
as it keeps having pending activity.

> - Whenever the main thread sends a postMessage, the worker's
> ContextLifecycleNotifer needs to iterate all ActiveDOMObjects and check their
> pending activities. This sounds heavy.

Do we now how heavy it is?

> So I'd propose the following implementation:
> 
> - Move hasPendingActivity from ActiveDOMObject to ScriptWrappable.
> 
> - When the worker thread performs a GC, the worker thread iterates all
> ScriptWrappables (we're already doing this) and check their
> hasPendingActivities. The worker thread reports the result to
> Worker::m_hasPendingActivity (in the main thread).
>
> - When the main thread performs a GC, do not collect the worker object while
> Worker::m_hasPendingActivity returns true.
> 
> In this implementation, the worker object will be collected after 1) the
worker
> thread lost all pending activities *and* 2) the worker thread performs a GC
> *and* 3) the main thread performs a GC. This will extend the lifetime of the
> worker object a bit longer than today though (because the worker object won't
be
> collected until both the worker thread and the main thread perform GCs).
> 
> What do you think?

Sounds reasonable to me, one question though- does it mean we assume worker
context : worker thread is 1:1?  In compositor worker cases for example we might
not collect worker objects on the worker thread if any of the worker context has
pending objects?

[haraken, 2015-11-01 17:33:25]

> > The spec requires that the worker object is kept alive while the worker has
> any
> > pending activity. ContextLifecycleNotifier::hasPendingActivity() is needed
to
> > realize the behavior. However, the current implementation has the following
> > issues:
> > 
> > - Whether the worker has any pending activity or not is judged when the main
> > thread sent the last postMessage
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> > not when the main thread performs a GC. This can lead to a wrong behavior if
> > some pending activity is initiated after the main thread sent the last
> > postMessage (and before the main thread performs a GC).
> 
> During worker context initialization (and while top-level script is executed)
> it's considered that the context has a pending activity, and if the top-level
> script initiated a new pending activity it is also reported in
> DedicatedWorkerGlobalScope::postInitialize, so I think it would be kept as far
> as it keeps having pending activity.

Ah, understand :) But then another question comes up.

- Why do you need to check pending activities at every postMessage? If the
pending activity is reported by DedicatedWorkerGlobalScope::postInitialize,
won't it be enough?

- I now understand how the worker object is kept alive while the worker context
has any pending activity, but I'm getting confused about how the worker object
gets collected. In the current implementation, when does the worker object (in
the main thread) get collected? As far as I understand, the worker object won't
get collected until the following event (*) happens.

(*) The main thread sends a postMessage to the worker thread after the worker
thread loses all pending activities.

Doesn't this mean that the worker object doesn't get collected until the main
thread sends a postMessage to the (mostly-dead) worker thread?

> 
> > - Whenever the main thread sends a postMessage, the worker's
> > ContextLifecycleNotifer needs to iterate all ActiveDOMObjects and check
their
> > pending activities. This sounds heavy.
> 
> Do we now how heavy it is?

Don't know :)

> > So I'd propose the following implementation:
> > 
> > - Move hasPendingActivity from ActiveDOMObject to ScriptWrappable.
> > 
> > - When the worker thread performs a GC, the worker thread iterates all
> > ScriptWrappables (we're already doing this) and check their
> > hasPendingActivities. The worker thread reports the result to
> > Worker::m_hasPendingActivity (in the main thread).
> >
> > - When the main thread performs a GC, do not collect the worker object while
> > Worker::m_hasPendingActivity returns true.
> > 
> > In this implementation, the worker object will be collected after 1) the
> worker
> > thread lost all pending activities *and* 2) the worker thread performs a GC
> > *and* 3) the main thread performs a GC. This will extend the lifetime of the
> > worker object a bit longer than today though (because the worker object
won't
> be
> > collected until both the worker thread and the main thread perform GCs).
> > 
> > What do you think?
> 
> Sounds reasonable to me, one question though- does it mean we assume worker
> context : worker thread is 1:1?  In compositor worker cases for example we
might
> not collect worker objects on the worker thread if any of the worker context
has
> pending objects?

Good point. Yes, in my proposal, the worker object won't get collected if any
worker context in the worker thread has a pending activity. (Maybe we can do
something smarter though.)

[kinuko, 2015-11-02 07:06:00]

On 2015/11/01 17:33:25, haraken wrote:
> > > The spec requires that the worker object is kept alive while the worker
has
> > any
> > > pending activity. ContextLifecycleNotifier::hasPendingActivity() is needed
> to
> > > realize the behavior. However, the current implementation has the
following
> > > issues:
> > > 
> > > - Whether the worker has any pending activity or not is judged when the
main
> > > thread sent the last postMessage
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> > > not when the main thread performs a GC. This can lead to a wrong behavior
if
> > > some pending activity is initiated after the main thread sent the last
> > > postMessage (and before the main thread performs a GC).
> > 
> > During worker context initialization (and while top-level script is
executed)
> > it's considered that the context has a pending activity, and if the
top-level
> > script initiated a new pending activity it is also reported in
> > DedicatedWorkerGlobalScope::postInitialize, so I think it would be kept as
far
> > as it keeps having pending activity.
> 
> Ah, understand :) But then another question comes up.
> 
> - Why do you need to check pending activities at every postMessage? If the
> pending activity is reported by DedicatedWorkerGlobalScope::postInitialize,
> won't it be enough?
> 
> - I now understand how the worker object is kept alive while the worker
context
> has any pending activity, but I'm getting confused about how the worker object
> gets collected. In the current implementation, when does the worker object (in
> the main thread) get collected? As far as I understand, the worker object
won't
> get collected until the following event (*) happens.
> 
> (*) The main thread sends a postMessage to the worker thread after the worker
> thread loses all pending activities.
> 
> Doesn't this mean that the worker object doesn't get collected until the main
> thread sends a postMessage to the (mostly-dead) worker thread?

Yup looks like... and that's exactly the part I wasn't fully sure about the
current code. But it happens only if the context had pending activity after
running the top-level script.

I think the current code's assumption is something like:
- GC the object if it is a short-living object, i.e. didn't start any pending
activity, after evaluating the top-level script, importing subresource scripts
and handling messages.
- Otherwise just keep it until the worker closes itself or the parent document
goes away.

And this probably works in an acceptable way in most cases.  I'd probably also
send the pending activity after the worker sending a message to the main
document (as worker would likely do so when it finishes what it's requested) if
we want to keep the current design.


> > > - Whenever the main thread sends a postMessage, the worker's
> > > ContextLifecycleNotifer needs to iterate all ActiveDOMObjects and check
> their
> > > pending activities. This sounds heavy.
> > 
> > Do we now how heavy it is?
> 
> Don't know :)
> 
> > > So I'd propose the following implementation:
> > > 
> > > - Move hasPendingActivity from ActiveDOMObject to ScriptWrappable.
> > > 
> > > - When the worker thread performs a GC, the worker thread iterates all
> > > ScriptWrappables (we're already doing this) and check their
> > > hasPendingActivities. The worker thread reports the result to
> > > Worker::m_hasPendingActivity (in the main thread).
> > >
> > > - When the main thread performs a GC, do not collect the worker object
while
> > > Worker::m_hasPendingActivity returns true.
> > > 
> > > In this implementation, the worker object will be collected after 1) the
> > worker
> > > thread lost all pending activities *and* 2) the worker thread performs a
GC
> > > *and* 3) the main thread performs a GC. This will extend the lifetime of
the
> > > worker object a bit longer than today though (because the worker object
> won't
> > be
> > > collected until both the worker thread and the main thread perform GCs).
> > > 
> > > What do you think?
> > 
> > Sounds reasonable to me, one question though- does it mean we assume worker
> > context : worker thread is 1:1?  In compositor worker cases for example we
> might
> > not collect worker objects on the worker thread if any of the worker context
> has
> > pending objects?
> 
> Good point. Yes, in my proposal, the worker object won't get collected if any
> worker context in the worker thread has a pending activity. (Maybe we can do
> something smarter though.)

I see. I don't have strong opinion on this redesign given that the current code
isn't aggressively doing GC either if it has any pending activity after
evaluating the script. Short-lived workers will start to live longer, and we
might want to keep watching memory regression.

[haraken, 2015-11-02 10:54:18]

Thanks for being persistent on this.

> > - I now understand how the worker object is kept alive while the worker
> context
> > has any pending activity, but I'm getting confused about how the worker
object
> > gets collected. In the current implementation, when does the worker object
(in
> > the main thread) get collected? As far as I understand, the worker object
> won't
> > get collected until the following event (*) happens.
> > 
> > (*) The main thread sends a postMessage to the worker thread after the
worker
> > thread loses all pending activities.
> > 
> > Doesn't this mean that the worker object doesn't get collected until the
main
> > thread sends a postMessage to the (mostly-dead) worker thread?
> 
> Yup looks like... and that's exactly the part I wasn't fully sure about the
> current code. But it happens only if the context had pending activity after
> running the top-level script.
> 
> I think the current code's assumption is something like:
> - GC the object if it is a short-living object, i.e. didn't start any pending
> activity, after evaluating the top-level script, importing subresource scripts
> and handling messages.
> - Otherwise just keep it until the worker closes itself or the parent document
> goes away.
> 
> And this probably works in an acceptable way in most cases.  I'd probably also
> send the pending activity after the worker sending a message to the main
> document (as worker would likely do so when it finishes what it's requested)
if
> we want to keep the current design.

What happens if we always just keep the worker object alive until the worker
closes itself or the parent document goes away? In other words, how helpful will
it be (for memory) to collect the worker object?

What matters for memory is when WorkerGlobalScope::dispose() gets called
(because it clears a V8 context and a bunch of heavy data structures). If I'm
not missing something, whether we collect the worker object (in the main thread)
or not is not related to the timing when WorkerGlobalScope::dispose() gets
called.

[kinuko, 2015-11-05 13:25:42]

On 2015/11/02 10:54:18, haraken wrote:
> Thanks for being persistent on this.
> 
> > > - I now understand how the worker object is kept alive while the worker
> > context
> > > has any pending activity, but I'm getting confused about how the worker
> object
> > > gets collected. In the current implementation, when does the worker object
> (in
> > > the main thread) get collected? As far as I understand, the worker object
> > won't
> > > get collected until the following event (*) happens.
> > > 
> > > (*) The main thread sends a postMessage to the worker thread after the
> worker
> > > thread loses all pending activities.
> > > 
> > > Doesn't this mean that the worker object doesn't get collected until the
> main
> > > thread sends a postMessage to the (mostly-dead) worker thread?
> > 
> > Yup looks like... and that's exactly the part I wasn't fully sure about the
> > current code. But it happens only if the context had pending activity after
> > running the top-level script.
> > 
> > I think the current code's assumption is something like:
> > - GC the object if it is a short-living object, i.e. didn't start any
pending
> > activity, after evaluating the top-level script, importing subresource
scripts
> > and handling messages.
> > - Otherwise just keep it until the worker closes itself or the parent
document
> > goes away.
> > 
> > And this probably works in an acceptable way in most cases.  I'd probably
also
> > send the pending activity after the worker sending a message to the main
> > document (as worker would likely do so when it finishes what it's requested)
> if
> > we want to keep the current design.
> 
> What happens if we always just keep the worker object alive until the worker
> closes itself or the parent document goes away? In other words, how helpful
will
> it be (for memory) to collect the worker object?
> 
> What matters for memory is when WorkerGlobalScope::dispose() gets called
> (because it clears a V8 context and a bunch of heavy data structures). If I'm
> not missing something, whether we collect the worker object (in the main
thread)
> or not is not related to the timing when WorkerGlobalScope::dispose() gets
> called.

Sorry for slow response, I was thinking about this while I don't have a clear
answer.  The Worker itself doesn't take too much memory, our preliminary
measurement we did sometime ago showed it'll take about 2.5MB, mostly in v8,
which doesn't seem big.

On the other hand it'd be somewhat observable from developers and it could cause
interoperability issues.  For example if a page repeatedly creates a new
one-shot worker just to process something (say, whenever an event happens), the
page's memory would just continue to increase if we don't GC them.  We could
tell the page author to explicitly call terminate() or close(), but it feels
it's a bug in our code, as it may not be needed in other browsers.  So if we
really want to make this change and we think worker should be explicitly closed
to be GC'ed I think we should probably file a spec issue.

[kinuko, 2015-11-05 14:00:25]

On 2015/11/05 13:25:42, kinuko wrote:
> On 2015/11/02 10:54:18, haraken wrote:
> > Thanks for being persistent on this.
> > 
> > > > - I now understand how the worker object is kept alive while the worker
> > > context
> > > > has any pending activity, but I'm getting confused about how the worker
> > object
> > > > gets collected. In the current implementation, when does the worker
object
> > (in
> > > > the main thread) get collected? As far as I understand, the worker
object
> > > won't
> > > > get collected until the following event (*) happens.
> > > > 
> > > > (*) The main thread sends a postMessage to the worker thread after the
> > worker
> > > > thread loses all pending activities.
> > > > 
> > > > Doesn't this mean that the worker object doesn't get collected until the
> > main
> > > > thread sends a postMessage to the (mostly-dead) worker thread?
> > > 
> > > Yup looks like... and that's exactly the part I wasn't fully sure about
the
> > > current code. But it happens only if the context had pending activity
after
> > > running the top-level script.
> > > 
> > > I think the current code's assumption is something like:
> > > - GC the object if it is a short-living object, i.e. didn't start any
> pending
> > > activity, after evaluating the top-level script, importing subresource
> scripts
> > > and handling messages.
> > > - Otherwise just keep it until the worker closes itself or the parent
> document
> > > goes away.
> > > 
> > > And this probably works in an acceptable way in most cases.  I'd probably
> also
> > > send the pending activity after the worker sending a message to the main
> > > document (as worker would likely do so when it finishes what it's
requested)
> > if
> > > we want to keep the current design.
> > 
> > What happens if we always just keep the worker object alive until the worker
> > closes itself or the parent document goes away? In other words, how helpful
> will
> > it be (for memory) to collect the worker object?
> > 
> > What matters for memory is when WorkerGlobalScope::dispose() gets called
> > (because it clears a V8 context and a bunch of heavy data structures). If
I'm
> > not missing something, whether we collect the worker object (in the main
> thread)
> > or not is not related to the timing when WorkerGlobalScope::dispose() gets
> > called.
> 
> Sorry for slow response, I was thinking about this while I don't have a clear
> answer.  The Worker itself doesn't take too much memory, our preliminary
> measurement we did sometime ago showed it'll take about 2.5MB, mostly in v8,
> which doesn't seem big.
> 
> On the other hand it'd be somewhat observable from developers and it could
cause
> interoperability issues.  For example if a page repeatedly creates a new
> one-shot worker just to process something (say, whenever an event happens),
the
> page's memory would just continue to increase if we don't GC them.  We could
> tell the page author to explicitly call terminate() or close(), but it feels
> it's a bug in our code, as it may not be needed in other browsers.  So if we
> really want to make this change and we think worker should be explicitly
closed
> to be GC'ed I think we should probably file a spec issue.

Would implementing something you described first (e.g. check pending activity
during worker thread GC) make the code complex?  If not I prefer we do that.  If
it feels unnecessarily complex we could surely file a spec bug.

[haraken, 2015-11-05 23:22:39]

On 2015/11/05 14:00:25, kinuko wrote:
> On 2015/11/05 13:25:42, kinuko wrote:
> > On 2015/11/02 10:54:18, haraken wrote:
> > > Thanks for being persistent on this.
> > > 
> > > > > - I now understand how the worker object is kept alive while the
worker
> > > > context
> > > > > has any pending activity, but I'm getting confused about how the
worker
> > > object
> > > > > gets collected. In the current implementation, when does the worker
> object
> > > (in
> > > > > the main thread) get collected? As far as I understand, the worker
> object
> > > > won't
> > > > > get collected until the following event (*) happens.
> > > > > 
> > > > > (*) The main thread sends a postMessage to the worker thread after the
> > > worker
> > > > > thread loses all pending activities.
> > > > > 
> > > > > Doesn't this mean that the worker object doesn't get collected until
the
> > > main
> > > > > thread sends a postMessage to the (mostly-dead) worker thread?
> > > > 
> > > > Yup looks like... and that's exactly the part I wasn't fully sure about
> the
> > > > current code. But it happens only if the context had pending activity
> after
> > > > running the top-level script.
> > > > 
> > > > I think the current code's assumption is something like:
> > > > - GC the object if it is a short-living object, i.e. didn't start any
> > pending
> > > > activity, after evaluating the top-level script, importing subresource
> > scripts
> > > > and handling messages.
> > > > - Otherwise just keep it until the worker closes itself or the parent
> > document
> > > > goes away.
> > > > 
> > > > And this probably works in an acceptable way in most cases.  I'd
probably
> > also
> > > > send the pending activity after the worker sending a message to the main
> > > > document (as worker would likely do so when it finishes what it's
> requested)
> > > if
> > > > we want to keep the current design.
> > > 
> > > What happens if we always just keep the worker object alive until the
worker
> > > closes itself or the parent document goes away? In other words, how
helpful
> > will
> > > it be (for memory) to collect the worker object?
> > > 
> > > What matters for memory is when WorkerGlobalScope::dispose() gets called
> > > (because it clears a V8 context and a bunch of heavy data structures). If
> I'm
> > > not missing something, whether we collect the worker object (in the main
> > thread)
> > > or not is not related to the timing when WorkerGlobalScope::dispose() gets
> > > called.
> > 
> > Sorry for slow response, I was thinking about this while I don't have a
clear
> > answer.  The Worker itself doesn't take too much memory, our preliminary
> > measurement we did sometime ago showed it'll take about 2.5MB, mostly in v8,
> > which doesn't seem big.
> > 
> > On the other hand it'd be somewhat observable from developers and it could
> cause
> > interoperability issues.  For example if a page repeatedly creates a new
> > one-shot worker just to process something (say, whenever an event happens),
> the
> > page's memory would just continue to increase if we don't GC them.  We could
> > tell the page author to explicitly call terminate() or close(), but it feels
> > it's a bug in our code, as it may not be needed in other browsers.  So if we
> > really want to make this change and we think worker should be explicitly
> closed
> > to be GC'ed I think we should probably file a spec issue.
> 
> Would implementing something you described first (e.g. check pending activity
> during worker thread GC) make the code complex?  If not I prefer we do that.

Thanks for a lot of advice. Let me try that approach.

[vivekg, 2015-12-01 04:10:45]

haraken@: Sorry about the delay in responding. I have been occupied with
internal work and it would be little unlikely that I can complete this activity.
Is there someone who can take this forward?

[haraken, 2015-12-02 04:14:54]

On 2015/12/01 04:10:45, vivekg_ wrote:
> haraken@: Sorry about the delay in responding. I have been occupied with
> internal work and it would be little unlikely that I can complete this
activity.
> Is there someone who can take this forward?

No one is working on this. Actually I'm getting lost :/

I tried to implement the approach described in #93 but realized that it wouldn't
work. Consider the following scenario:

1) The worker thread runs a GC. No pending activity is found.
2) The worker thread creates a new pending activity (e.g., setTimeout).
3) The main thread runs a GC. It collects the worker object because no pending
activity was observed at 1). This is wrong.

If we don't collect the worker object at 3), it means that we cannot collect the
worker object forever.

One solution would be to synchronously iterate all wrappers of the worker thread
at 3) and check if there is no pending activity. But it would be unacceptably
heavy.

In short, I have no idea for now.

[haraken, 2016-01-22 17:18:48]

On 2015/12/02 04:14:54, haraken wrote:
> On 2015/12/01 04:10:45, vivekg_ wrote:
> > haraken@: Sorry about the delay in responding. I have been occupied with
> > internal work and it would be little unlikely that I can complete this
> activity.
> > Is there someone who can take this forward?
> 
> No one is working on this. Actually I'm getting lost :/
> 
> I tried to implement the approach described in #93 but realized that it
wouldn't
> work. Consider the following scenario:
> 
> 1) The worker thread runs a GC. No pending activity is found.
> 2) The worker thread creates a new pending activity (e.g., setTimeout).
> 3) The main thread runs a GC. It collects the worker object because no pending
> activity was observed at 1). This is wrong.
> 
> If we don't collect the worker object at 3), it means that we cannot collect
the
> worker object forever.
> 
> One solution would be to synchronously iterate all wrappers of the worker
thread
> at 3) and check if there is no pending activity. But it would be unacceptably
> heavy.
> 
> In short, I have no idea for now.

I'll take over this work. I uploaded a CL to
https://codereview.chromium.org/1609343002/

[pfeldman, 2016-07-22 18:15:30]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org

[kozy, 2016-07-30 04:30:52]

kozyatinskiy@chromium.org changed reviewers:
- kozyatinskiy@chromium.org