171745 - PPEmbed download will stop if Browser window is closed.

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: Mac, defect)

Description

[moconnell]

Go to a webpage with files to download (www.mozilla.org- make sure its a large file so it 
takes some time to download). Click a link to start a download. Select save from the 
navigation dialog. After the download begins and you have a progress dialog, close the 
browser window behind the progress dialog.

The download stops. 

Results after this point vary and are unpredictable. Sometimes it will crash when trying to 
quit the app.

Thread 0 Crashed:
 #0   0x004ccee0 in CBrowserChrome::OnHideTooltip(void)
 #1   0x034e2900 in 0x34e2900
 #2   0x02b47900 in ChromeTooltipListener::HideTooltip(void)
 #3   0x02b46d20 in ChromeTooltipListener::RemoveChromeListeners(void)
 #4   0x02b457fc in nsDocShellTreeOwner::RemoveChromeListeners(void)
 #5   0x02b449e8 in nsDocShellTreeOwner::WebBrowser(nsWebBrowser *)
 #6   0x02b4ab00 in nsWebBrowser::InternalDestroy(void)
 #7   0x02b4a858 in nsWebBrowser::_dt(void)
 #8   0x02b4ac84 in nsWebBrowser::Release(void)
 #9   0x0069ce54 in nsCOMPtr_base::_dt(void)
 #10  0x004cb418 in _dt__25nsCOMPtr<13nsIWebBrowser>Fv
 #11  0x004d1724 in CBrowserShell::_dt(void)
 #12  0x00516c28 in 0x516c28
 #13  0x0051a0cc in 0x51a0cc
 #14  0x004e16d8 in CBrowserWindow::_dt(void)
 #15  0x004ea0ac in CBrowserApp::AttemptQuitSelf(long)
 #16  0x005087c0 in 0x5087c0
 #17  0x005077f8 in 0x5077f8
 #18  0x00507d9c in 0x507d9c
 #19  0x004e91a8 in CBrowserApp::HandleAppleEvent(AEDesc const &, AEDesc &, 
AEDesc &, long)
 #20  0x00548510 in 0x548510
 #21  0x00544d88 in 0x544d88
 #22  0x005460f0 in 0x5460f0
 #23  0x735fce08 in TryEventTable
 #24  0x735f2d8c in AEMDispatcher
 #25  0x735f3f4c in aeResumeTheCurrentEvent
 #26  0x735f79d8 in aeSend
 #27  0x731c8cac in SendAEToSelf
 #28  0x73114500 in HandleCompatibilityCommandEvent
 #29  0x731b29cc in CompatibilityEventHandler
 #30  0x73118b18 in DispatchEventToHandlers
 #31  0x73101da8 in SendEventToEventTargetInternal
 #32  0x731b65c8 in SendEventToEventTarget
 #33  0x7310ba7c in SendHICommandEvent
 #34  0x731a81dc in SendMenuItemSelectedEvent
 #35  0x7312c70c in HandleKeyboardEvent
 #36  0x73118b18 in DispatchEventToHandlers
 #37  0x73101da8 in SendEventToEventTargetInternal
 #38  0x73150048 in SendEventToEventTargetWithOptions
 #39  0x731a008c in HandleKeyboardEvent
 #40  0x731ac054 in ToolboxEventDispatcherHandler
 #41  0x73118bc4 in DispatchEventToHandlers
 #42  0x73101da8 in SendEventToEventTargetInternal
 #43  0x731b65c8 in SendEventToEventTarget
 #44  0x731d2904 in ToolboxEventDispatcher
 #45  0x731cfc70 in CallEventDispatchHook
 #46  0x73179478 in GetOrPeekEvent
 #47  0x731a1478 in GetNextEventMatchingMask
 #48  0x731ae418 in WNEInternal
 #49  0x731c5518 in WaitNextEvent
 #50  0x005078b0 in 0x5078b0
 #51  0x00507688 in 0x507688
 #52  0x004e604c in main

Comment 1

[Simon Fraser [no longer active]]

There are two problems here.

First, the supercommander of the CMultiDownloadProgress is the browser window,
which causes that to be deleted when the browser window closes. This is
relatively easy to fix.

Second, when no browser windows are open, there are no nsToolkits around to pump
the event queue, and the download stalls. This is harder to fix. The code
currently assumes that only browser windows are gecko 'clients'; there isn't a
way to keep gecko alive when only the download window is showing.

