872330 - senseless code in nsDownload::Cancel

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Christian Ascheberg]

Attachment: remove code

There is some code in nsDownload::Cancel which seems senseless to me:

In nsDownload::Cancel (see patch):

if (IsPaused() && !IsResumable())
 (void)Resume();

Resume() will return NS_ERROR_UNEXPECTED because of !IsResumable().

The comment mentions fake-paused downloads, but I do not see how this code is doing anything. Or am I wrong?
Removing the code might not even be the right thing to do, I am not quite sure.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 749593 [details] [diff] [review]
remove code

Review of attachment 749593 [details] [diff] [review]:
-----------------------------------------------------------------

That change has been done in http://hg.mozilla.org/mozilla-central/rev/d9a090a88b0d

the ::Resume() code was different at that time, it was actually checking IsResumable, and if false was trying to Resume the request. That code has been changed in http://hg.mozilla.org/mozilla-central/rev/7b2615f943d8 that actually made so that non-resumable downloads cannot be paused.
That basically means we should never reach a situation where (IsPaused() && !isResumable()) is true.
You may try to follow the code and verify I properly interpreted the changes too.

I think removing the code may even be fine, though at this point we may replace it with:
MOZ_ASSERT_IF(IsPaused(), isResumable());
While doesn't have a big value by itself, it should help us by making sure this was a dead condition.

Regardless, good catch!

Comment 2

[Christian Ascheberg]

(In reply to Marco Bonardo [:mak] from comment #1)
> That basically means we should never reach a situation where (IsPaused() &&
> !isResumable()) is true.

Thank you for stating the revisions. I think you're right.

> While doesn't have a big value by itself, it should help us by making sure
> this was a dead condition.

It just seems to me to be a very random location to make this assertion. It gives the impression that the following code depends on that condition, while it does not. (After all the existing call to Resume(), given that it would ever happen, still does not do anything.) Nevertheless, if you still think it's best I will modify the patch as suggested.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Christian Ascheberg from comment #2)
> It just seems to me to be a very random location to make this assertion. It
> gives the impression that the following code depends on that condition,
> while it does not. (After all the existing call to Resume(), given that it
> would ever happen, still does not do anything.) Nevertheless, if you still
> think it's best I will modify the patch as suggested.

Yes you are right, that's why I said it doesn't have a big value by itself. My point was to ensure this is removing dead code and there isn't some case we didn't consider that may cause that bogus situation.
I took a second look to verify the code paths, SetState is already well protected, it's set to PAUSED only from Pause() that is already throwing if !IsResumable(). The only other point where mDownloadState is set to some other value is GetDownloadFromDB. Here a download may be set to PAUSED and the server may have changed in the meanwhile (very rare case), so that it's no more resumable, but that's unlikely a problem, we will try to resume and something may fail, not a critical problem to care about.
So, I think I'll just take your first patch, thanks for insisting for a second check :)

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 749593 [details] [diff] [review]
remove code

Review of attachment 749593 [details] [diff] [review]:
-----------------------------------------------------------------

That change has been done in http://hg.mozilla.org/mozilla-central/rev/d9a090a88b0d

the ::Resume() code was different at that time, it was actually checking IsResumable, and if false was trying to Resume the request. That code has been changed in http://hg.mozilla.org/mozilla-central/rev/7b2615f943d8 that actually made so that non-resumable downloads cannot be paused.
That basically means we should never reach a situation where (IsPaused() && !isResumable()) is true.
You may try to follow the code and verify I properly interpreted the changes too.

I think removing the code may even be fine, though at this point we may replace it with:
MOZ_ASSERT_IF(IsPaused(), isResumable());
While doesn't have a big value by itself, it should help us by making sure this was a dead condition.

Regardless, good catch!

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

oops, Splinter posted again the old review, please ignore it, the patch is fine as-is.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

just please fix the commit message before setting checkin-needed.

Comment 7

[Christian Ascheberg]

Attachment: remove code

fixed the commit message
r=mak77 as per comment #5 and #6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f173389ff4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4f173389ff4