174167 - Need to define error handling for nsIWebBrowserPersist

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Boris Zbarsky [:bzbarsky]]

What happens if the persistence object experiences a network error while, eg,
doing saveURI?  Will it notify the user or will it send a useful onStateChange
notification to its WebProgressListener?

What happens if the persistence object gets an error page while attempting to
fetch the data (eg HTTP 404 or something along those lines).  Should it save the
error page? Should it notify the user?  Should it send a useful onStateChange
notification?

(My personal preference is that the progress listener be notified in both cases;
the notification for 404 pages and the like probably needs to be conditional on
an option/flag set by the caller).

Comment 1

[Boris Zbarsky [:bzbarsky]]

This blocks fixing a number of bugs in our "save link as"/"save as" code because
right now we do our own (very broken) checking for data availability.

Comment 2

[Samir Gehani]

adt: nsbeta1-

Comment 3

[Adam Lock]

As far as the web browse persist object is concerned, it is possible to
determine the response code from the registered nsIWebProgressListener. During
onStateChange, QI the nsIRequest for the nsIHttpChannel and you can determine
whether the response code is acceptable. It should be possible to determine
whether to cancel or continue at this stage.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Perhaps we could document nsIWebProgressListener a bit to make it clear
when one would check the channel status?  As a start, defining the various state
flags would be nice... (which one corresponds to "OnStartRequest fired",
exactly?)  The goal here is to make sure to cancel before any files are created
or overwritten in cases when we cancel....

Comment 5

[Jonathan Wilson]

ok, is there something I can do here to help fix this?
This bug (or more specificly bug 208584 which depends on this one via bug
160454) is almost a showstopper for me. Not being able to save a whole bunch of
important files is a vert large problem...

Comment 6

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.

Comment 7

[Boris Zbarsky [:bzbarsky]]

This can't possibly block 1.7, imo.  The changes that need to happen here need
to happen in an alpha milestone, not after RC2.

Comment 8

[Ben Basson]

My apologies for bugspam... I intended to nominate for 1.8a and apparently
missed. This bug is holding back 160454 which has a whole stack of bugs
dependent on it in turn. It'd be nice to see this bug fixed sometime in the near
future, if for no other reason than to pave the way for 160454 and the 23 (at
time of writing) bugs that it blocks. What do you think, Boris?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think if someone posted a patch for this I'd be very happy, and that I won't
have time to work on this in the foreseeable future myself...

Comment 10

[Boris Zbarsky [:bzbarsky]]

We've gone ahead and checked in a fix for bug 160454 without fixing this, but
that regresses saving of links pointing to error pages.  Ideally, trying to do
that should fail and let the user know instead of silently saving bogus data.

I think a call-time persist flag to control the behavior when hitting an error
page together with a notification back to caller when an error is hit so the
caller can tell the user are the way to go.

Darin, biesi, any objections?  Phil, any interest in working on this?

Comment 11

[Phil]

(In reply to comment #10)
> Darin, biesi, any objections?  Phil, any interest in working on this?

Possibly - but I'd need to find an hour or 3 to produce some scenarios to
examine this problem... maybe this week, but pretty busy.

Comment 12

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

we already seem to have:
 85   /** Fail on broken inline links */
 86   const unsigned long PERSIST_FLAGS_FAIL_ON_BROKEN_LINKS = 4096;

isn't this what we want?

Comment 13

[Boris Zbarsky [:bzbarsky]]

All that currently does is call EndDownload(NS_ERROR_FAILURE) if the AsyncOpen()
fails on the channel.  It doesn't handle 404 detection.  It doesn't notify the
caller in a useful way.

Furthermore, that flag is forced on for saveURI and saveChannel.  So reusing it
would make it impossible to save an error page that one is viewing.

Comment 14

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

>Furthermore, that flag is forced on for saveURI and saveChannel.  So reusing it
>would make it impossible to save an error page that one is viewing.

that would not exactly be a regression. we have a bug about that, as I recall.

Comment 15

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

(In reply to comment #14)
> that would not exactly be a regression. we have a bug about that, as I recall.

ok.. with phil's patch, it would be.


bz and I discussed this, we decided on this:
- add a flag like FAIL_FOR_ERROR_RESPONSES
  if set: OnStartRequest tries to QI the request to nsIHttpChannel and checks
for isRequestSucceeded. if it isn't, it cancels the channel with some error code.
- that will lead to onstoprequest getting called, with that error code. it
passes this code on to the listener, which can show an error dialog as needed.

Comment 16

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

although it might be a good idea to make onstopr and
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1629
use the same function to report an error...

Comment 17

[Boris Zbarsky [:bzbarsky]]

That's a UI matter, really.  What we really need is to give embeddors an easy
way to hook the same object into both places involved, then do it ourselves.

Comment 18

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

the webprogresslistener in question is, in the usual case, the nsIDownload
implementation of the embeddor.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  You just want to share the code that decides what to tell the listener?

The right thing is to make exthandler use the saveChannel method of the
persistence object, imo.  Then we can consolidate some code...

Comment 20

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

(In reply to comment #19)

what I want to do is to make either both codepieces call onStateChange, or make
both call onStatusChange, not have one call onStatusChange, and the other call
onStateChange.

calling saveChannel is not really possible, because the channel is opened alraedy...

Comment 21

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

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

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

Bug 383716 probably helps this bug a bit

Comment 24

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.