Comment 2

[Simon Fraser [no longer active]]

Attachment: Patch

This patch does two main thing:

1. Sets the supercommander of the CMultiDownloadProgress to the top commander,
rather than the 'current' commander, so that the browser window doesn't close
it when it is closed:

 CMultiDownloadProgress::CMultiDownloadProgress() :
+    LCommander(LCommander::GetTopCommander()),
     mWindow(nil)

2. Have the CBrowserApp keep a ref to the nsIToolkit, so that the toolkit is
kept alive, even when all browser windows are closed (so that downloads
continue). This is a rather ugly fix; better would be to have a singleton
object that does this, and manages the Init/Term embedding stuff.

I made two other changes:

     if (LEventDispatcher::GetCurrentEventDispatcher()) { // Can this ever be
NULL?
	 EventRecord theEvent;
-	 while (::WaitNextEvent(updateMask | activMask, &theEvent, 0, nil))
+	 if (::WaitNextEvent(updateMask | activMask, &theEvent, 0, nil))
+	 {
	    
LEventDispatcher::GetCurrentEventDispatcher()->DispatchEvent(theEvent);
+	 }
     }

This fixed an issue while debugging that caused the app to get into an infinite
loop (we'd never not have update events pending).

		 SInt32    urlTextLen;
		 mURLField->GetText(nil, 0, &urlTextLen);
		 StPointerBlock  urlTextPtr(urlTextLen, true, false);
		 mURLField->GetText(urlTextPtr.Get(), urlTextLen, &urlTextLen);

-		 mBrowserShell->LoadURL(nsDependentCString(urlTextPtr.Get(),
urlTextLen));
+		 nsCAutoString urlString; urlString.Assign(urlTextPtr.Get(),
urlTextLen);
+		 mBrowserShell->LoadURL(urlString);

This fixes an nsDependentCString assertion (must wrap null-terminated string).

Comment 3

[moconnell]

>>>>>
2. Have the CBrowserApp keep a ref to the nsIToolkit, so that the toolkit is
kept alive, even when all browser windows are closed (so that downloads
continue). This is a rather ugly fix; better would be to have a singleton
object that does this, and manages the Init/Term embedding stuff.
<<<<<

This is really ugly for embedders. I would suspect most embedders like myself already 
have a LApplication class other than CBrowserApp. So now to have CBrowserShell 
dependent on CBrowserApp is not good. I agree there should be a singleton in the 
embedding API that is part of the Init/Terminate routines.

I can mimic this code in my application and make similar changes in CBrowserShell to 
use the routines in my application. However, its not a good long term fix.

I'm willing to use it for now since I'm on a very tight schedule

Comment 4

[Jaime Rodriguez, Jr.]

This is a blocker for a major embedding customer, so I am marking it as
nsbeta1+/topembed+.

Comment 5

[Michael Buckland]

jaime nominated this via email so marking adt1.0.2
I did not see r= or sr= but correct me if I am wrong.

Comment 6

[Mike Pinkerton (not reading bugmail)]

-                mBrowserShell->LoadURL(nsDependentCString(urlTextPtr.Get(),
urlTextLen));
+                nsCAutoString urlString; urlString.Assign(urlTextPtr.Get(),
urlTextLen);
+                mBrowserShell->LoadURL(urlString);

it might fix the assert, but doesn't it cause an extra copy? i've never been too
clear on nsDependentString. can't you just change it to |urlTextLen +
sizeof(char)| to tell nsDependentCString there's a null in teh buffer?

-        while (::WaitNextEvent(updateMask | activMask, &theEvent, 0, nil))
+        if (::WaitNextEvent(updateMask | activMask, &theEvent, 0, nil))

this needs to be a |while| otherwise it doesn't work, says Matt. Perhaps you
should comment it to explain that if you get stuck here when debugging to change
it to an |if|, but it needs to stay |while| for regular runtime.

i also agree this is skanky, and as matt says, doesn't work for apps that use
their own app. we should file a bug on conrad to rework this a la chimera's
mechansim which registers windows that "care about gecko" with a centralized object.

r=pink to keep the ball rolling.

Comment 7

[Simon Fraser [no longer active]]

Well, the while() loop prevents this from working when debugging, and, I
suspect, may hose some apps that generate lots of update events. Maybe we should
just handle the activate?

> it might fix the assert, but doesn't it cause an extra copy?

Yeah, but what's one copy when you're just about to save a file? It's safer.

Comment 8

[Scott Collins]

Comment on attachment 101187 [details] [diff] [review]
Patch

why did you change from using a dependent string to making a copy before
calling |LoadURL|?

Comment 9

[Simon Fraser [no longer active]]

scc: because 'urlTextPtr' is not null-terminated, as nsDependentString requires.
It asserted.

Comment 10

[jag (Peter Annema)]

The comment above the assertion tells you what to do:

http://lxr.mozilla.org/seamonkey/source/string/public/nsDependentString.h#68

In this case: |Substring(urlTextPtr.Get(), urlTextPtr.Get() + urlTextLen)|

Comment 11

[Conrad Carlen (not reading bugmail)]

> I would suspect most embedders like myself already 
have a LApplication class other than CBrowserApp. So now to have CBrowserShell 
dependent on CBrowserApp is not good.

I agree. I'm very against adding this dependency on a toolkit. If we want to
pump the PLEvent queue whether there are any open windows, I have a patch for
that which uses Carbon events. I'm back in action and will be on this. 

Comment 12

[Conrad Carlen (not reading bugmail)]

> Second, when no browser windows are open, there are no nsToolkits around to pump
the event queue, and the download stalls.

The event queue pumping in PPEmbed is not dependent on having a window open or
an nsToolkit. It's done here:
http://lxr.mozilla.org/seamonkey/source/embedding/browser/powerplant/source/EmbedEventHandling.cpp#262
This repeater is attached to the embedding app and gets called every time thru
the event queue - window or not.

That the supercommander of the download progress is the browser window and so
closing that browser window kills the download is the main problem.

Taking bug.

Comment 13

[Simon Fraser [no longer active]]

> The event queue pumping in PPEmbed is not dependent on having a window open or
> an nsToolkit. It's done here:

Actually, it is. The nsToolkit is released when the last browser window is
closed, so gets removed from the list of repeaters, and stops calling processing
the event queue. Something at the app level needs to hold a ref to the
nsIToolkit; at present, only browser window-level things do.

Comment 14

[Conrad Carlen (not reading bugmail)]

> Actually, it is. The nsToolkit is released when the last browser window is
closed

Yep - you're right. BTW, I just took this bit from the patch:

 CMultiDownloadProgress::CMultiDownloadProgress() :
+    LCommander(LCommander::GetTopCommander()),
     mWindow(nil)

along with the patch on bug 44678 and that fixes this bug - without exposing
nsIToolkit and creating evil dependencies in the PPEmbed code. The patch for bug
44678 makes the toolkit a no-op on Carbon. Granted, it's a riskier patch (not in
terms of stability, but in terms of odd perf regressions) but it is what we
should be doing.

