Protect RawResource from deletion in RawResource::didSendData() and didDownloadData()

Protect RawResource from deletion in RawResource::didSendData() and didDownloadData()

ResourceClientWalker uses a reference to m_clients (a member of the
RawResource instance). Therefore, when w.next() is invoked, the RawResource
instance must be alive.

RawResourceClient (e.g. DocumentThreadableLoader) may destroy the RawResource
in didSendData() and didDownloadData().

Like other methods (appendData(), willFollowRedirect(), ...) these methods
should also have a protect.

BUG=527046
R=japhet,hiroshige

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201626

[tyoshino (SeeGerritForStatus), 2015-09-01 13:52:03]

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

[tyoshino (SeeGerritForStatus), 2015-09-01 13:53:43]

tyoshino@chromium.org changed reviewers:
+ japhet@chromium.org
- mkwst@chromium.org

[tyoshino (SeeGerritForStatus), 2015-09-01 13:54:07]

mkwst is OOO?

Adding japhet@.

[tyoshino (SeeGerritForStatus), 2015-09-01 15:46:21]

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

[tyoshino (SeeGerritForStatus), 2015-09-01 15:46:28]

On 2015/09/01 13:54:07, tyoshino wrote:
> mkwst is OOO?
> 
> Adding japhet@.

also +hiroshige

[Nate Chapin, 2015-09-01 17:31:37]

LGTM. Test?

[hiroshige, 2015-09-01 19:26:31]

On 2015/09/01 17:31:37, Nate Chapin wrote:
> Test
I called DocumentThreadableLoader::cancel() or delete DocumentThreadableLoader
in ThreadableLoaderClient::didSendData/didDownloadData(), but in such cases
Resource is not destructed synchronously
  because Resource::canDelete() returns false
  because Resource::hasRightHandleCountApartFromCache(0) returns false
  because ResourceFetcher::m_documentResources has ResourcePtr to Resource,
and the Resource is destructed in
ResourceFetcher::garbageCollectDocumentResourcesTimerFired().

[Nate Chapin, 2015-09-01 21:24:35]

Ok, thanks for trying. Still LGTM

[tyoshino (SeeGerritForStatus), 2015-09-02 05:07:49]

On 2015/09/01 19:26:31, hiroshige (slow) wrote:
> On 2015/09/01 17:31:37, Nate Chapin wrote:
> > Test
> I called DocumentThreadableLoader::cancel() or delete DocumentThreadableLoader
> in ThreadableLoaderClient::didSendData/didDownloadData(), but in such cases
> Resource is not destructed synchronously
>   because Resource::canDelete() returns false
>   because Resource::hasRightHandleCountApartFromCache(0) returns false
>   because ResourceFetcher::m_documentResources has ResourcePtr to Resource,
> and the Resource is destructed in
> ResourceFetcher::garbageCollectDocumentResourcesTimerFired().

Oh, thanks for checking. I see, m_handleCount is incremented by the
ResourceFetcher instance to keep the instance alive.

So, this is safe where the fetcher outlives the RawResource, right?

[tyoshino (SeeGerritForStatus), 2015-09-02 05:30:36]

On 2015/09/02 05:07:49, tyoshino wrote:
> On 2015/09/01 19:26:31, hiroshige (slow) wrote:
> > On 2015/09/01 17:31:37, Nate Chapin wrote:
> > > Test
> > I called DocumentThreadableLoader::cancel() or delete
DocumentThreadableLoader
> > in ThreadableLoaderClient::didSendData/didDownloadData(), but in such cases
> > Resource is not destructed synchronously
> >   because Resource::canDelete() returns false
> >   because Resource::hasRightHandleCountApartFromCache(0) returns false
> >   because ResourceFetcher::m_documentResources has ResourcePtr to Resource,
> > and the Resource is destructed in
> > ResourceFetcher::garbageCollectDocumentResourcesTimerFired().
> 
> Oh, thanks for checking. I see, m_handleCount is incremented by the
> ResourceFetcher instance to keep the instance alive.
> 
> So, this is safe where the fetcher outlives the RawResource, right?

Discussed offline. Seems correct. Anyway, it's good to have the protect. Sending
to CQ. Thanks for review.

[hiroshige, 2015-09-02 05:36:55]

lgtm.

It's good to have protects, because
- there might be another path that results in synchronous Resource deletion, and
- even if the current source is safe, the reason why it is safe is undocumented
(and thus we shouldn't depend on it / it's hard to infer/ensure the safeness).

[tyoshino (SeeGerritForStatus), 2015-09-02 05:40:37]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2015-09-02 05:40:49]

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

[commit-bot: I haz the power, 2015-09-02 07:32:35]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201626