Destroy() implementation is unsufficient in BeOS port.

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: Sergei Dolgov, Assigned: Sergei Dolgov)

Tracking

({fixed1.8.1.8})

Trunk
x86
BeOS
fixed1.8.1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

15 years ago
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

15 years ago
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).
(Assignee)

Comment 3

15 years ago
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
(Assignee)

Comment 4

15 years ago
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.
(Assignee)

Comment 5

15 years ago
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)

Updated

14 years ago
Blocks: 193008

Updated

14 years ago
Status: NEW → ASSIGNED

Comment 6

14 years ago
taking assignment
Assignee: jag → arougthopher
Status: ASSIGNED → NEW
(Assignee)

Comment 7

14 years ago
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.
(Assignee)

Comment 8

14 years ago
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.
(Assignee)

Comment 9

13 years ago
*** Bug 226888 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

13 years ago
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.
(Assignee)

Comment 11

12 years ago
Created attachment 199103 [details] [diff] [review]
patch

draft of better destroy.
Now recurses nsIWidget kids, not BView's children.
also some other things are set in more logical manner
(Assignee)

Updated

12 years ago
Assignee: beos → sergei_d
Status: NEW → ASSIGNED
(Assignee)

Comment 12

12 years ago
Comment on attachment 199103 [details] [diff] [review]
patch

traversing kids such way wouldn't work. old classical mistake. preparing new
version.
Attachment #199103 - Attachment is obsolete: true
(Assignee)

Comment 13

11 years ago
Created attachment 222252 [details] [diff] [review]
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().
(Assignee)

Comment 14

11 years ago
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.
(Assignee)

Comment 15

11 years ago
Created attachment 222257 [details] [diff] [review]
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
Attachment #222252 - Attachment is obsolete: true

Comment 16

11 years ago
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?
(Assignee)

Comment 17

11 years ago
Comment on attachment 222257 [details] [diff] [review]
patch

r?
Attachment #222257 - Flags: review?(thesuckiestemail)

Comment 18

11 years ago
Comment on attachment 222257 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se

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+
(Assignee)

Comment 19

11 years ago
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.
(Assignee)

Comment 20

11 years ago
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 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 21

11 years ago
Should we also land this on the 1.8 branch for Firefox 2.0?
(Assignee)

Comment 22

11 years ago
I think we do.
As, besides other things, this patch may fix some potential leaks.
(Assignee)

Comment 23

10 years ago
Wondering if we really landed that to branch...

Comment 24

10 years ago
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

10 years ago
I double checked.  nsWindow code was restructured as part of bug 322051, landed on 2006-04-11, long after the branch was created.
(Assignee)

Comment 26

10 years ago
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
CLOSEWINDOW.

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

Comment 27

10 years ago
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
Attachment #282824 - Attachment is obsolete: true
(Assignee)

Comment 28

10 years ago
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 29

10 years ago
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.
Attachment #282827 - Flags: review?(doug)
Attachment #282827 - Flags: review+
Attachment #282827 - Flags: approval1.8.1.8?

Comment 30

10 years ago
(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

10 years ago
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 on attachment 282827 [details] [diff] [review]
more complete patch for branch

approved for 1.8.1.8, a=dveditz for release-drivers
Attachment #282827 - Flags: approval1.8.1.8? → approval1.8.1.8+
(Assignee)

Comment 33

10 years ago
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 
Keywords: fixed1.8.1.8
You need to log in before you can comment on or make changes to this bug.