241199 - Camino fails to save documents and images

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Greg K.]

Camino/2004-04-20-08 fails to save web pages or images. It works fine using
Mozilla-Mac/2004-04-20-08.

1. Access URL
2. File/Save As...

Camino saves nothing.

Additionally, files such as .sit or .gz files *are* downloaded invisibly in the
background, but are not appearing in the Downloads window.

Comment 1

[Mike Pinkerton (not reading bugmail)]

cc'ing darin, he changed the d/l code most recently.

Comment 2

[Mike Pinkerton (not reading bugmail)]

greg, on what date did this start happening in camino?

Comment 3

[Stuart Morgan]

Presumably this is fallout from bug 240835.  Hopefully, it will have fixed
itself in the 4-21 build, since it looks like the 4-20 Camino build was before
the final checkin, but the 4-20 build was after.

Comment 4

[Greg K.]

The problem is new in the 20040420 build. It is not evident in the 20040416
build. There were no Camino builds between those two.

Comment 5

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

bug 240835's patches did not affect camino...

http://lxr.mozilla.org/mozilla/source/camino/src/embedding/CHDownloadFactories.mm#86
looks like it should also check for nsITransfer now, not only nsIDownload... and
it should also support nsISupports...
but I don't think that'd fix this...

Comment 6

[Mike Pinkerton (not reading bugmail)]

the patch wasn't in the bug, darin had to land it after camino caught fire from
his checkins.

Comment 7

[Greg K.]

Still evident using 2004042108.

Comment 8

[Greg K.]

Disregard comment 7. What I thought was the 0421 build is still just 0420.

Comment 9

[Stuart Morgan]

*** Bug 241232 has been marked as a duplicate of this bug. ***

Comment 10

[Stuart Morgan]

This was definitely already broken by 2004-04-17 (12:00).

I looked again more closely, and it seems I was mistaken about the checkin
times. I don't see anything in bonsai after the 4/20 NB, so this probably
*won't* fix itself in the next build.

Comment 11

[ashish]

(In reply to comment #4)
> The problem is new in the 20040420 build. It is not evident in the 20040416
> build. There were no Camino builds between those two.



yes, i am having this trouble with 2004042108 nightly. this was not there in
16th April nightly.

Comment 12

[Warren TenBrook]

2004042308 still broken.  I guess the bustage fixes checked in over the last day
didn't target this bug.

Comment 13

[Darin Fisher]

I'm unable to investigate this problem right now since I don't have immediate
access to a Mac.  I will be at the Mozilla Foundation sometime this week, and
I'll try to use one of the Macs there to debug this, but in the meantime if
someone can help investigate this, I would greatly appreciate it.  Thanks in
advance!

Comment 14

[Stuart Morgan]

I'm playing around with the debuger, but I'm not familiar enough with this code
to know what I'm looking for. Here's what I found while watching an attempt to
save a web page:

The code fails in the init method of nsDownloadListener
http://lxr.mozilla.org/mozilla/source/camino/src/download/nsDownloadListener.mm#68
on the line:

  NS_ENSURE_TRUE(targetFile, NS_ERROR_INVALID_ARG);

because this if statement above it fails, and thus targetFile is never set:

  nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(mDestination);
  if (fileURL)

The line that sets fileURL is new from the breakage fixing:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/camino/src/download&command=DIFF_FRAMESET&file=nsDownloadListener.mm&rev1=1.11&rev2=1.12&root=/cvsroot

I'm not 100% clear on what do_QueryInterface does, exactly, but I assume that it
expects mDestination to be initialized, and I'm pretty sure it isn't yet--the
only initialization for it that I see is later in this same function, where it's
set to the method parameter aTarget. Should this be using aTarget instead?


I hope that's somewhat helpful. I'll keep trying to figure it out, but if
someone wants to point me in the right direction that would be great.

Comment 15

[Stuart Morgan]

Sorry, I should have simply tested before posting. The answer is yes: using
aTarget in place of mDestination makes saving pages and images work correctly.

I'm still seeing problems with the download window not opening itself, and the
transfer status being listed as "interrupted", but that may be bug 240367. I'm
updating my build to incorporate that patch, then I'll test various saving and
downloading and post any remaining issues.

Comment 16

[Steve Kong]

This bug is probably related to this one:

http://bugzilla.mozilla.org/show_bug.cgi?id=240748

It does not crash Camino, but maybe it is related?  I remember around the
20040417 build it was crashing when I tried to save files also.

Comment 17

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

(In reply to comment #14)
> because this if statement above it fails, and thus targetFile is never set:
> 
>   nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(mDestination);
>   if (fileURL)

Heh. The reason is, of course, that mDestination is null at this point - it's
only set to mTarget later in this function.

> I'm not 100% clear on what do_QueryInterface does, exactly, but I assume that it
> expects mDestination to be initialized, and I'm pretty sure it isn't yet--the
> only initialization for it that I see is later in this same function, where it's
> set to the method parameter aTarget. Should this be using aTarget instead?

That does seem like the correct fix. (That, or move the mDestination setting to
earlier in this function)

> I hope that's somewhat helpful. I'll keep trying to figure it out, but if
> someone wants to point me in the right direction that would be great.

That does explain this breakage. Thanks for investigating this!

(I don't think bug 240748 is related...)

Comment 18

[Stuart Morgan]

Attachment: Use aTarget instead of mDestination

I think this is preferable to moving |mDestination = aTarget|, since that would
split up the member initializations.

The patch for bug 240367 does indeed fix the status/progress issues, so I think
this is all we need.

Comment 19

[Stuart Morgan]

Comment on attachment 146910 [details] [diff] [review]
Use aTarget instead of mDestination

Requesting sr; too simple to need an r

Comment 20

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

you can have an r=biesi anyway :)

Comment 21

[Mike Pinkerton (not reading bugmail)]

landed on trunk only (branch doesn't have this problem).

thanks, everyone, for following up and nailing this!

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 146910 [details] [diff] [review]
Use aTarget instead of mDestination

sr=pink

Comment 23

[Asa Dotzler [:asa]]

mike, with bug 24867 landing on the branch, this will need to be landed there as
well (according to bug 24867 comment 211).  If so, then a=asa if needed.

Comment 24

[Mike Pinkerton (not reading bugmail)]

aieeeeeeee. this is going to suck.

reopening for branch landing, keep a close watch on bug 24867

Comment 25

[Stuart Morgan]

Darin was good to us, and checked in the correct version of the Camino patch, so
we should be all set here.