Closed Bug 134523 Opened 18 years ago Closed 17 years ago

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


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



(Not tracked)



(Reporter: jeff, Assigned: alecf)



(Keywords: helpwanted, topembed-)


(2 files, 3 obsolete files)

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: (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
QA Contact: mdunn → depstein
-> Chak
Assignee: adamlock → chak
This feature has always worked fine and now it's broken.

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

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?
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?

for my own reference while working on this bug:

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)
Rick : Do you know the answer to Alec's question regarding the uriloader..thanks
we're seeing the same thing on mac as well (bug 134523)
Ever confirmed: true
Keywords: topembed
OS: Windows NT → All
Hardware: PC → All
(doh, i meant 137622) ;)
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
Assignee: chak → alecf
Summary: Downloading is broken in MfcEmbed (Regression) → Downloading is broken in MfcEmbed and in Mac builds(Regression)
*** Bug 137622 has been marked as a duplicate of this bug. ***
CC'ing Law since he set up the downloader UI as an embed component.
ok, I'm on this. I think this is blake's fault though :) (or my fault for
reviewing :))
embedding clients need this asap, it's a blocker and needs to be on the _branch_
by friday.
Severity: critical → blocker
Attached patch band-aid fixSplinter Review
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.
so assuming this works (Still waiting on my mfcembed build) - what branch do I
check this into? 1.0? 0.9.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.0
yep, that fixed it. Now I just need some reviews.
adding the usual nomination keywords.
Keywords: adt1.0.0, mozilla1.0
Attachment #80680 - Flags: superreview+
Comment on attachment 80680 [details] [diff] [review]
band-aid fix

> 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 ";1" which
implements the nsIDownload iface. I have a feeling I'll be needing to make one
soon for PPEmbed.
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;


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


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:

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).
Sorry, "it can just provide or register a component with the contact ID
";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?
 should we add that component to the embed package in case they want to use the
XUL progress dialog?
And, in addition to;1, would you also need to implement;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. 
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.
conrad: all you need to do is implement;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.
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 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
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).
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

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
Attached patch take 3, hopefully full patch (obsolete) — Splinter Review
Attachment #80880 - Attachment is obsolete: true
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
diff -u -2 -r1.13.20.1 nsHelperAppDlg.xul
--- mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul	10 Apr 2002 02:37:51 -0000
+++ 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 on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

Attachment #80883 - Flags: review+
Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch for branch checkin
Attachment #80883 - Flags: approval+
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).
Whiteboard: [adt3] [ETA Needed]
they need this today. asap. ETA an hour ago. this does not impact machV at all,
embedding only.
Whiteboard: [adt3] [ETA Needed] → [adt1] [ETA 4/24/02]
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.
the new bug is bug 140136
Assignee: alecf → pinkerton
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.
Keywords: adt1.0.0adt1.0.0+
Whiteboard: [adt1] [ETA 4/24/02] → [adt1] [ETA - Needed NOW for embed]
fixed on branch. trunk upcoming
Keywords: adt1.0.0+fixed1.0.0
landed on trunk (whew)
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 80680 [details] [diff] [review]
band-aid fix

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #80680 - Flags: approval+
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.
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.
I missed Alecf's new bug here:

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. 
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.

Here's the crash log:
nsDownloadManager::AssertProgressInfoFor(const char * 0x0353e0f0) line 269 + 48
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
Resolution: FIXED → ---
ack! nsDownloadManager isn't supposed to be part of the embedded build. I
thought appcomps.dll wasn't in the default embedded product.

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.  
*** Bug 137643 has been marked as a duplicate of this bug. ***
well, it works in mac builds now. over to alecf for other issues.
Assignee: pinkerton → alecf
Is this fixed on the trunk, as well as the branch? If so, pls resolve as fixed.
Alecf checked in a patch for:

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.
Blocks: 126245
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.
ah, that could work... I didn't know embeddors already implemented that. I'll
see what I can do.
works for me with mozilla a1 on windows xp
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.
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
Target Milestone: mozilla1.0 → mozilla1.1beta
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.
moving bugs to mozilla 1.2alpha that missed 1.1beta
Target Milestone: mozilla1.1beta → mozilla1.2alpha
this is no longer a blocker - downloads work now, they just don't show progress.
Lowering to "critical"
Severity: blocker → critical
Priority: P1 → P2
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)
Keywords: helpwanted
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
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.
Whiteboard: [adt1] [ETA - Needed NOW for embed]
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?
Keywords: topembedtopembed-
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.
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
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?
FWIW, since this bug was active, PPEmbed (Mac builds) got its own native
download UI impl and doesn't have this problem.
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.
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.