BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige

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

[hiroshige, 2016-07-12 10:17:32]

lgtm.

[haraken, 2016-07-12 10:29:51]

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

[haraken, 2016-07-12 10:29:51]

https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:277: if (!m_reader
|| !m_scriptState->getExecutionContext() ||
m_scriptState->getExecutionContext()->activeDOMObjectsAreStopped())

Would it be better to clear m_reader when contextDestroyed gets called?

[yhirano, 2016-07-12 10:30:05]

Description was changed from

==========
BodyStreamBuffer shouldn't work when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige
==========

to

==========
BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige
==========

[yhirano, 2016-07-12 10:30:05]

yhirano@chromium.org changed reviewers:
- haraken@chromium.org

[yhirano, 2016-07-12 10:30:17]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-07-12 10:30:19]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2016-07-12 10:32:33]

On 2016/07/12 10:29:51, haraken wrote:
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):
> 
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:277: if
(!m_reader
> || !m_scriptState->getExecutionContext() ||
> m_scriptState->getExecutionContext()->activeDOMObjectsAreStopped())
> 
> Would it be better to clear m_reader when contextDestroyed gets called?

We've already done it (see |stop|), but it's possible that didGetReadable is
called via another object's |stop|.

[haraken, 2016-07-12 10:35:54]

On 2016/07/12 10:32:33, yhirano wrote:
> On 2016/07/12 10:29:51, haraken wrote:
> >
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:277: if
> (!m_reader
> > || !m_scriptState->getExecutionContext() ||
> > m_scriptState->getExecutionContext()->activeDOMObjectsAreStopped())
> > 
> > Would it be better to clear m_reader when contextDestroyed gets called?
> 
> We've already done it (see |stop|), but it's possible that didGetReadable is
> called via another object's |stop|.

Hmm, that sounds nasty. What object calls didGetReadable?

[yhirano, 2016-07-12 10:41:40]

On 2016/07/12 10:35:54, haraken wrote:
> On 2016/07/12 10:32:33, yhirano wrote:
> > On 2016/07/12 10:29:51, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> > > File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> > > third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:277: if
> > (!m_reader
> > > || !m_scriptState->getExecutionContext() ||
> > > m_scriptState->getExecutionContext()->activeDOMObjectsAreStopped())
> > > 
> > > Would it be better to clear m_reader when contextDestroyed gets called?
> > 
> > We've already done it (see |stop|), but it's possible that didGetReadable is
> > called via another object's |stop|.
> 
> Hmm, that sounds nasty. What object calls didGetReadable?

For example:

SourceContext::stop (in DataConsumerTee.cpp)
 -> SourceContext::stopInternal
 -> DestinationContext::setResult (in DataConsumerTee.cpp)
 -> DestinationContext::notify
 -> BodyStreamBuffer::didGetReadable

[yhirano, 2016-07-14 14:13:08]

On 2016/07/12 10:41:40, yhirano wrote:
> On 2016/07/12 10:35:54, haraken wrote:
> > On 2016/07/12 10:32:33, yhirano wrote:
> > > On 2016/07/12 10:29:51, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> > > > File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2146453002/diff/1/third_party/WebKit/Source/m...
> > > > third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:277: if
> > > (!m_reader
> > > > || !m_scriptState->getExecutionContext() ||
> > > > m_scriptState->getExecutionContext()->activeDOMObjectsAreStopped())
> > > > 
> > > > Would it be better to clear m_reader when contextDestroyed gets called?
> > > 
> > > We've already done it (see |stop|), but it's possible that didGetReadable
is
> > > called via another object's |stop|.
> > 
> > Hmm, that sounds nasty. What object calls didGetReadable?
> 
> For example:
> 
> SourceContext::stop (in DataConsumerTee.cpp)
>  -> SourceContext::stopInternal
>  -> DestinationContext::setResult (in DataConsumerTee.cpp)
>  -> DestinationContext::notify
>  -> BodyStreamBuffer::didGetReadable

haraken@, do you have further comments, or is it OK for you to land this change?

[hiroshige, 2016-07-15 10:06:16]

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

[yhirano, 2016-07-20 01:44:45]

haraken@: ping

[haraken, 2016-07-20 07:46:14]

On 2016/07/20 01:44:45, yhirano wrote:
> haraken@: ping

Sorry, I missed this one...

Would it make more sense to clear DestinationContext::m_client so that
DestinationContext::notify does not need to call in
BodyStreamBuffer::didGetReadable?

[yhirano, 2016-07-20 08:05:49]

On 2016/07/20 07:46:14, haraken wrote:
> On 2016/07/20 01:44:45, yhirano wrote:
> > haraken@: ping
> 
> Sorry, I missed this one...
> 
> Would it make more sense to clear DestinationContext::m_client so that
> DestinationContext::notify does not need to call in
> BodyStreamBuffer::didGetReadable?

Theoretically it is possible that DestinationContext's ExecutionContext is
different from BodyStreamBuffer's ExecutionContext (same-origin iframe for
example). In such a case we need to call BodyStreamBuffer::didGetReadable to
notify the error, because the BodyStreamBuffer is associated with an active
ExecutionContext.

[haraken, 2016-07-20 08:25:34]

On 2016/07/20 08:05:49, yhirano wrote:
> On 2016/07/20 07:46:14, haraken wrote:
> > On 2016/07/20 01:44:45, yhirano wrote:
> > > haraken@: ping
> > 
> > Sorry, I missed this one...
> > 
> > Would it make more sense to clear DestinationContext::m_client so that
> > DestinationContext::notify does not need to call in
> > BodyStreamBuffer::didGetReadable?
> 
> Theoretically it is possible that DestinationContext's ExecutionContext is
> different from BodyStreamBuffer's ExecutionContext (same-origin iframe for
> example). In such a case we need to call BodyStreamBuffer::didGetReadable to
> notify the error, because the BodyStreamBuffer is associated with an active
> ExecutionContext.

ok, LGTM

[yhirano, 2016-07-20 08:31:51]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-07-20 08:31:51]

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

[commit-bot: I haz the power, 2016-07-20 08:32:14]

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-07-20 13:07:38]

Description was changed from

==========
BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige
==========

to

==========
BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige
==========

[commit-bot: I haz the power, 2016-07-20 13:07:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-20 13:10:00]

Description was changed from

==========
BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige
==========

to

==========
BodyStreamBuffer shouldn't run scripts when ActiveDOMObjects are stopped

Some DataConsumerHandle subclasses close the handle when ActiveDOMObjects are
stopped. BodyStreamBuffer::didGetReadable should not do anything because
executing scripts are forbidden in such cases. As BodyStreamBuffer::stop will
be called shortly, just returning early is enough.

BUG=625563
R=hiroshige

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

[commit-bot: I haz the power, 2016-07-20 13:10:01]

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