Closed Bug 164498 Opened 22 years ago Closed 18 years ago

Destroy() implementation is unsufficient in BeOS port.


(Core :: XUL, defect)

Not set





(Reporter: sergei_d, Assigned: sergei_d)



(Keywords: fixed1.8.1.8)


(2 files, 3 obsolete files)

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.
This does not happen on my build.  Sergei, can you try again, with a clean tree,
or download a nightly build?
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).
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:
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.
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()

Blocks: 193008
taking assignment
Assignee: jag → arougthopher
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.
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.
*** Bug 226888 has been marked as a duplicate of this bug. ***
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.
Attached patch patch (obsolete) — Splinter Review
draft of better destroy.
Now recurses nsIWidget kids, not BView's children.
also some other things are set in more logical manner
Assignee: beos → sergei_d
Comment on attachment 199103 [details] [diff] [review]

traversing kids such way wouldn't work. old classical mistake. preparing new
Attachment #199103 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter 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().
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.
Attached patch patchSplinter 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
Attachment #222252 - Attachment is obsolete: true
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 on attachment 222257 [details] [diff] [review]

Attachment #222257 - Flags: review?(thesuckiestemail)
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?
Attachment #222257 - Flags: review?(thesuckiestemail) → review+
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.
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
Closed: 18 years ago
Resolution: --- → FIXED
Should we also land this on the 1.8 branch for Firefox 2.0?
I think we do.
As, besides other things, this patch may fix some potential leaks.
Wondering if we really landed that to branch...
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.
I double checked.  nsWindow code was restructured as part of bug 322051, landed on 2006-04-11, long after the branch was created.
Attached patch patch for branch (obsolete) — Splinter Review
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.
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
Attachment #282824 - Attachment is obsolete: true
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?
Attachment #282827 - Flags: review?(doug)
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
Attachment #282827 - Flags: review?(doug)
Attachment #282827 - Flags: review+
Attachment #282827 - Flags: approval1.8.1.8?
(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.
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 on attachment 282827 [details] [diff] [review]
more complete patch for branch

approved for, a=dveditz for release-drivers
Attachment #282827 - Flags: approval1.8.1.8? → approval1.8.1.8+
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision:; previous revision:
Keywords: fixed1.8.1.8
You need to log in before you can comment on or make changes to this bug.