116279 - persist not working without nsIAuthPrompt - Does not get to saving file

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[plambert17]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.6+) Gecko/20011211
BuildID:    2001121103

By holding down the shift key and selecting an entry on this web page
a download of a zip file should begin.  The file on the client is created by the
contents never downloads.  The file created is about 5K when it should be at
least 50K.

This web site works for NS 4.7 but not Moz or NS6.2

Reproducible: Always
Steps to Reproduce:
1.go to web page http://www.brisnet.com/cgi-bin/trk_report.cgi?drs
2.while holding down the shift key click any of the links
3.Save file dialog will appear, select target folder and then save.
4.downloading dilaog will appear for a short time.

Actual Results:  The file is created on the target folder but the file contents
never downloads properly.  Also, the user and passord dialog never appears.

Expected Results:  After going to the page, I select a link to download a file.  
First, a "Save File" dialog is presented and once "save" is selected a  file
saving dialog appears.  Just as the download begins, another security dialog
appears requesting the user ID and password.  (This download requires an
account.  But you will be able to see the bug without one.)  After the user
enters their account info the download continues properly until complete.


NS6.2 and Moz never even get as far as presenting the user account info.  Try on
NS4.7 first to know what to expect.  Cancel out when it asks for user account.

Comment 1

[Boris Zbarsky [:bzbarsky]]

what are the contents of the file that is saved by Mozilla?  Could you attach it 
to this bug using 
http://bugzilla.mozilla.org/attachment.cgi?bugid=116279&action=enter ?

Comment 2

[plambert17]

Attachment: zipped text file

This is a representative file that I successfully download using NS4.7
but not NS 6.2 or Moz

Comment 3

[Boris Zbarsky [:bzbarsky]]

No, you misunderstand.  I'm sorry I was not clear enough.  You say that Mozilla 
creates a file but that that file does not contain the right contents (it's too 
small).  What _does_ this too small file contain?

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  Reporter sent me the file that gets sent.  We're saving the HTML that comes 
with the 401 response instead of asking the user for a password and the like.

Over to Adam, ccing others who may be responsible.

Comment 5

[Darin Fisher]

this may be a dupe of bug 54349, or at least vinay's patch might solve this bug
as well.

Comment 6

[Vinay Badami]

Same cause different code path.

This is pretty close, but not exactly the same. We solved the bug# 54349 by
setting up loadgroups and looking for the callbacks on the loadgroups if a
callback is not specified on the http channel itself.

In this case, we get called thru
nsHttpChannel::SetNotificationCallbacks(nsHttpChannel * const 0x044d2cd0,
nsIInterfaceRequestor * 0x038bb018) line 1884
NS_OpenURI(nsIChannel * * 0x0012a0e0, nsIURI * 0x044e1848, nsIIOService *
0x00c2ef70, nsILoadGroup * 0x00000000, nsIInterfaceRequestor * 0x038bb018,
unsigned int 0) line 156 + 16 bytes
nsWebBrowserPersist::SaveURIInternal(nsIURI * 0x044e1848, nsIInputStream *
0x00000000, nsILocalFile * 0x044e08e0, int 0) line 472 + 45 bytes
nsWebBrowserPersist::SaveURI(nsWebBrowserPersist * const 0x038bb01c, nsIURI *
0x044e1848, nsIInputStream * 0x00000000, nsILocalFile * 0x044e08e0) line 228

We do not have a load group (ie it is null) and the callbacks set does not have
an nsIAuthPrompt interface.
Hence we fail to prompt resulting in unauthorized message getting stored.

Comment 7

[Darin Fisher]

vinay, adam: the code calling necko needs to be fixed to supply a nsIAuthPrompt
implementation, either attached to the load group or the channel itself.

Comment 8

[Adam Lock]

Do I really need to pass in (and implement) a load group?

From the code it looks like it would be sufficient to extend
nsWebBrowserPersist::GetInterface to expose one, either one implemented
internally or one set by the client via some property on the
nsIWebBrowserPersist interface.

Comment 9

[Darin Fisher]

you just need to ensure that the channel either has notification callbacks,
which include a nsIAuthPrompt implementation, or a load group that has
notification callbacks, which include a nsIAuthPrompt implementation.  without
either of these necko cannot satisfy requests which require user authentication.