Comment 15

[Jaime Rodriguez, Jr.]

Removing adt1.0.2 nomination as this does not yet have a super review, nor bake
time on the trunk.

Comment 16

[saari (gone)]

does this still need to land?

Comment 17

[Conrad Carlen (not reading bugmail)]

> does this still need to land?

No, it should never land. See comment 14. Bug 44678 should be landed in 1.3.

Comment 18

[Conrad Carlen (not reading bugmail)]

Since bug 44768 is in, this should be a one-liner. See comment 14.

Comment 19

[Conrad Carlen (not reading bugmail)]

Moving milestone.

Comment 20

[Conrad Carlen (not reading bugmail)]

-> Future. Focus is shifting from PowerPlant to Cocoa.

Comment 21

[Juan José Mata]

5/5 EDT triage: minusing topembed+ status.

Comment 22

[Juan José Mata]

After talking to Samir, removing bogus keyword and status whiteboard monikers.

Lowering to minor severity given that PPEmbed is no longer a priority.  Sounds
like we weren't even happy with the approach we were taking here to begin with
anyway.

Probably should be closed WONTFIX, Conrad?

Comment 23

[Benjamin Smedberg]

Mass change of bugs in the Embedding: Mac component in preparation for archiving it. I don't believe any of these are useful any more, as we don't have a mac embedding API. If they are relevant, they should be moved to an alternate component.