Cleanly detach XHR and other pending loader clients from Inspector.

Cleanly detach XHR and other pending loader clients from Inspector.

If the XHR object is finalized without first being notified of
ExecutionContext destruction, its prefinalizer is responsible for making
up the difference and behave as if that did.

Do so by delegating to contextDestroyed(); this takes care of
releasing its resources promptly, along with unregistering as
a loader client (with its associated async loader and Inspector.)

Also make other Inspector loading clients cleanly detach when
finalized; prevents Inspector from keeping dead raw pointers to them.

R=yhirano,haraken
BUG=667254
Review-Url: https://codereview.chromium.org/2649323005
Cr-Commit-Position: refs/heads/master@{#446660}
Committed: https://chromium.googlesource.com/chromium/src/+/ef8b2502b396988865f9d4754748a3767a40160e

[sof, 2017-01-24 19:42:04]

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

[commit-bot: I haz the power, 2017-01-24 19:42:37]

Dry run: 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, 2017-01-24 21:03:20]

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

[commit-bot: I haz the power, 2017-01-24 21:03:21]

Dry run: This issue passed the CQ dry run.

[sof, 2017-01-25 09:57:26]

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

[commit-bot: I haz the power, 2017-01-25 09:57:42]

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

[sof, 2017-01-25 10:15:14]

Description was changed from

==========
Upon abrupt disposal, fully detach XMLHttpRequest object.

If the XHR object is finalized without first being notified
of ExecutionContext destruction, its prefinalizer is responsible
for making up the difference and behave as if that did.

Do so by calling upon contextDestroyed(); this takes care of
detaching as loader clients along with releasing its resources
promptly.

R=
BUG=667254
==========

to

==========
Cleanly detach XHR and other pending loader clients from Inspector.

If the XHR object is finalized without first being notified of
ExecutionContext destruction, its prefinalizer is responsible for making
up the difference and behave as if that did.

Do so by delegating to contextDestroyed(); this takes care of
releasing its resources promptly, along with unregistering as
a loader client (with its associated async loader and Inspector.)

Also make other Inspector loading clients cleanly detach when
finalized; prevents Inspector from keeping dead raw pointers to them.

R=
BUG=667254
==========

[sof, 2017-01-25 10:54:29]

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

[sof, 2017-01-25 10:54:30]

please take a look.

[yhirano, 2017-01-25 11:05:03]

haraken, sof: I think contextDestroyed not being called is a bug, do you agree
with that? Or we will need to call contextDestroyed in pre-finalizer in general,
forever? contextDestroyed can be complex and I don't like to call complex
functions in a pre-finalizer, so I don't like to call contextDestroyed in
pre-finalizer in general.

[yhirano, 2017-01-25 11:05:04]

haraken, sof: I think contextDestroyed not being called is a bug, do you agree
with that? Or we will need to call contextDestroyed in pre-finalizer in general,
forever? contextDestroyed can be complex and I don't like to call complex
functions in a pre-finalizer, so I don't like to call contextDestroyed in
pre-finalizer in general.