Comment 10

[Adam Lock]

The checkin for bug 110135 now means that the GetInterface impl on
webbrowserpersist will call GetInterface on the client's registered progress if
it implements nsIInterfaceRequestor. This should allow the client to supply an
nsIAuthPrompt when required.

Comment 11

[Adam Lock]

Some background info on current behaviour. Shift-clicking works like this:

1. Link click is handled in contentAreaClick.js. The Shift key is detected
causing saveURL to be called in contentAreaUtils.js.
2. saveURL calls saveInternal which instantiates an object called
nsHeaderSniffer that saves the URI to a temporary file. The purpose of this
temporary save is to get the content type of the data so a reasonable save
dialog can be displayed.
3. nsHeaderSniffer implements nsIWebProgressListener. When the first
onStateChange with a STATE_START flag arrives, the routine gets the content type
from the URI request and aborts the transfer (so no data is actually downloaded,
just the header info).3. After this the nsHeaderSniffer calls a callback routine
supplied by saveInternal (foundHeaderInfo) which displays the appropriate file
selector for the content type and initiates the REAL download via nsProgressDlg
So the issue for password protected URIs is that nsHeaderSniffer fails because
there is no nsIAuthPrompt. Neither is there one on nsProgressDlg which does the
real download. It doesn't make sense for both to implement supply an
nsIAuthPrompt because that would mean the user is prompted twice for username &
password. Caching the username & password could also be fraught for security
reasons and weird authentication schemes (e.g. username/password expires or
changes such as with securid).

Therefore the current thinking (Ben & myself) is that nsHeaderSniffer should not
implement nsIAuthPrompt by design and in the event of an authorization failure
it should assume the data to be an octet-stream and display a generic file save
dialog. After the user hits okay, the real download is started and nsProgressDlg
does provide an nsIAuthPrompt so that the download can proceed correctly.

Comment 12

[Adam Lock]

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

Comment 13

[Adam Lock]

Attachment: Work in progress

Work in progress patch fixes problem with save-as dialog not appearing at all
(in contentAreaUtils.js) by moving some try-catch blocks. Stuff in
nsProgressDlg.js is still to being worked on but should display auth prompts
when required.

Patch also does not include a few tweaks to nsIWebBrowserPersist::CancelSave
and nsIInterfaceRequestor::GetInterface impls on webbrowserpersist object that
make it work.

Comment 14

[Adam Lock]

Attachment: New patch

This new patch does the following:

* Implements nsIInterfaceRequestor on the URI checker object
* The URI checker passes through GetInterfaces calls for nsIAuthPrompt to the
  registered listener.
* Implements nsIAuthPrompt & nsIInterfaceRequestor in nsHeaderSniffer

This impl, puts up a user/password prompt when confronted with a URL requiring
authorization. I am *hoping* but have yet to verify that the HTTP channel impl
will cache the user/pass so it is not required when saving actually begins. I
have yet to verify this, so this patch is still work in progress.

Comment 15

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 16

[Darin Fisher]

http should remember the user:pass combo.

Comment 17

[Adam Lock]

Attachment: Cleaned up patch

I have verified that user/pass authorization supplied to the authprompt during
URI checking is carried through to actual persistence. This patch is a cleanup
of the last one and ready for review.

r/sr please?

Comment 18

[Darin Fisher]

does the HeaderSniffer need to QI to nsIAuthPrompt?  i noticed that it
doesn't... is that ok?

Comment 19

[Adam Lock]

Attachment: Updated patch

nsURIChecker was updated last night to include its own nsIInterfaceRequestor
impl. This new updated patch is smaller because of this.

Comment 20

[Adam Lock]

Attachment: Updated patch 2

Patch makes nsIAuthPrompt available through QI and GI but is otherwise the
same.

Comment 21

[Darin Fisher]

what about the "dump" statements?  shouldn't those be removed / commented out?

Comment 22

[Kathleen :Brade]

Comment on attachment 72624 [details] [diff] [review]
Updated patch 2

r=brade

Comment 23

[Adam Lock]

