Closed Bug 118719 Opened 21 years ago Closed 21 years ago

Save as waiting for reply from server before showing dialog

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.0.1

People

(Reporter: johann.petrak, Assigned: bugs)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [adt3 RTM])

Attachments

(1 file, 3 obsolete files)

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.
Reporter: Please always specify which "Build ID" you're using,
as found in the title bar of the Mozilla window.
build 2002010721/linux - however i believe this problem already exists for some
time.
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.
Assignee: law → ben
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
methinks i've been seeing this with today's bits --definitely on all platforms
[and mozilla.org appears slow as well].
Keywords: perf
OS: Linux → All
QA Contact: sairuh → jrgm
Hardware: PC → All
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.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
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)
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.
Attached patch Proposed patch (obsolete) — Splinter Review
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).
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...
Keywords: patch, review
Target Milestone: Future → ---
Attached patch Patch v 1.1 (obsolete) — Splinter Review
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.
Attachment #67701 - Attachment is obsolete: true
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
Attachment #67737 - Flags: superreview+
-> 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. 
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Comment on attachment 67737 [details] [diff] [review]
Patch v 1.1

Looks good to me.  Thanks for catching that missing addref!
Attachment #67737 - Flags: review+
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.
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.
Indeed, that's no good....

I'll see what I can come up with in the way of error handling.
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.
Attached patch Patch v 1.2. (obsolete) — Splinter Review
This is untested, unfortunately, since my build is refusing to start... will
try to test it tonight.
Attachment #67737 - Attachment is obsolete: true
Attached patch Patch v 1.3Splinter Review
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.
Attachment #68015 - Attachment is obsolete: true
Thanks for finding the addref problem!
Can we get this reviewed and checked in soon? Composer needs it for link 
checker and publishing work.
Blocks: 88208, 108296
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 on attachment 68071 [details] [diff] [review]
Patch v 1.3

r=cmanske on the composer part. Tested and works great!
Attachment #68071 - Flags: review+
Boris: Sounds good to me!
*** Bug 124105 has been marked as a duplicate of this bug. ***
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 :-).
Well... that should lead to more consistent error-handling, no?  :)
Attachment 68071 [details] [diff] checked in.  People should retest with builds that include this
fix and see how it is...
Oh, it would be consistent anyhow, since I would just cut/paste the same code
wherever else it was needed :-).
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...
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..
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.
At the moment the worst case is still whatever the default network timeout is
(whicis far too long for this).  Hence my suggestion.
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
so, bz. What is left to be done with this bug?
I say we resolve this and file a separate bug for the timeout thing if/when we
decide to do it.... 

Acting accordingly.  :)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
check to see if you are most likely seeing bug 128927.  
nsbeta1+ per Nav triage team, adt3
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt3]
->1.0.1/RTM per Nav triage team.  Let's reconsider this after we get beta feedback.
Whiteboard: [adt3] → [adt3 RTM]
Target Milestone: mozilla1.0 → mozilla1.0.1
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.
resolving fixed again, since the new bug is filed on further work on this stuff.
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
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? 
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.
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. 
Keywords: fixed1.0.0
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.