Last Comment Bug 164498 - Destroy() implementation is unsufficient in BeOS port.
: Destroy() implementation is unsufficient in BeOS port.
: fixed1.8.1.8
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 BeOS
: -- normal (vote)
: ---
Assigned To: Sergei Dolgov
: John Morrison
: Neil Deakin
: 226888 (view as bug list)
Depends on:
Blocks: 193008
  Show dependency treegraph
Reported: 2002-08-25 04:19 PDT by Sergei Dolgov
Modified: 2007-10-01 16:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.90 KB, patch)
2005-10-10 14:39 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
patch (8.66 KB, patch)
2006-05-16 13:54 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
patch (7.94 KB, patch)
2006-05-16 14:47 PDT, Sergei Dolgov
thesuckiestemail: review+
Details | Diff | Splinter Review
patch for branch (1.46 KB, patch)
2007-09-29 04:32 PDT, Sergei Dolgov
no flags Details | Diff | Splinter Review
more complete patch for branch (4.92 KB, patch)
2007-09-29 05:24 PDT, Sergei Dolgov
doug: review+
dveditz: approval1.8.1.8+
Details | Diff | Splinter Review

Description Sergei Dolgov 2002-08-25 04:19:17 PDT
Closing window in BeOS Mozilla results in closing native children only (BViews),
but not child windows in Mozilla sence.

E.g. - open Find Dialog. Then close its "parent" window via Menu->File->Close or
closing button in titlebar. Main window is gone, but dialog is still alive.
Comment 1 Paul 2002-08-25 11:35:44 PDT
This does not happen on my build.  Sergei, can you try again, with a clean tree,
or download a nightly build?
Comment 2 hacker formerly known as 2002-11-08 16:09:09 PST
I see this with a current trunk build.  The Find dialog just hangs around until
you hit the Cancel button and then the application exits (if you only had one
browser window open).
Comment 3 Sergei Dolgov 2002-11-09 11:12:19 PST
Currently BeOS port handles parent/children realations only inside single
BWindow - all children are treated as BViews and it only cares about BeOS-native
children in Destroy(). Same in GetParent() function - it casts BView::Parent()
to nsWidget, ignoring some other Mozilla's parent/children relations, like those
between content windows and dialogs.

This approach makes simpler implementation of Move/Resize methods in BeOS ports,
but seems causing some other problems, like annoying scrolling:
Comment 4 Sergei Dolgov 2002-11-17 11:34:50 PST
Also it is possible that mentioned parent/children relations handling is related
to bug - "Mozilla for BeOS
doesn't display images on many sites." - BeOS API has some flackyness not only
for BWindow Z-order handling, but also for BViews belonging to same parent.
Comment 5 Sergei Dolgov 2003-02-27 13:19:20 PST
seems problem is borrowed from Win32 code check for GuiThread