[yhirano, 2017-01-25 11:06:26]

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
(right):

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:216: void
detachClientRequest(ExecutionContext*, ThreadableLoaderClient*);
Please mention that this can be called from a pre-finalizer.

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1043: void
InspectorNetworkAgent::detachClientRequest(
ditto

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:153: void
detachClientRequest(ThreadableLoaderClient*);
ditto

[sof, 2017-01-25 11:08:40]

On 2017/01/25 11:05:04, yhirano wrote:
> haraken, sof: I think contextDestroyed not being called is a bug, do you agree
> with that? Or we will need to call contextDestroyed in pre-finalizer in
general,
> forever? contextDestroyed can be complex and I don't like to call complex
> functions in a pre-finalizer, so I don't like to call contextDestroyed in
> pre-finalizer in general.

contextDestroyed() will not be called for documents that are frame attached, for
one.

We prefer to do less work in prefinalizers, but touching other heap objects and
allocating, is not a problem. If that is required; we're touching a heap hash
map here and the XHR throttle object.

[sof, 2017-01-25 11:09:51]

On 2017/01/25 11:08:40, sof wrote:
> On 2017/01/25 11:05:04, yhirano wrote:
> > haraken, sof: I think contextDestroyed not being called is a bug, do you
agree
> > with that? Or we will need to call contextDestroyed in pre-finalizer in
> general,
> > forever? contextDestroyed can be complex and I don't like to call complex
> > functions in a pre-finalizer, so I don't like to call contextDestroyed in
> > pre-finalizer in general.
> 
> contextDestroyed() will not be called for documents that are frame attached,
for
> one.
> 
> We prefer to do less work in prefinalizers, but touching other heap objects
and
> allocating, is not a problem. If that is required; we're touching a heap hash
> map here and the XHR throttle object.

s/are frame/aren't frame/ :)

[commit-bot: I haz the power, 2017-01-25 11:40:11]

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

[commit-bot: I haz the power, 2017-01-25 11:40:12]

Dry run: This issue passed the CQ dry run.

[sof, 2017-01-25 11:44:30]

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
(right):

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:216: void
detachClientRequest(ExecutionContext*, ThreadableLoaderClient*);
On 2017/01/25 11:06:26, yhirano wrote:
> Please mention that this can be called from a pre-finalizer.

Hmm. I don't see the value of adding that comment, especially not here. Calling
detach/clear/dispose methods from prefinalizers, is something we do throughout
the codebase. Revivification bugs due to prefinalizer actions over-extending the
lifetime of a near-dead object, I cannot recall having seen many issues on since
introducing Oilpan prefinalizers, will be caught out by ASan.

It would be unexpected that a detach method like this would do something like
this in any case; detach means letting go.

[yhirano, 2017-01-27 07:57:59]

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
(right):

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:216: void
detachClientRequest(ExecutionContext*, ThreadableLoaderClient*);
On 2017/01/25 11:44:30, sof wrote:
> On 2017/01/25 11:06:26, yhirano wrote:
> > Please mention that this can be called from a pre-finalizer.
> 
> Hmm. I don't see the value of adding that comment, especially not here.
Calling
> detach/clear/dispose methods from prefinalizers, is something we do throughout
> the codebase. Revivification bugs due to prefinalizer actions over-extending
the
> lifetime of a near-dead object, I cannot recall having seen many issues on
since
> introducing Oilpan prefinalizers, will be caught out by ASan.
> 
> It would be unexpected that a detach method like this would do something like
> this in any case; detach means letting go.

LocalFrame::detach does a lot of things so I don't think it's super-clear. I'm
asking for the comment because inspector is an external module from XHR.

[sof, 2017-01-27 08:40:35]

thanks for the followup, getting a fix in sooner for this is preferable.

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
(right):

https://codereview.chromium.org/2649323005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:216: void
detachClientRequest(ExecutionContext*, ThreadableLoaderClient*);
On 2017/01/27 07:57:59, yhirano wrote:
> On 2017/01/25 11:44:30, sof wrote:
> > On 2017/01/25 11:06:26, yhirano wrote:
> > > Please mention that this can be called from a pre-finalizer.
> > 
> > Hmm. I don't see the value of adding that comment, especially not here.
> Calling
> > detach/clear/dispose methods from prefinalizers, is something we do
throughout
> > the codebase. Revivification bugs due to prefinalizer actions over-extending
> the
> > lifetime of a near-dead object, I cannot recall having seen many issues on
> since
> > introducing Oilpan prefinalizers, will be caught out by ASan.
> > 
> > It would be unexpected that a detach method like this would do something
like
> > this in any case; detach means letting go.
> 
> LocalFrame::detach does a lot of things so I don't think it's super-clear. I'm
> asking for the comment because inspector is an external module from XHR.

ok.. clarified.

[yhirano, 2017-01-27 08:58:44]

lgtm, thanks.

https://codereview.chromium.org/2649323005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/eventsource/EventSource.cpp (right):

https://codereview.chromium.org/2649323005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/eventsource/EventSource.cpp:376: void
EventSource::contextDestroyed(ExecutionContext*) {
InspectorInstrumentation::detachClientRequest(executionContext, this) needed?

[sof, 2017-01-27 09:09:40]

haraken@: owner review when you next have moment, please?

https://codereview.chromium.org/2649323005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/eventsource/EventSource.cpp (right):

https://codereview.chromium.org/2649323005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/eventsource/EventSource.cpp:376: void
EventSource::contextDestroyed(ExecutionContext*) {
On 2017/01/27 08:58:44, yhirano wrote:
> InspectorInstrumentation::detachClientRequest(executionContext, this) needed?

The cancellation happening during close() ought to handle the unregistration wrt
Inspector, but might as make it clear and do it early.

[haraken, 2017-01-27 09:47:58]

LGTM

https://codereview.chromium.org/2649323005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/2649323005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1097:
contextDestroyed(getExecutionContext());

I'd prefer not manually calling contextDestroyed (it should be called only by
LifecycleNotifer). Can we instead make contextDestroyed() call dispose()?

[sof, 2017-01-27 10:19:17]

Description was changed from

==========
Cleanly detach XHR and other pending loader clients from Inspector.

If the XHR object is finalized without first being notified of
ExecutionContext destruction, its prefinalizer is responsible for making
up the difference and behave as if that did.

Do so by delegating to contextDestroyed(); this takes care of
releasing its resources promptly, along with unregistering as
a loader client (with its associated async loader and Inspector.)

Also make other Inspector loading clients cleanly detach when
finalized; prevents Inspector from keeping dead raw pointers to them.

R=
BUG=667254
==========

to

==========
Cleanly detach XHR and other pending loader clients from Inspector.

If the XHR object is finalized without first being notified of
ExecutionContext destruction, its prefinalizer is responsible for making
up the difference and behave as if that did.

Do so by delegating to contextDestroyed(); this takes care of
releasing its resources promptly, along with unregistering as
a loader client (with its associated async loader and Inspector.)

Also make other Inspector loading clients cleanly detach when
finalized; prevents Inspector from keeping dead raw pointers to them.

R=yhirano,haraken
BUG=667254
==========

[sof, 2017-01-27 10:19:54]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-01-27 10:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2649323005/#ps80001
(title: "rebalance XHR disposal steps")

[sof, 2017-01-27 10:19:59]

https://codereview.chromium.org/2649323005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/2649323005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1097:
contextDestroyed(getExecutionContext());
On 2017/01/27 09:47:57, haraken wrote:
> 
> I'd prefer not manually calling contextDestroyed (it should be called only by
> LifecycleNotifer). Can we instead make contextDestroyed() call dispose()?

ok, adjusted.

[commit-bot: I haz the power, 2017-01-27 10:20:25]

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, 2017-01-27 13:52:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485512394242850,
"parent_rev": "0db58634865bb99accfe011eb7fcc313476f615e", "commit_rev":
"ef8b2502b396988865f9d4754748a3767a40160e"}

