134523 - Downloading is broken in MfcEmbed and in Mac builds(Regression)

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Jeff Doozan]

Attempting to download a file using MfcEmbed results in the filepicker dialog,
but the progress bar never gets displayed, and the file is not saved.

Test Case:

Click: ftp://ftp.mozilla.org/pub/ls-lR.gz (note: it happens with HTTP files, too)

Expected results:

 - File picker dialog appears
 - User chooses file name/location
 - Progress dialog is displayed while the file is downloading
 - The file is saved at the location specified by the user

Actual Results:

 - File picker dialog appears
 - User chooses file name/location
 - No Progress dialog
 - The file is not saved

Comment 1

[Adam Lock]

-> Chak

Comment 2

[Chak Nanga]

This feature has always worked fine and now it's broken.

Alec/Blake : Do you think this is related to the recent nsIDownload changes?

Comment 3

[Alec Flett]

ack, this might be me.
The basic problem is that we need an embedding-only dll to do some stub work to
make the progress dialog appear.

that said, we supposed to make sure that the file would continue to be saved as
before, but the progress dialog wouldn't be shown. We're certain the file isn't
being saved anyway?

Comment 4

[Chak Nanga]

Hi Alec : 

Yes, the file's not being saved.

Please take a look at the comments towards the top of
mfcembed/components/HelperAppDlg.cpp to see how MfcEmbed was handling downloads etc.

I'm not sure if that's still the right way in the current state of things?

Thanks

Comment 5

[Alec Flett]

for my own reference while working on this bug:
http://lxr.mozilla.org/seamonkey/source/embedding/tests/mfcembed/components/HelperAppDlg.cpp

Anyhow, the uriloader is supposed to be opening the initial progress dialog..
but what was SUPPOSED to happen for embedding was that the progress dialog was
going to stop opening, but the uriloader would still pump the download. I'll see
if I can figure out how all these things are hooked up. (I only know it starting
in the uriloader, I don't know where the initial uri load gets kicked off)

Comment 6

[Chak Nanga]

Rick : Do you know the answer to Alec's question regarding the uriloader..thanks

Comment 7

[Mike Pinkerton (not reading bugmail)]

we're seeing the same thing on mac as well (bug 134523)

Comment 8

[Mike Pinkerton (not reading bugmail)]

(doh, i meant 137622) ;)

Comment 9

[Chak Nanga]

Per Mike's comments above, this seems more than an MfcEmbed issue. 

Sending it Alec's way to address the core issue here.

Alec : Please reassign if you're not the owner...thanks

Comment 10

[Mike Pinkerton (not reading bugmail)]

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

Comment 11

[Conrad Carlen (not reading bugmail)]

CC'ing Law since he set up the downloader UI as an embed component.

Comment 12

[Alec Flett]

ok, I'm on this. I think this is blake's fault though :) (or my fault for
reviewing :))

Comment 13

[Mike Pinkerton (not reading bugmail)]

embedding clients need this asap, it's a blocker and needs to be on the _branch_
by friday.

Comment 14

[Alec Flett]

Attachment: band-aid fix

Ah ha! I found the problem. mProgressWindowCreated is a flag which determines
if the progress window worked. In the embedded case, we don't have a progress
window available.. so we just need to fake out the external helper app handler.


here's a quick hack that will fix the problem for now. We really should just
rename this boolean variable to something like "mProgressValid" - because it is
a valid state to have no progress window.

I haven't tested this just yet because I wanted to get the patch up ASAP. Feel
free to test this, I'm going to fiddle with it now.

Comment 15

[Alec Flett]

so assuming this works (Still waiting on my mfcembed build) - what branch do I
check this into? 1.0? 0.9.9?

Comment 16

[Alec Flett]

yep, that fixed it. Now I just need some reviews.
adding the usual nomination keywords.

Comment 17

[Blake Ross]

Comment on attachment 80680 [details] [diff] [review]
band-aid fix

