Fixed a bug in URLFetcherCore that caused random failures if the object was freed while doing a chu…

Fixed a bug in URLFetcherCore that caused random failures if stopped (because the URLFetcherImpl was destroyed) during a chunked upload.


BUG=124390
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133881

[hans, 2012-04-23 16:23:27]

On 2012/04/23 09:27:48, Primiano Tucci wrote:

just wondering about the description message:

"if the object was freed while doing a chunked upload" do you really mean freed
here? Because if the object was really freed, we would be using deallocated
memory and that sounds like a bad idea?

Isn't the issue that it can be *stopped* while doing an upload (i.e. there is
still something keeping a reference to the object)?

[Primiano Tucci (use gerrit), 2012-04-23 18:39:06]

On 2012/04/23 16:23:27, hans wrote:
> On 2012/04/23 09:27:48, Primiano Tucci wrote:
> 
> just wondering about the description message:
> 
> "if the object was freed while doing a chunked upload" do you really mean
freed
> here? Because if the object was really freed, we would be using deallocated
> memory and that sounds like a bad idea?
> 
> Isn't the issue that it can be *stopped* while doing an upload (i.e. there is
> still something keeping a reference to the object)?

What happens in detail is that URLFetcherImpl is freed, and its dtor issues a
stop on the URLFetcherCore (that is refcounted, so it stays alive). There is no
other "publicly accessible" way to stop the URLFetcherCore in the current
implementation.
So, I agree with you that the issue is experienced in the Core and is caused by
the Stop. However, since the Core is not publicly accessible , the only way to
trigger a Stop and cause the bug is freeing the Impl.
Perhaps we should try to summarize it in a shorter sentence?

[hans, 2012-04-24 07:51:27]

On 2012/04/23 18:39:06, Primiano Tucci wrote:
> On 2012/04/23 16:23:27, hans wrote:
> > On 2012/04/23 09:27:48, Primiano Tucci wrote:
> > 
> > just wondering about the description message:
> > 
> > "if the object was freed while doing a chunked upload" do you really mean
> freed
> > here? Because if the object was really freed, we would be using deallocated
> > memory and that sounds like a bad idea?
> > 
> > Isn't the issue that it can be *stopped* while doing an upload (i.e. there
is
> > still something keeping a reference to the object)?
> 
> What happens in detail is that URLFetcherImpl is freed, and its dtor issues a
> stop on the URLFetcherCore (that is refcounted, so it stays alive). There is
no
> other "publicly accessible" way to stop the URLFetcherCore in the current
> implementation.
> So, I agree with you that the issue is experienced in the Core and is caused
by
> the Stop. However, since the Core is not publicly accessible , the only way to
> trigger a Stop and cause the bug is freeing the Impl.
> Perhaps we should try to summarize it in a shorter sentence?

Ah, thanks for explaining, that makes sense.

Maybe something like "Fix a bug in URLFetcherCore that caused random failures if
the object was already stopped (because the URLFetcherImpl was destroyed) while
doing a chunked upload."

[Satish, 2012-04-24 08:28:06]

lgtm

[eroman, 2012-04-24 18:53:50]

lgtm

[commit-bot: I haz the power, 2012-04-25 07:51:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10175006/1

[commit-bot: I haz the power, 2012-04-25 09:19:20]

Change committed as 133881