Signal no pending activity in destructed contexts.

Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=

Committed: https://crrev.com/cf43a3b249e837503fad6d815d74346d7a187f84
Cr-Commit-Position: refs/heads/master@{#438787}

[sof, 2016-12-14 13:55:56]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 13:56:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-12-14 14:03:49]

sigbjornf@opera.com changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[sof, 2016-12-14 14:03:50]

please take a look.

[sof, 2016-12-14 14:27:31]

Description was changed from

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't take the state of the
ExecutionContext into account, only considering if event listeners are
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the tests.

R=
BUG=658240
==========

to

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

R=
BUG=658240
==========

[commit-bot: I haz the power, 2016-12-14 15:20:20]

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

[commit-bot: I haz the power, 2016-12-14 15:20:21]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-14 15:56:20]

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

[haraken, 2016-12-14 15:56:20]

LGTM

Before enabling the wrapper tracing, we don't need the getExecutionContext()
check, because we forcibly return false from hasPendingActivity on a detached
context:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

However, we don't yet have the logic in the wrapper tracing. So your change is
needed after enabling the wrapper tracing.

An ideal fix would be to add ActiveScriptWrappable::m_executionContext and make
ActiveScriptWrappable::hasPendingActivity return false after the context is
detached.

[sof, 2016-12-14 16:21:56]

Description was changed from

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

R=
BUG=658240
==========

to

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=658240
==========

[sof, 2016-12-14 16:26:33]

On 2016/12/14 15:56:20, haraken wrote:
> LGTM
> 
> Before enabling the wrapper tracing, we don't need the getExecutionContext()
> check, because we forcibly return false from hasPendingActivity on a detached
> context:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> 
> However, we don't yet have the logic in the wrapper tracing. So your change is
> needed after enabling the wrapper tracing.
> 
> An ideal fix would be to add ActiveScriptWrappable::m_executionContext and
make
> ActiveScriptWrappable::hasPendingActivity return false after the context is
> detached.

Yes, it'd be better if this detail could be kept out of hasPendingActivity()
implementations and instead be provided "for free". Extending all
ActiveScriptWrappables with an extra field is not "free" for those
implementations which doesn't override hasPendingActivity(), unfortunately.

I've updated the description to make it more accurate.

[dcheng, 2016-12-14 17:03:11]

On 2016/12/14 16:26:33, sof wrote:
> On 2016/12/14 15:56:20, haraken wrote:
> > LGTM
> > 
> > Before enabling the wrapper tracing, we don't need the getExecutionContext()
> > check, because we forcibly return false from hasPendingActivity on a
detached
> > context:
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > 
> > However, we don't yet have the logic in the wrapper tracing. So your change
is
> > needed after enabling the wrapper tracing.
> > 
> > An ideal fix would be to add ActiveScriptWrappable::m_executionContext and
> make
> > ActiveScriptWrappable::hasPendingActivity return false after the context is
> > detached.
> 
> Yes, it'd be better if this detail could be kept out of hasPendingActivity()
> implementations and instead be provided "for free". Extending all
> ActiveScriptWrappables with an extra field is not "free" for those
> implementations which doesn't override hasPendingActivity(), unfortunately.
> 
> I've updated the description to make it more accurate.

This seems a bit fragile. There's a number of places that override this, and
they all check this slightly differently. It's also hard to tell if they're
correct just by inspection. For example, DOMFileSystem::hasPendingActivity()
just check that there are a positive number of callbacks. It's hard for me to
tell if that check is correct or not, given this patch.

[sof, 2016-12-14 18:16:40]

On 2016/12/14 17:03:11, dcheng wrote:
> On 2016/12/14 16:26:33, sof wrote:
> > On 2016/12/14 15:56:20, haraken wrote:
> > > LGTM
> > > 
> > > Before enabling the wrapper tracing, we don't need the
getExecutionContext()
> > > check, because we forcibly return false from hasPendingActivity on a
> detached
> > > context:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > > 
> > > However, we don't yet have the logic in the wrapper tracing. So your
change
> is
> > > needed after enabling the wrapper tracing.
> > > 
> > > An ideal fix would be to add ActiveScriptWrappable::m_executionContext and
> > make
> > > ActiveScriptWrappable::hasPendingActivity return false after the context
is
> > > detached.
> > 
> > Yes, it'd be better if this detail could be kept out of hasPendingActivity()
> > implementations and instead be provided "for free". Extending all
> > ActiveScriptWrappables with an extra field is not "free" for those
> > implementations which doesn't override hasPendingActivity(), unfortunately.
> > 
> > I've updated the description to make it more accurate.
> 
> This seems a bit fragile. There's a number of places that override this, and
> they all check this slightly differently. It's also hard to tell if they're
> correct just by inspection. For example, DOMFileSystem::hasPendingActivity()
> just check that there are a positive number of callbacks. It's hard for me to
> tell if that check is correct or not, given this patch.

I don't understand what you expect to see. Each object has a different notion of
must-stay-alive, that's why they override the default of returning |false|. What
commonality do you expect to see?