sr=blake

Comment 18

[Conrad Carlen (not reading bugmail)]

> In the embedded case, we don't have a progress
window available.. so we just need to fake out the external helper app handler.

Assuming the embedding app wants a progress window, it can just provide or
register a component with the contact ID "@mozilla.org/download;1" which
implements the nsIDownload iface. I have a feeling I'll be needing to make one
soon for PPEmbed.

Comment 19

[Syd Logan]

So, by incorrectly stating that a progress dialog was created, are we setting
ourselves up for code that relies on that progress dialog being there to fail,
in other words, code that may send notifications, but there is no one listening?

Seems like we should simply decouple the processing of the content from the
existence of the progress dialog in order to address this... is the better fix
to change:

1372   if (mProgressWindowCreated && !mCanceled)
1373   {
1374     nsMIMEInfoHandleAction action = nsIMIMEInfo::saveToDisk;

to

1372   if (!mCanceled)
1373   {
1374     nsMIMEInfoHandleAction action = nsIMIMEInfo::saveToDisk;

in
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1372

since we don't seem to care either way according to your patch, and so that the
rest of the code that looks at mProgressWindowCreated, for example:

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1576

1576     if (!mProgressWindowCreated) 
1577       ShowProgressDialog();

will then be accurate? (I'm afraid of someone coming in and adding code at a
later time assuming that mProgressWindowCreated is actually telling the truth,
leading to regressions).

Comment 20

[Conrad Carlen (not reading bugmail)]

Sorry, "it can just provide or register a component with the contact ID
"@mozilla.org/download;1" which implements the nsIDownload iface." was supposed
to be a question.

In other words, if the embedding app wants a progress dialog:
 it can either do that? If so, is there anything else required?
or
 should we add that component to the embed package in case they want to use the
XUL progress dialog?

Comment 21

[Conrad Carlen (not reading bugmail)]

And, in addition to @mozilla.org/download;1, would you also need to implement 
@mozilla.org/download-manager;1? Yikes, that's a lot of work. Can somebody who
designed this summarize what's the minimum an embedding app needs to provide in
order to complete a download *with* a progress dialog? We then need a native
impl as an example of how an embeddor would do this but, in the short term, the
required bits need to be in the embedding package. 

Comment 22

[Mike Pinkerton (not reading bugmail)]

what if the embedded app wants to use the _xul_ progress window that we bundle
up so nicely in embed.jar? why is that failing?

what exactly is the "download component"? are we not packaging something
correctly? I'm still a little confused why embedding doesn't just pick this up
from seamonkey.

Comment 23

[Alec Flett]

conrad: all you need to do is implement @mozilla.org/download;1 which implements
nsIDownload - the goal was to abstract out the concept of a "dialog" from the
external helper app handler, and allow a download object to create whatever kind
of progress object it wants (i.e. in the case of an embeddor, they could make a
progress dialog, in the case of seamonkey, they update the download manager)

syd: by "incorrectly" stating that a progress dialog is created, we are making
the code behave correctly. I think that it is LESS risky to do my patch, than to
remove the "if (mProgressWindowCreated..." because there may be OTHER cases
where ShowProgressDialog has NOT been called, where we really do need the state
of mProgressWindowCreated to be PR_FALSE. 

The real confusion is basically that the helper app handler no longer has the
concept of a progress window, so really this variable is named incorrectly. The
variable should be named "mProgressValid" as I stated in comment. I have every
intention of cleaning this up on the trunk - in fact I'll go so far as to say
that the above patch is for the BRANCH ONLY, unless ADT requests that it bake on
the trunk.

pinkerton: the reason is that the seamonkey version is now tied into the
download manager, and is built into the "appcomps" DLL, which is not part of
embedding. In order for embedded apps to use our XUL progress dialog, they will
have to create a stub object which instantiates the XUL progress dialog rather
than our download manager.

Comment 24

[Mike Pinkerton (not reading bugmail)]

but we have the appcomps dll (which embedded apps do need for global history)
and it still doesn't work. what else could be failing?

Comment 25

[Conrad Carlen (not reading bugmail)]

Comment on attachment 80680 [details] [diff] [review]
band-aid fix

I see why this works but how about a quick search & replace on that variable
name? r=ccarlen

Comment 26

[Blake Ross]

I think alec pretty much summed it up, but just to reiterate: no, embedding
clients don't have to implement nsIDownloadManager, only the tiny nsIDownload. 
I created the embedding version of nsIDownload but there was apparently no place
to stick it that wasn't brought in by seamonkey, so I was told to just not show
progress for downloads in embedded builds for the time being (but the download
still should have occurred, which is this bug).

Comment 27

[Mike Pinkerton (not reading bugmail)]

Attachment: package everything to use stub JS component

this registers the js component to handle downloads in PPEmbed and sets up
packaging for mac (stubbed out on other platforms in case they don't want the
XUL). 

Also fixes a problem with our embed.jar manifest where it assumed we were
running on unix. This caused mac packaging to miss a bunch of property files
and xul.

needs r/sr, in addition to alecf's patch. ASAP, please, we're on a tight
deadline.

Comment 28

[Mike Pinkerton (not reading bugmail)]

Attachment: same as above, but uses en-mac on mach-o builds

Comment 29

[Mike Pinkerton (not reading bugmail)]

Attachment: take 3, hopefully full patch

Comment 30

[Mike Pinkerton (not reading bugmail)]

Attachment: f'ing mozilla keeps trunctating the patch

Comment 31

[Mike Pinkerton (not reading bugmail)]

grrrr. here's the last bit that moz keeps trunctating

Index: mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul
===================================================================
RCS file: /cvsroot/mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul,v
retrieving revision 1.13.20.1
diff -u -2 -r1.13.20.1 nsHelperAppDlg.xul
--- mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul	10 Apr 2002 02:37:51 -0000	1.13.20.1
+++ mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul	24 Apr 2002 23:51:18 -0000
@@ -25,5 +25,5 @@
 -->
 
-<?xml-stylesheet href="chrome://navigator/skin/" type="text/css"?> 
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 
 <?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>

nothing else below that. just r/sr it.

Comment 32

[Brian Ryner (not reading)]

Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

r=bryner

Comment 33

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

Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

sr=[e-mail]

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

a=[e-mail] for branch checkin

Comment 35

[Jaime Rodriguez, Jr.]

Is this needed for MachV, or solely for embedding. If needed for embedding, when
does it need to be on the 1.0 branch (pls provide a date).

Comment 36

[Mike Pinkerton (not reading bugmail)]

they need this today. asap. ETA an hour ago. this does not impact machV at all,
embedding only.

Comment 37

[Alec Flett]

I'm reassigning this to you pink, since you're doing the bulk of the work here.
I'm landing my other band-aid fix on the trunk now and will clean up in another bug.

Comment 38

[Alec Flett]

the new bug is bug 140136

Comment 39

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checking in on the 1.0 branch. Pls land
this today, then add the fixed1.0.0 keyword.

Comment 40

[Mike Pinkerton (not reading bugmail)]

fixed on branch. trunk upcoming

Comment 41

[Mike Pinkerton (not reading bugmail)]

landed on trunk (whew)

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 80680 [details] [diff] [review]
band-aid fix

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

Comment 43

[amutch]

Just checked this with MFCEmbed 5/7/2002 nightly build from the trunk and it is
NOT working. Running the test case for the bug still does not generate the
progress dialog while the file is downloading. It does appear that files are
being downloaded so that is progress!
:)

Does alecf's patch fix this problem or should a new bug be opened on that? This
bug was intended to cover the progress dialog.

Comment 44

[Mike Pinkerton (not reading bugmail)]

i downloaded the 5/8 PPEmbed build on the trunk and this works just fine for me.
i get all the d/l progress dialogs.

Comment 45

[amutch]

I missed Alecf's new bug here:

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

That bug hasn't completed the review process to be checked in. Is that bug 
intended to address the download progress dialog issue?  Mike indicates that it 
appears in PPEmbed but it is not appearing in MFCEmbed. 

Comment 46

[David Epstein]

checked with MfcEmbed on WinNT4.0 with Mozilla 1.0.0 Gecko 20020509 build. I
wasn't able to download the nightly build with it. First, got a 'channel doesn't
have a prompt' assertion, then the open/save dialog. But when I tried saving it,
it either doesn't save (prog dlog doesn't appear) or I got a memory crash.
Reopening.