I left the dump statements in the error handling and prompt code because it
won't impact performance and might yield useful information. If you want to me
to take them out I can.

Comment 24

[Darin Fisher]

what happens to dump statements in an optimized build?  if they are displayed on
the console then that is probably bad.  i seem to recall that we aren't supposed
to print out anything on the console in an optimized build.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Dump statements are not shown by default.  There is a debug pref to turn them on
in optimized builds.

Comment 26

[Darin Fisher]

Comment on attachment 72624 [details] [diff] [review]
Updated patch 2

bz: thx for clearing that up for me...

sr=darin

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 72624 [details] [diff] [review]
Updated patch 2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 28

[Adam Lock]

Fix checked into trunk

Comment 29

[Adam Lock]

From [e-mail]:

"Downloaded nightly build 2002030604 and tested the shift-click file download
feature to see if the login box appears.  Yes it did!  I proceeded to download a
couple of files and they were received properly. They are zip files and they
unzipped just fine. 
When the download box appearred the title read "-1% of zipfile."  This is not
the title but rather the remaining amount of file to download. You might want to
check what text you are setting as the download title. 

On the second download I was not prompted for my user account and ID. This is
probably being saved on the server end.  But I do remember there was some
discussion about the browser remembering the user ID and password.  Bottom line
is, that it worked as expected."

Does anyone know about this -1% issue?

Comment 30

[Boris Zbarsky [:bzbarsky]]

That would be bug 128801 and is unrelated to this bug.

Comment 31

[plambert17]

Attachment: screen shot of % download error

I am attaching the screen shot of the download dialog that shows the % download

error in the title.  I first tried this with IE5.5 and it got the download %
correct but scrambled the title of the file.  The download dialog title should
read
either the Kbs or percentage currently loaded.	For example, downloading the
file "myfile" would give the following.  "0.5 Kb of Myfile downloaded" the Kb
will increment as the file loads.  On Moz you get the following:  "2% Kb of
Myfile downloaded.  The problem may be that Moz is attempting to convert the
number to a % when the acutal Kbs is being presented.  You can test with by
attempting to dowonload any link while holding the shift key down, it does not
need to be the ones I chose.

Comment 32

[David Epstein]

using Mozilla nightly build 2002031803, shift-click on links of this web site
bring up save dialog, saves contents fine, but does NOT display password dialog.
Also using the  3/15 Mozilla debug build, I'm constantly getting this assertion
when loading the url: "invalid value for p3p policy ...". It's occurring in
nsCookies.cpp line 889. Problem resetting cookies?

Comment 33

[Adam Lock]

WFM on latest trunk Win32 pull.

David can you retest on a different build?

Comment 34

[Boris Zbarsky [:bzbarsky]]

David, you'd only get the dialog the first time the username/password is asked
for by the site.  After that, basic auth passwords are remembered until browser
restart.

Comment 35

[David Epstein]

Tried out Mozilla nightly build 2002031903 and 0.9.9 3/19 debug. Individual
product links (bullet point items like 'Aqueduct') display psswd dlog w/ or w/o
shift key. Executable download, toolbar and Price List links directly display
save dlog. 

Comment 36

[Adam Lock]

I believe this is the correct behaviour. Only the bulleted list items require
authentication, i.e. those pointing to https://www.BRISnet.com/secure-bin/...

shift+click on those and you see the authentication dialog, followed by the save
as dialog.

The other links are not authenticated and will immediately show the save as dialog.

Comment 37

[David Epstein]

I'm able to get the authorization prompt even without using the shift key on
these bulleted links. Here's the difference in behavior:
Shift + click: get Prompt dialog. If Cancel, get Save Link indicating "link
could not be saved"
Click: get Prompt dialog. If Cancel, goes to Unauthorized page indicating
"proper authorization is required".

Comment 38

[Boris Zbarsky [:bzbarsky]]

What behavior did you expect?  The behavior you describe in comment 37 seems
pretty reasonable to me...

Comment 39

[Adam Lock]

Marking fixed again.

I believe the behaviour is correct. Click or shift click and you'll get the auth
prompt. The shift key just bypasses the content handler / app launcher and goes
straight into the save dialog.

Comment 40

[David Epstein]

thanks for clarifying this. verifying.