118719 - Save as waiting for reply from server before showing dialog

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P3)

Description

[[e-mail]]

When trying to "save link as..." or "save as.." from an URL that has
a very slow server, nothing seems to happen. Usually a user will think
he missed the context menu option and try again, agin nothing is happening.
Then, minutes again, suddenly the save dialog pops up. It seems the dialog
is only shown after a reply from the slow server is received. This is
very bad, because it is utterly confusing. If the reply is really needed 
to initialize the dialog (I doubt it), at least pop up an interim dialog
with disabled entry fields and stuff greyed out, showing what is happening.

Comment 1

[Karl Johan Kleist]

Reporter: Please always specify which "Build ID" you're using,
as found in the title bar of the Mozilla window.

Comment 2

[[e-mail]]

build 2002010721/linux - however i believe this problem already exists for some
time.

Comment 3

[Bill Law]

This is a consequence of us fetching the http headers to figure out the content
type (and possible content-disposition).  There may be other bugs that are
essentially the same.

We need to see what happens when the link is bogus, also.

Comment 4

[[e-mail]]

this is especially cumbersome in the case an image that is already displayed
should be saved: in that case, the image is already there, but save as still
seems to contact the server and then refech it instead of just storing whatever
is in the cache.

Comment 5

[sairuh (rarely reading bugmail)]

methinks i've been seeing this with today's bits --definitely on all platforms
[and mozilla.org appears slow as well].

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

We could/should find a better way of finding content type for certain types of
object, but I'm not likely to get to that before this release.

Comment 7

[timeless]

why do we need the content type if the user specified save as? (i'm assuming 
this bug is focused on cache misses and not items where we should be copying 
from cache. i'm assuming there's another bug for the fact that we use a network 
link when there's a displayed version of the object on the current page)

Comment 8

[Boris Zbarsky [:bzbarsky]]

We don't need the content-type.  we need the content-disposition...

We should be doing two things:

1)  Using the cache
2)  Using HEAD, not GET.

We currently do neither; the current solution is a kludge.  We should be opening
our own http channel instead of having webbrowserpersist do it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Patch to use nsIURIChecker, which does the right thing with HEAD, checking
response status, and so forth.

Just tested this and this makes us not do a fetch at all for Save Page As and
Save Image As.	For Save Link As we act more sanely (do a HEAD request instead
of GET, so it's much faster).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Akkana, Darin, would you look over my changes to nsIURIChecker?  Akkana, what
should the load flags be for composer?  I left it at LOAD_NORMAL for now, since
that's what the old behavior was....

Unfuturing since we have a working patch; this should be retriaged...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 1.1

This one makes us not crash on GC following Save As.  :)  AsyncCheckURI
returned a ref to the linkChecker but did not addref it....  Also, hold on to
the link checker as a property on the header sniffer, just in case.

Comment 12

[Darin Fisher]

Comment on attachment 67737 [details] [diff] [review]
Patch v 1.1

>Index: xpfe/communicator/resources/content/contentAreaUtils.js

>+  this.linkChecker = Components.classes["@mozilla.org/network/urichecker;1"]
>+    .createInstance().QueryInterface(Components.interfaces.nsIURIChecker);

the explicit call to QueryInterface can be replaced with:

      .createInstance(Components.interfaces.nsIURIChecker);

however, i'm not exactly sure that this is preferred style :-/


patch seems fine to me.. sr=darin

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

-> 1.0 so I can review bz's patch. 

We need both the content-type AND the content-disposition (although I see patch
1.1 maintains that).. content-type is used to prefill the filter list with an
appropriate set of file types. 

Comment 14

[Akkana Peck]

Comment on attachment 67737 [details] [diff] [review]
Patch v 1.1

Looks good to me.  Thanks for catching that missing addref!

Comment 15

[Akkana Peck]

If this fixes the problem, I hope we can get it in sooner than 1.0.  I hit this
all the time ... sometimes the dialog never does come up, sometimes it's several
minutes (long after I've given up and gone to a terminal window to run a manual
ftp), sometimes I try a second time and then both dialogs come up within seconds
of each other.

Comment 16

[Bill Law]

What happens in the case of a bogus url, or some kind of error occurs while
fetching it?  It looks like it will just fail silently.

I know there's currently issues with error handling in general, but it seems
like in this case we should give the user some notification before we go ahead
and prompt for a save-to file name.

Trust me, we've ignored error handling while downloading far too often and for
far too long.  I say we fix that bug while we're making it faster.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Indeed, that's no good....

I'll see what I can come up with in the way of error handling.

Comment 18

[Boris Zbarsky [:bzbarsky]]

So I'm seeing two options:

1)  Bail on sniffer failures.  Put up an alert telling the user the url is "not
    reachable" or some such.  I don't like this approach because I feel that the
    sniffer can fail while the data is in fact saveable (eg see the hack the
    sniffer does for 404 responses; maybe there are other such hacks that are
    needed).
2)  Proceed on sniffer failures.  Get the content type from the document object
    if we have a document object, otherwise try the uri extension, otherwise
    have an empty type calling into foundHeaderInfo.  Then if saving _really_
    fails handle the error in the persistance object.

