senseless code in nsDownload::Cancel

RESOLVED FIXED in mozilla24

Status

()

Toolkit
Downloads API
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Christian Ascheberg, Assigned: Christian Ascheberg)

Tracking

Trunk
mozilla24
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

786 bytes, patch
Christian Ascheberg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 749593 [details] [diff] [review]
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.
Attachment #749593 - Flags: review?(mak77)
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!
Attachment #749593 - Flags: review?(mak77)
(Assignee)

Comment 2

5 years ago
(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.
Assignee: nobody → c.ascheberg
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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 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!
Attachment #749593 - Flags: review+
oops, Splinter posted again the old review, please ignore it, the patch is fine as-is.
just please fix the commit message before setting checkin-needed.
(Assignee)

Comment 7

5 years ago
Created attachment 755517 [details] [diff] [review]
remove code

fixed the commit message
r=mak77 as per comment #5 and #6
Attachment #749593 - Attachment is obsolete: true
Attachment #755517 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4f173389ff4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.