Here's the crash log:
nsDownloadManager::AssertProgressInfoFor(const char * 0x0353e0f0) line 269 + 48
bytes
nsDownloadManager::AssertProgressInfo() line 255 + 12 bytes
nsDownloadManager::Open(nsDownloadManager * const 0x03b3c840, nsIDOMWindow *
0x00000000) line 660
nsDownloadProxy::Init(nsDownloadProxy * const 0x0353c1b0, nsIURI * 0x035214c0,
nsILocalFile * 0x0353c420, const unsigned short * 0x00000000, const unsigned
short * 0x0353e770, __int64 1021064481500000, nsIWebBrowserPersist * 0x00000000)
line 79 + 34 bytes
nsExternalAppHandler::ShowProgressDialog() line 1475 + 78 bytes
nsExternalAppHandler::SaveToDisk(nsExternalAppHandler * const 0x03522084,
nsIFile * 0x00000000, int 0) line 1597
CHelperAppLauncherDialog::Show(CHelperAppLauncherDialog * const 0x03536950,
nsIHelperAppLauncher * 0x03522084, nsISupports * 0x00d7b460) line 244 + 16 bytes
nsExternalAppHandler::OnStartRequest(nsExternalAppHandler * const 0x03522080,
nsIRequest * 0x03527f30, nsISupports * 0x00000000) line 1169 + 87 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x03527e40,
nsIRequest * 0x03527f30, nsISupports * 0x00000000) line 229 + 34 bytes
nsFTPChannel::OnStartRequest(nsFTPChannel * const 0x03527f40, nsIRequest *
0x03526b10, nsISupports * 0x00000000) line 676 + 48 bytes
DataRequestForwarder::DelayedOnStartRequest(nsIRequest * 0x035264f0, nsISupports
* 0x00000000) line 280
DataRequestForwarder::OnDataAvailable(DataRequestForwarder * const 0x03526b14,
nsIRequest * 0x035264f0, nsISupports * 0x00000000, nsIInputStream * 0x03526700,
unsigned int 0, unsigned int 2720) line 356 + 19 bytes
nsOnDataAvailableEvent::HandleEvent() line 193 + 70 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03523164) line 116
PL_HandleEvent(PLEvent * 0x03523164) line 596 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d3e6a0) line 526 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0455039a, unsigned int 49517, unsigned int 0,
long 13887136) line 1077 + 9 bytes
US