[commit-bot: I haz the power, 2017-01-27 13:52:57]

Description was changed from

==========
Cleanly detach XHR and other pending loader clients from Inspector.

If the XHR object is finalized without first being notified of
ExecutionContext destruction, its prefinalizer is responsible for making
up the difference and behave as if that did.

Do so by delegating to contextDestroyed(); this takes care of
releasing its resources promptly, along with unregistering as
a loader client (with its associated async loader and Inspector.)

Also make other Inspector loading clients cleanly detach when
finalized; prevents Inspector from keeping dead raw pointers to them.

R=yhirano,haraken
BUG=667254
==========

to

==========
Cleanly detach XHR and other pending loader clients from Inspector.

If the XHR object is finalized without first being notified of
ExecutionContext destruction, its prefinalizer is responsible for making
up the difference and behave as if that did.

Do so by delegating to contextDestroyed(); this takes care of
releasing its resources promptly, along with unregistering as
a loader client (with its associated async loader and Inspector.)

Also make other Inspector loading clients cleanly detach when
finalized; prevents Inspector from keeping dead raw pointers to them.

R=yhirano,haraken
BUG=667254

Review-Url: https://codereview.chromium.org/2649323005
Cr-Commit-Position: refs/heads/master@{#446660}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef8b2502b396988865f9d4754748...
==========

[commit-bot: I haz the power, 2017-01-27 13:52:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ef8b2502b396988865f9d4754748...