[dcheng, 2016-12-14 18:29:35]

On 2016/12/14 18:16:40, sof wrote:
> On 2016/12/14 17:03:11, dcheng wrote:
> > On 2016/12/14 16:26:33, sof wrote:
> > > On 2016/12/14 15:56:20, haraken wrote:
> > > > LGTM
> > > > 
> > > > Before enabling the wrapper tracing, we don't need the
> getExecutionContext()
> > > > check, because we forcibly return false from hasPendingActivity on a
> > detached
> > > > context:
> > > > 
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> > > > 
> > > > However, we don't yet have the logic in the wrapper tracing. So your
> change
> > is
> > > > needed after enabling the wrapper tracing.
> > > > 
> > > > An ideal fix would be to add ActiveScriptWrappable::m_executionContext
and
> > > make
> > > > ActiveScriptWrappable::hasPendingActivity return false after the context
> is
> > > > detached.
> > > 
> > > Yes, it'd be better if this detail could be kept out of
hasPendingActivity()
> > > implementations and instead be provided "for free". Extending all
> > > ActiveScriptWrappables with an extra field is not "free" for those
> > > implementations which doesn't override hasPendingActivity(),
unfortunately.
> > > 
> > > I've updated the description to make it more accurate.
> > 
> > This seems a bit fragile. There's a number of places that override this, and
> > they all check this slightly differently. It's also hard to tell if they're
> > correct just by inspection. For example, DOMFileSystem::hasPendingActivity()
> > just check that there are a positive number of callbacks. It's hard for me
to
> > tell if that check is correct or not, given this patch.
> 
> I don't understand what you expect to see. Each object has a different notion
of
> must-stay-alive, that's why they override the default of returning |false|.
What
> commonality do you expect to see?

Right, but when is it OK for hasPendingActivity() to not check that the context
is still live? Is there some guideline here?

[sof, 2016-12-14 19:26:35]

On 2016/12/14 18:29:35, dcheng wrote:
> On 2016/12/14 18:16:40, sof wrote:
> > On 2016/12/14 17:03:11, dcheng wrote:
> > > On 2016/12/14 16:26:33, sof wrote:
> > > > On 2016/12/14 15:56:20, haraken wrote:
...
> > > > 
> > > > Yes, it'd be better if this detail could be kept out of
> hasPendingActivity()
> > > > implementations and instead be provided "for free". Extending all
> > > > ActiveScriptWrappables with an extra field is not "free" for those
> > > > implementations which doesn't override hasPendingActivity(),
> unfortunately.
> > > > 
> > > > I've updated the description to make it more accurate.
> > > 
> > > This seems a bit fragile. There's a number of places that override this,
and
> > > they all check this slightly differently. It's also hard to tell if
they're
> > > correct just by inspection. For example,
DOMFileSystem::hasPendingActivity()
> > > just check that there are a positive number of callbacks. It's hard for me
> to
> > > tell if that check is correct or not, given this patch.
> > 
> > I don't understand what you expect to see. Each object has a different
notion
> of
> > must-stay-alive, that's why they override the default of returning |false|.
> What
> > commonality do you expect to see?
> 
> Right, but when is it OK for hasPendingActivity() to not check that the
context
> is still live? Is there some guideline here?

Only that the predicate must transition to |false| eventually to prevent
leaks/retention. I'm not aware of cases where that's not what's wanted, but
haraken@ may have come across some.

Some implementations have weaker forms of life-end than EOL for the
ExecutionContext, but could the implementations be more regular? Definitely;
looking closer, I think extending ActiveScriptWrappables with an
ExecutionContext member to handle the detached ExecutionContext case, is the way
to go.

[Michael Lippautz, 2016-12-14 21:45:46]

On 2016/12/14 19:26:35, sof wrote:
> On 2016/12/14 18:29:35, dcheng wrote:
> > On 2016/12/14 18:16:40, sof wrote:
> > > On 2016/12/14 17:03:11, dcheng wrote:
> > > > On 2016/12/14 16:26:33, sof wrote:
> > > > > On 2016/12/14 15:56:20, haraken wrote:
> ...
> > > > > 
> > > > > Yes, it'd be better if this detail could be kept out of
> > hasPendingActivity()
> > > > > implementations and instead be provided "for free". Extending all
> > > > > ActiveScriptWrappables with an extra field is not "free" for those
> > > > > implementations which doesn't override hasPendingActivity(),
> > unfortunately.
> > > > > 
> > > > > I've updated the description to make it more accurate.
> > > > 
> > > > This seems a bit fragile. There's a number of places that override this,
> and
> > > > they all check this slightly differently. It's also hard to tell if
> they're
> > > > correct just by inspection. For example,
> DOMFileSystem::hasPendingActivity()
> > > > just check that there are a positive number of callbacks. It's hard for
me
> > to
> > > > tell if that check is correct or not, given this patch.
> > > 
> > > I don't understand what you expect to see. Each object has a different
> notion
> > of
> > > must-stay-alive, that's why they override the default of returning
|false|.
> > What
> > > commonality do you expect to see?
> > 
> > Right, but when is it OK for hasPendingActivity() to not check that the
> context
> > is still live? Is there some guideline here?
> 
> Only that the predicate must transition to |false| eventually to prevent
> leaks/retention. I'm not aware of cases where that's not what's wanted, but
> haraken@ may have come across some.
> 
> Some implementations have weaker forms of life-end than EOL for the
> ExecutionContext, but could the implementations be more regular? Definitely;
> looking closer, I think extending ActiveScriptWrappables with an
> ExecutionContext member to handle the detached ExecutionContext case, is the
way
> to go.