Comment 47

[Alec Flett]

ack! nsDownloadManager isn't supposed to be part of the embedded build. I
thought appcomps.dll wasn't in the default embedded product.

Comment 48

[amutch]

It was our understanding that appcomps wasn't going to be needed. We were
including it for FTP but alecf fixed that bug a while ago.  

Comment 49

[Chris Petersen]

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

Comment 50

[Mike Pinkerton (not reading bugmail)]

well, it works in mac builds now. over to alecf for other issues.

Comment 51

[Jaime Rodriguez, Jr.]

Is this fixed on the trunk, as well as the branch? If so, pls resolve as fixed.
thanks!

Comment 52

[amutch]

Alecf checked in a patch for:

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

but that doesn't seem to have solved the downloading dialog problem. I just
tested that in MFCEmbed June 4, 2002 nightly and you still don't get a
downloading dialog.

I don't mean to make a go on and on about this but I can't imagine too many
embedders using the Mozilla 1.0 source when it doesn't support downloading, at
least on Windows platforms.

Comment 53

[Adam Lock]

I'm seeing this problem in the ActiveX control as well for my work on bug 126245.

Can we have some kind of fallback behaviour so that
nsIHelperAppLauncherDialog::ShowProgressDialog is called on the embedding apps
implementation when nsIDownload is not available? 

