[Streams] UnderlyingSourceBase should be kept alive only when locked

[Streams] UnderlyingSourceBase should be kept alive only when locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
Committed: https://crrev.com/68e61c0671cfb913078d5d48200292339e7b52cd
Cr-Commit-Position: refs/heads/master@{#389038}

[yhirano, 2016-04-20 03:14:21]

Description was changed from

==========
[Streams] UnderlyingSourceBase should kept alive only when stream is locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alvie until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

to

==========
[Streams] UnderlyingSourceBase should kept alive only when stream is locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

[yhirano, 2016-04-20 03:15:20]

Description was changed from

==========
[Streams] UnderlyingSourceBase should kept alive only when stream is locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

to

==========
[Streams] UnderlyingSourceBase should be kept alive only when stream is locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

[yhirano, 2016-04-20 03:40:06]

yhirano@chromium.org changed reviewers:
+ tyoshino@chromium.org

[yhirano, 2016-04-20 07:50:00]

Description was changed from

==========
[Streams] UnderlyingSourceBase should be kept alive only when stream is locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

to

==========
[Streams] UnderlyingSourceBase should be kept alive only when locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

[domenic, 2016-04-20 20:38:03]

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

[domenic, 2016-04-20 20:38:04]

Thanks very much for tracking this down. LGTM with a nit.

https://codereview.chromium.org/1902963003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ReadableStream.js (right):

https://codereview.chromium.org/1902963003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ReadableStream.js:284: CallOrNoop(
Should be `const underlyingSource = stream[readableStreamUnderlyingSource];
callFunction(underlyingSource.notifyLockAcquired, underlyingSource)`. No need
for the extra checks/fallback CallOrNoop does. (Same below.)

[yhirano, 2016-04-21 01:50:50]

https://codereview.chromium.org/1902963003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ReadableStream.js (right):

https://codereview.chromium.org/1902963003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ReadableStream.js:284: CallOrNoop(
On 2016/04/20 20:38:04, domenic wrote:
> Should be `const underlyingSource = stream[readableStreamUnderlyingSource];
> callFunction(underlyingSource.notifyLockAcquired, underlyingSource)`. No need
> for the extra checks/fallback CallOrNoop does. (Same below.)

Done.

[tyoshino (SeeGerritForStatus), 2016-04-21 05:54:15]

lgtm

[yhirano, 2016-04-21 05:58:21]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-04-21 05:58:22]

The patchset sent to the CQ was uploaded after l-g-t-m from domenic@chromium.org
Link to the patchset: https://codereview.chromium.org/1902963003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-21 05:58:26]

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

[commit-bot: I haz the power, 2016-04-21 05:58:27]

This CL has an open dependency (Issue 1899163002 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[yhirano, 2016-04-22 04:13:10]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-04-22 04:13:20]

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

[commit-bot: I haz the power, 2016-04-22 06:16:28]

Description was changed from

==========
[Streams] UnderlyingSourceBase should be kept alive only when locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

to

==========
[Streams] UnderlyingSourceBase should be kept alive only when locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

[commit-bot: I haz the power, 2016-04-22 06:16:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:44:44]

Description was changed from

==========
[Streams] UnderlyingSourceBase should be kept alive only when locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832
==========

to

==========
[Streams] UnderlyingSourceBase should be kept alive only when locked

In a situation that UnderlyingSourceBase lives in Blink and the associated
stream is exposed to the script, we're relying on ActiveScriptWrappable
mechanism to keep the underlying source alive even when it is unreachable from
the script, because it is very difficult to handle the following script with
other means.

let stream = ...;
let reader = stream.getReader();
let promise = reader.read();
reader = null;
stream = null;
// Now the script doesn't have a reference to the underlying source but we
// need to keep it alive until the promise resolves or rejects.

On the other hand, current UnderlyingSourceBase::hasPendingActivity returns
true when the associated controller is active and it's too leaky. For example,

fetch('/big-resource-with-cache-control-no-store');

leaks because no one read the body and the controller will stay active.

This CL introduces notifyLockAcquired and notifyLockReleased to
UnderlyingSourceBase to notify the locking status and make
UnderlyingSourceBase::hasPendingActivity use the information.

BUG=596832

Committed: https://crrev.com/68e61c0671cfb913078d5d48200292339e7b52cd
Cr-Commit-Position: refs/heads/master@{#389038}
==========

[commit-bot: I haz the power, 2016-04-22 19:44:45]

Patchset 3 (id:??) landed as
https://crrev.com/68e61c0671cfb913078d5d48200292339e7b52cd
Cr-Commit-Position: refs/heads/master@{#389038}