405107 - File->Save Page As... download often reports 0 bytes as saved

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Martijn Wargers (dead)]

To reproduce:
- Visit http://www.google.nl/firefox (although it seems to happen on every page)
- Do "File->Save Page As..."

Expected result:
- Page should be saved, reported amount of bytes saved should at least be > 0 bytes.

Actual result:
0 bytes saved as reported by the download manager.

It seems to happen when I first have visited the page.

I can look for a regression range, if wanted.

Comment 1

[Shawn Wilsher :sdwilsh]

A regression range would be awesome if you could get it.

Edward - any thoughts on this off the top of your head?

Comment 2

[Martijn Wargers (dead)]

Sorry, this isn't a real regression, this started happening as soon as bug 223895 was fixed.

Fyi, when I do a replace of the old saved file, you can't see the bug, e.g. the correct size is being reported.

Comment 3

[Ed Lee :Mardak]

The issue here is that webbrowserpersist actually gives us multiple start/stops for each thing it's downloading from the page. So probably an image is getting saved before the actual html page that we're looking for the file size of.

Comment 4

[Shawn Wilsher :sdwilsh]

So, what our our options here to deal with the issue of the webbrowserpersist giving us multiple start/stops?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

use totalprogress instead of selfprogress?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> use totalprogress instead of selfprogress?
We do use totalprogress for all I can tell.  However, the code that could be causing this is this maybe?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.153#2061

Comment 7

[Ed Lee :Mardak]

Strange... can anybody reproduce this? I tried on several pages on windows and mac and they're all reporting a file size now.....

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm, but that shouldn't cause 0 bytes reported, right? 

Comment 9

[Ed Lee :Mardak]

Attachment: v1

(In reply to comment #8)
> Hm, but that shouldn't cause 0 bytes reported, right? 
Was there something recently that caused webbrowserpersist files to be saved/created in a different order?

This bug came up probably because in the "STOP" code, we try getting the file size from disk, but that might be 0 because the html file was just created -- another file finished (perhaps an image) and triggered the STOP code.

The reason why I ask if the download order changed is because some reason the file /doesn't/ exist, so it's falling back on the max transfer size (the else case):

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.156&mark=2105-2107#2087

This fix doesn't completely solve the issue of multiple-file downloads from WBP. It should work for saving pages because those files are transferring from cache.

I still can't reproduce the bug anymore, but this patch will help make sure it doesn't show up as 0.

Comment 10

[Ed Lee :Mardak]

Attachment: v1

Whoops. diff didn't pick up .h

Comment 11

[Ed Lee :Mardak]

This won't work completely if ProgressChange64 never gets called..

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 299470 [details] [diff] [review]
v1

sure, this should help if something strange happens - but I'm with you that I cannot get it to reproduce :/

r=sdwilsh

Comment 13

[Ed Lee :Mardak]

Attachment: v1.1

Now with more network action!

I haven't tested this yet, but it sounds like it should work according to nsWebBrowserPersist.cpp code. The NETWORK bit is only set if it's totally completed.

I can get this to reproduce if I don't browse to any other pages after opening the app. Open app, cmd-s, save, open DM, 0 bytes.... sometimes.

Checking if 2 flags (out of more than 2 flags) are set..
(flags & FLAG1) && (flags & FLAG2)
or..
flags & (FLAG1 | FLAG2) == (FLAG1 | FLAG2)
or..
!(~flags & (FLAG1 | FLAG2))

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

BTW, https://build.mozilla.org/tryserver-builds/2008-01-26_17:[e-mail]-dlmgr.0size/ fixed this for me...

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 299487 [details] [diff] [review]
v1.1

>-  } else if ((aStateFlags & STATE_STOP) && IsFinishable()) {
>+  } else if ((aStateFlags & STATE_STOP) && (aStateFlags & STATE_IS_NETWORK) &&
>+             IsFinishable()) {
eh, a comment about this please

r=sdwilsh

Comment 17

[Ed Lee :Mardak]

If we can trigger a "save page as" dialog and save it with a mochitest... ooh.

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.157; previous revision: 1.156
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.62; previous revision: 1.61
done

Comment 18

[Ed Lee :Mardak]

Attachment: v1.2

As checked in with more comments on the NETWORK stuff.

Thanks for trying out the trybuilds.

Comment 19

[İsmail Dönmez]

Looks like this regressed download manager, it always shows as downloading even after download is finished, this is on Linux, latest CVS.

Comment 20

[Ed Lee :Mardak]

The first part of this fix got webbrowserpersist downloads working. The second part of this fix was landed in bug 414306 for exthandler downloads.

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

-and-

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=3952

in-litmus+