164498 - Destroy() implementation is unsufficient in BeOS port.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Sergei Dolgov]

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]

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 [e-mail]]

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]

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:
http://bugzilla.mozilla.org/show_bug.cgi?id=129310

Comment 4

[Sergei Dolgov]

Also it is possible that mentioned parent/children relations handling is related
to bug http://bugzilla.mozilla.org/show_bug.cgi?id=157303 - "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]

seems problem is borrowed from Win32 code check for GuiThread

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

in NS_METHOD nsWindow::Destroy()

(mozilla/widget/src/beos/nsWindow.cpp)

Comment 6

[Paul]

taking assignment

Comment 7

[Sergei Dolgov]

Unsure if it is really fixed.
Closing main window don't close related dialogs, like Find.
also, my opinion about bug 
http://bugzilla.mozilla.org/show_bug.cgi?id=187286 is releated to this.

Comment 8

[Sergei Dolgov]

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]

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

Comment 10

[Sergei Dolgov]

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]

Attachment: patch

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]

Comment on attachment 199103 [details] [diff] [review]
patch

traversing kids such way wouldn't work. old classical mistake. preparing new
version.

Comment 13

[Sergei Dolgov]

Attachment: patch

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]

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]

Attachment: patch

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]

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]

Comment on attachment 222257 [details] [diff] [review]
patch

r?

Comment 18

[tqh]

Comment on attachment 222257 [details] [diff] [review]
patch

r=[e-mail]

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]

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]

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

Comment 21

[Doug Shelton]

Should we also land this on the 1.8 branch for Firefox 2.0?

Comment 22

[Sergei Dolgov]

I think we do.
As, besides other things, this patch may fix some potential leaks.

Comment 23

[Sergei Dolgov]

Wondering if we really landed that to branch...

Comment 24

[Doug Shelton]

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]

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]

Attachment: 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
CLOSEWINDOW.

I'm wondering if it affects, when applied, also Thunderbird problem, when Compose window stays opened after sending mail.

Comment 27

[Sergei Dolgov]

Attachment: 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]

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]

Comment on attachment 282827 [details] [diff] [review]
more complete patch for branch

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

Requesting approval for 1.8.1.8.

Comment 30

[Doug Shelton]

(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]

One more for the branch.  It appears 1.8.1.8 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]]

Comment on attachment 282827 [details] [diff] [review]
more complete patch for branch

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 33

[Sergei Dolgov]

Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.91.4.27; previous revision: 1.91.4.26
done