lgtm, thanks for working on this

[haraken, 2016-12-15 00:10:03]

On 2016/12/14 19:26:35, sof wrote:
> On 2016/12/14 18:29:35, dcheng wrote:
> > On 2016/12/14 18:16:40, sof wrote:
> > > On 2016/12/14 17:03:11, dcheng wrote:
> > > > On 2016/12/14 16:26:33, sof wrote:
> > > > > On 2016/12/14 15:56:20, haraken wrote:
> ...
> > > > > 
> > > > > Yes, it'd be better if this detail could be kept out of
> > hasPendingActivity()
> > > > > implementations and instead be provided "for free". Extending all
> > > > > ActiveScriptWrappables with an extra field is not "free" for those
> > > > > implementations which doesn't override hasPendingActivity(),
> > unfortunately.
> > > > > 
> > > > > I've updated the description to make it more accurate.
> > > > 
> > > > This seems a bit fragile. There's a number of places that override this,
> and
> > > > they all check this slightly differently. It's also hard to tell if
> they're
> > > > correct just by inspection. For example,
> DOMFileSystem::hasPendingActivity()
> > > > just check that there are a positive number of callbacks. It's hard for
me
> > to
> > > > tell if that check is correct or not, given this patch.
> > > 
> > > I don't understand what you expect to see. Each object has a different
> notion
> > of
> > > must-stay-alive, that's why they override the default of returning
|false|.
> > What
> > > commonality do you expect to see?
> > 
> > Right, but when is it OK for hasPendingActivity() to not check that the
> context
> > is still live? Is there some guideline here?

We should always check getExecutionContext(). Before enabling the wrapper
tracing, we're doing that in V8GCController
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...).

After enabling the wrapper tracing, we should do that in ActiveScriptWrappable,
but we don't yet have that logic. So Sigbjorn is trying to land this CL  as a
short-term fix.

> 
> Only that the predicate must transition to |false| eventually to prevent
> leaks/retention. I'm not aware of cases where that's not what's wanted, but
> haraken@ may have come across some.
> 
> Some implementations have weaker forms of life-end than EOL for the
> ExecutionContext, but could the implementations be more regular? Definitely;
> looking closer, I think extending ActiveScriptWrappables with an
> ExecutionContext member to handle the detached ExecutionContext case, is the
way
> to go.

[dcheng, 2016-12-15 00:31:10]

Ok, LGTM

[sof, 2016-12-15 08:29:46]

Description was changed from

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=658240
==========

to

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=
==========

[sof, 2016-12-15 08:32:21]

I'll get on to that ActiveScriptWrappable change right away, if no one else is
busy working on this already.

[haraken, 2016-12-15 08:40:01]

On 2016/12/15 08:32:21, sof wrote:
> I'll get on to that ActiveScriptWrappable change right away, if no one else is
> busy working on this already.

Thanks!

[sof, 2016-12-15 08:49:52]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-12-15 08:50:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 08:53:43]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481791792154960, "parent_rev":
"c8e73edbf82a563e16340440b995852861fb918a", "commit_rev":
"8d0fa0e67ece016c7ca2921a66749e3c52d9e895"}

[commit-bot: I haz the power, 2016-12-15 08:54:11]

Description was changed from

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=
==========

to

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=

Review-Url: https://codereview.chromium.org/2571193002
==========

[commit-bot: I haz the power, 2016-12-15 08:54:12]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-15 08:56:42]

Description was changed from

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=

Review-Url: https://codereview.chromium.org/2571193002
==========

to

==========
Signal no pending activity in destructed contexts.

Various hasPendingActivity() overrides weren't taking the state of the
ExecutionContext into account, only considering if event listeners were
registered. We're not interested in holding onto a script environment
after an execution context has been destroyed, so adjust the predicates
to return false if the ExecutionContext has been destructed.

The V8GCController wrapper visitors already check if hasPendingActivity()
implementations incorrectly return |true| when used inside of destroyed
ExecutionContexts, but that check is not handled by trace wrappers
(ActiveScriptWrappable.)

R=
BUG=

Committed: https://crrev.com/cf43a3b249e837503fad6d815d74346d7a187f84
Cr-Commit-Position: refs/heads/master@{#438787}
==========

[commit-bot: I haz the power, 2016-12-15 08:56:43]

Patchset 1 (id:??) landed as
https://crrev.com/cf43a3b249e837503fad6d815d74346d7a187f84
Cr-Commit-Position: refs/heads/master@{#438787}