if (toolkit != nsnull && !toolkit->IsGuiThread()) {
		MethodInfo info(this, this, nsWindow::DESTROY);

in NS_METHOD nsWindow::Destroy()

Comment 6 Paul 2003-09-05 17:15:35 PDT
taking assignment
Comment 7 Sergei Dolgov 2003-09-05 18:46:16 PDT
Unsure if it is really fixed.
Closing main window don't close related dialogs, like Find.
also, my opinion about bug is releated to this.
Comment 8 Sergei Dolgov 2003-09-05 18:51:18 PDT
btw, i observed bug with mail composing a bit related to this bug.
after Reply-To and pushing send button, Mozilla freezes again for some, not so
long time at status indicator, and then zombie "Compose" window appears.
It cannot be closed in any way besides closing whole app.
But i have feeling that i saw similar bug for other platforms.
Comment 9 Sergei Dolgov 2004-06-19 09:56:14 PDT
*** Bug 226888 has been marked as a duplicate of this bug. ***
Comment 10 Sergei Dolgov 2004-06-19 10:07:21 PDT
One of problems for improper quitting behaviour is invisible window specifics.
From Mozilla side such window never receives Show() call, but for BWindow it
means that its looper is never started.
Maybe i will open separate bug to fix just BLooper/BWindow->Run() issue, as
Destroy and Quit problem is bit larger, but fixing looper start problem may help
lot of other issues.
Comment 11 Sergei Dolgov 2005-10-10 14:39:45 PDT
Created attachment 199103 [details] [diff] [review]

draft of better destroy.
Now recurses nsIWidget kids, not BView's children.
also some other things are set in more logical manner
Comment 12 Sergei Dolgov 2005-10-10 18:24:53 PDT
Comment on attachment 199103 [details] [diff] [review]

traversing kids such way wouldn't work. old classical mistake. preparing new
Comment 13 Sergei Dolgov 2006-05-16 13:54:37 PDT
Created attachment 222252 [details] [diff] [review]

nsWindowBeOS::DispatchMessage() now catches B_QUIT_REQUESTED and does what QuitRequested did before - sends CLOSEWINDOW message.

nsWindowBeOS::QuitRequested() now returns true,checking if there are some children remained and sending then CLOSEWINDOW message.

nsWindow::CallMethod() case CLOSEWINDOW now iterates through all mozilla children of this widget and sends B_QUIT_REQUESTED to BWindows of those children. It is intended to close dependent dialogs (like those "Find" in SeaMonkey) and hidden popups (like menu items).

StandardWindowCreate for dialogs and popups now adds those "dependent" widgets (with some sort of parents) presented to hierarchy using nsWidget::AddChild(this).
NB!This is only thing I doubt in this patch. It works, but how Mozilla reacts on children which weren't added by "gecko"?

nsWindow::Destroy() restructured for more safe look.

nsDrawingSurface::UnlockDrawable was fixed for safety - because mBitmap may be gone already, UnlockDrawable could decide that we are unlocking onscreen mView and try to unlock non-existent Looper().
Comment 14 Sergei Dolgov 2006-05-16 14:16:46 PDT
Looks like there are problems with destroying from outside (like Deskbar) if there are some hidden windows remained. Probably removing option if (CountChildren() != 0) in QuitRequested may help a bit.

Or maybe in children iteration loop in CloseWindow we should activate those windows to whose we are sending B_QUIT_REQUESTED.

Need bit more experiments. Also, I'm wondering if we really now call Run() (via Show()) for all BLoopers we created, after throuwing away Invisible windows from our code.
Comment 15 Sergei Dolgov 2006-05-16 14:47:39 PDT
Created attachment 222257 [details] [diff] [review]

Last guess was bingo!
We really need Run() looper explicitly in StandardWindowCreate. Without that message chain tends to broke.
IIRC we had it once in code (maybe in Show()/Hide() form, but tqh removed in together with invisible windows when refactored the method).
With Run() we even don't need dirty trick with adding nsChildren artifically.

So new patch
Comment 16 tqh 2006-05-16 23:29:15 PDT
Hmm, did I remove that? That wasn't my intention.

Also if I remember correctly, run left the looper unlocked. So didn't we call some Show, Hide combo?
Comment 17 Sergei Dolgov 2006-05-17 09:10:36 PDT
Comment on attachment 222257 [details] [diff] [review]

Comment 18 tqh 2006-05-17 11:34:19 PDT
Comment on attachment 222257 [details] [diff] [review]

I do think this is a bit 'nasty' and would hope there was a more elegant solution. Can't see anything wrong with it though. Have you checked for leaks?
Comment 19 Sergei Dolgov 2006-05-17 12:31:35 PDT
Only real thing which was added and may look nasty (besides restored Run()) is native children cleanup iteration loop. But seems quite logical safety measure, preventing some leaks. But you are right, maybe we can reach that all with less code in future

And only real essential change is "return true" instead "return false" in nsWindowBeOS::QuitRequested.

Didn't notice anything like mem-leak, and those threads visible in ProcessController - new code seems handling it very nice.

I think if there are some problems, people will report it with new FireFox build, published by tigerdog today. IIRC DownloadManager there was quite sensitive to leaking.
Comment 20 Sergei Dolgov 2006-05-17 12:36:22 PDT
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.124; previous revision: 1.123
Checking in mozilla/widget/src/beos/nsPopupWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsPopupWindow.cpp,v  <--  nsPopupWindow.cpp
new revision: 1.2; previous revision: 1.1
Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.24; previous revision: 1.23
Comment 21 Doug Shelton 2006-06-24 20:50:51 PDT
Should we also land this on the 1.8 branch for Firefox 2.0?
Comment 22 Sergei Dolgov 2006-06-25 15:14:02 PDT
I think we do.
As, besides other things, this patch may fix some potential leaks.
Comment 23 Sergei Dolgov 2007-09-28 07:46:34 PDT
Wondering if we really landed that to branch...
Comment 24 Doug Shelton 2007-09-28 08:02:10 PDT
Sergei, I am pretty sure we did not.  Just looking at the patch, it applies to nsPopupWindow.  If I recall, nsWindow and nsPopupWindow were split after the branch was created.  So this code could not have been applied.
Comment 25 Doug Shelton 2007-09-28 08:30:02 PDT
I double checked.  nsWindow code was restructured as part of bug 322051, landed on 2006-04-11, long after the branch was created.
Comment 26 Sergei Dolgov 2007-09-29 04:32:56 PDT
Created attachment 282824 [details] [diff] [review]
patch for branch

Looks like we need to add minimal part of original patch code to branch to put Mozilla perform "close" operations properly - that is, code in CallMethod case

I'm wondering if it affects, when applied, also Thunderbird problem, when Compose window stays opened after sending mail.
Comment 27 Sergei Dolgov 2007-09-29 05:24:04 PDT
Created attachment 282827 [details] [diff] [review]
more complete patch for branch

Ignore previous comment. It worked for single window only.

This one is complete version of trunk patch, adapted to branch.

Only part for gfx is missing - I will investigate if we really need that cleanup in gfx
Comment 28 Sergei Dolgov 2007-09-29 05:38:23 PDT
Comment on attachment 282827 [details] [diff] [review]
more complete patch for branch

gfx changes were already aplied in some other branch patch. So this is final.

Do we need review for this, Doug?
Comment 29 Doug Shelton 2007-09-29 13:41:19 PDT
Comment on attachment 282827 [details] [diff] [review]
more complete patch for branch

Patch applies cleanly and works as intended.  Firefox and Thunderbird both close cleanly from Deskbar and on shutdown with the patch applied.  

Requesting approval for
Comment 30 Doug Shelton 2007-09-29 13:42:25 PDT
(In reply to comment #26)
> I'm wondering if it affects, when applied, also Thunderbird problem, when
> Compose window stays opened after sending mail.
It doesn't affect this problem.  Compose window still stays open.
Comment 31 Doug Shelton 2007-09-30 23:01:38 PDT
One more for the branch.  It appears will be freezing very soon.  I'm currently unable to update CVS.  If this is approved (I hope it is!) I hope Sergei or maybe Biesi can make the change in CVS.  
Comment 32 Daniel Veditz [:dveditz] 2007-10-01 15:09:30 PDT
Comment on attachment 282827 [details] [diff] [review]
more complete patch for branch

approved for, a=dveditz for release-drivers
Comment 33 Sergei Dolgov 2007-10-01 16:14:34 PDT
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision:; previous revision:

Note You need to log in before you can comment on or make changes to this bug.