I personally prefer solution #2, attaching a patch that implements it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 1.2.

This is untested, unfortunately, since my build is refusing to start... will
try to test it tonight.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 1.3

OK, now it's tested.  This tries to get a content type by all the available
means till it runs out of options.  Makes sure to always put up the filepicker.

Comment 21

[Charles Manske]

Thanks for finding the addref problem!
Can we get this reviewed and checked in soon? Composer needs it for link 
checker and publishing work.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I could land the URIChecker/composer changes now (they have sr=darin,
r=akkana,bbaetz) and leave the actual Save As changes until Bill has reviewed
them.  Would that work?

Comment 23

[Charles Manske]

Comment on attachment 68071 [details] [diff] [review]
Patch v 1.3

r=cmanske on the composer part. Tested and works great!

Comment 24

[Charles Manske]

Boris: Sounds good to me!

Comment 25

[Matthias Versen [:Matti]]

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

Comment 26

[Bill Law]

r=law

For the contentAreaUtils.js changes.

One thing, though.  It looks like we passing too few arguments to saveInternal
when called from saveDocument.  I think that's why sometimes documents aren't
being saved from out of the cache.

It also explains the funky "||" in the line at
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#200.

I'm not thrilled that you're passing the buck on actually handling the errors. 
But that's only because I'll have to take care of it over in bug 27609 :-).

Comment 27

[Boris Zbarsky [:bzbarsky]]

Well... that should lead to more consistent error-handling, no?  :)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment 68071 [details] [diff] checked in.  People should retest with builds that include this
fix and see how it is...

Comment 29

[Bill Law]

Oh, it would be consistent anyhow, since I would just cut/paste the same code
wherever else it was needed :-).

Comment 30

[Dimitrios]

Cool. Save As dialog appears shortly after the Save Link As command. Not as fast
as NS4 (delay is still measurable) but this delay is small enough. So there's no
impression that the command doesn't work (as it was before). Build 2002020803,
Win98. Didn't tested on Win2K, somehow I think the delay will be signficantly
larger there...

Comment 31

[Boris Zbarsky [:bzbarsky]]

Note that if we get _really_ desperate we can put in a timer to fire after some
time (1sec?).  If we have not heard from the network by then, just give up and
guess something...

Just a thought if it turns out that it's still taking too long..

Comment 32

[[e-mail]]

Just a thought: what is the worst case? How long will it wait for 
a server that doesnt respond? A timeout of 1 second sounds reasonable,
one does not expect a delay between the save-as request and the dialog.

Comment 33

[Boris Zbarsky [:bzbarsky]]

At the moment the worst case is still whatever the default network timeout is
(whicis far too long for this).  Hence my suggestion.

Comment 34

[[not reading bugmail]]

regarding this perf on w2k, I see it is much faster now with the patch but not a
large noticable delay that is slow to bring up;  Boris, if you think you need to
tweak the perf on that, I'd be all for it.  But, it may be more helpful on a
slow system.. maybe add this to the open window/performance tracking tests.  

I'd say, as of right now the patch makes it much better on w2k, still a small
delay but not bad at all. I'm running P3-733 256meg ram w/16384 for disk &
memory cache in Moz. 

-Dennis

Comment 35

[Peter Trudelle]

so, bz. What is left to be done with this bug?

Comment 36

[Boris Zbarsky [:bzbarsky]]

I say we resolve this and file a separate bug for the timeout thing if/when we
decide to do it.... 

Acting accordingly.  :)

Comment 37

[Akkana Peck]

The comments at the end of this bug suggest that there was another bug filed to
cover the same problem reported here, but don't mention the bug number.  This is
still as big a problem as ever -- I often wait several minutes between when I
choose "Save link as" from the menu and when the dialog actually pops up. 
Reopening.  If the issue is now tracked in another bug, please post the number here.

Comment 38

[[not reading bugmail]]

check to see if you are most likely seeing bug 128927.  

Comment 39

[Boris Zbarsky [:bzbarsky]]

Bug 133732 filed.

Comment 40

[Peter Trudelle]

nsbeta1+ per Nav triage team, adt3

Comment 41

[Peter Trudelle]

->1.0.1/RTM per Nav triage team.  Let's reconsider this after we get beta feedback.

Comment 42

[Peter Trudelle]

Mass moving nsbeta1+/adt3 bugs assigned to Navigator team engineers out to
target milestone 1.0.1.  Please confine your attentions to driving down our list
of TM 1.0 bugs for beta.  Better to help, debug, or test one of them than fix
one of these.

Comment 43

[Boris Zbarsky [:bzbarsky]]

resolving fixed again, since the new bug is filed on further work on this stuff.

Comment 44

[Ben Goodger (use ben at mozilla dot org for email)]

Before patch 1.3 can be landed on the branch I need to know who sr'ed it, and
who r'ed the non contentAreaUtils.js parts. Boris? 

Comment 45

[Boris Zbarsky [:bzbarsky]]

Ben, this landed on Feb 7, before there was a branch.  The state of this code is
identical on trunk and branch at the moment.

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

Oh, in this case I'll go ahead and mark this bug fixed1.0.0 then. If there's
other work that needs to go on the branch/trunk that can be handled in the other
bugs mentioned.