It seems weird that embedders have to implement yet another object when there's
a ShowProgressDialog method on an interface they do implement. It doesn't appear
to be called anywhere.

Comment 54

[Alec Flett]

ah, that could work... I didn't know embeddors already implemented that. I'll
see what I can do.

Comment 55

[jr]

works for me with mozilla a1 on windows xp

Comment 56

[amutch]

Please read the summary and the comments. This bug affects embedded builds of
Mozilla, not the main Mozilla distribution. The fact that it WORKSFORME in
Mozilla is irrelevant to this bug.

Comment 57

[Chak Nanga]

Hi Andrew : Could you please attach the patch that K-Meleon folks came up with
to fix this issue for review by Alec/others...thanks

Comment 58

[Alec Flett]

ugh, I was just looking at this.. it looks really messy to me right now, but
maybe I just need to wrap my head around this. It looks to ME like we're using
nsIHelperAppLauncherDialog in:
  OnStartRequest() we call Show() to ask if we can save to disk
  PromptForSaveToFile to prompt where we're saving the file

but we're NOT using it for progress. I'm now wondering if all this nsIDownload
junk belongs in seamonkey's nsIHelperAppLauncherDlg implementation.. if I can
find it :)
I wonder if we used to do it that way, before the nsIDownload stuff landed.

Comment 59

[Alec Flett]

moving bugs to mozilla 1.2alpha that missed 1.1beta

Comment 60

[Alec Flett]

this is no longer a blocker - downloads work now, they just don't show progress.
Lowering to "critical"

Comment 61

[Alec Flett]

From what I understand, downloading is no longer broken, so I'm punting this to
1.3alpha and/or hoping someone else will take care of it... (or fix the subject
line to tell us what is really going on)

Comment 62

[Juan José Mata]

Clearing status whiteboard since it applied only to the part of the bug that is
now fixed.  This remains open only due to the progress dialog now.

Removing the topembed+ so that triage can happen again here.

Comment 63

[Juan José Mata]

Topembed triage: minusing this bug.

We should probably close this bug as fixed and open a new one regarding the
progress meter implementation.  Opinions?  Alec?

Comment 64

[Alec Flett]

yeah, this bug is a mess at this point. I don't even know what we should be
doing here, and I wonder if the fix might fall out of the mime work that might
eventually be done by darin and friends.

Comment 65

[amutch]

Last I checked, this is still only half-fixed per the initial report as the 
progress dialog isn't appearing in Windows builds. However, the focus was the 
actual downloading of the file and that does appear to be working consistently. 
So, a new bug focused just on the progress meter issue is probably in order. 
Hopefully, that will keep it from falling off the radar. Who wants to file the 
bug?  Alecf?
:)

Comment 66

[Conrad Carlen (not reading bugmail)]

FWIW, since this bug was active, PPEmbed (Mac builds) got its own native
download UI impl and doesn't have this problem.

Comment 67

[David Epstein]

verified against MfcEmbed in Mozilla 1.2b Mozilla/5.0 (Windows; U; WinNT4.0;
en-US; rv:1.2b) Gecko/20021026 build. Download works fine, but as noted,
progress dialog isn't working. Will close out this bug and file one addressing
the prog dlog in MfcEmbed.

Comment 68

[David Epstein]

submitted bug 177408 for prog dlog not working in MfcEmbed. Didn't see any bug
submitted for this, but if there is, please dup to it. Thanks.