http://mxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsWebShellWindow.cpp#452 nsWebShellWindow receives GOTFOCUS events *not* via the view manager. So it's susceptible to the same problems that we fixed for most cases in bug 399852. We need a similar fix for nsWebShellWindow, although it should be a bit simpler for nsWebShellWindow.

It is not clear to me what is the right place to unsuppress focus. The stack trace (for an assertion) in bug 395609 shows that there is some focus processed when destroying window/docshell/presshell. Unload event is dispatched in nsDocShell::Destroy(), so I guess focus shouldn't be suppressed at that point. DocumentViewerImpl::Destroy() might be better place, or nsDocShell::Destroy(), but just after dispatching unload.

I just noticed there is already some kind of blur suppression. (Bug 68454)

Though, I'm not at all sure blur suppression is used at all. Bug 68454 added blur suppression check to nsGlobalWindow ::HandleDOMEvent. It was wrong and didn't even suppress blur in all cases. 1.8 has the GetBlurSuppression() still in ::HandleDOMEvent. Apparently it was me who removed GetBlurSuppression() call when I rewrote event dispatching :) (Seems like there is still something to clean up.) During Bug 68454 time window closing was handled in a quite different way and there wasn't event target chain to keep things alive. ...but anyway, blur suppression code isn't something that can be used to fix this bug, as far as I see.

Wouldn't we unsuppress where bug 399852 unsuppresses? Namely, in nsFocusEventSuppressor::Unsuppress, which is called by nsCSSFrameConstructor::EndUpdate?

That is one place to unsuppress, but the stack trace in bug 395609 shows that view is destroyed when presshell is destroyed. At that point we aren't updating CSSFC.

> nsDocShell::Destroy(), but just after dispatching unload. That sounds safest, since we know unload can do arbitrary things.

I still wonder what is the right interface to add focussuppressing. nsViewManager and nsWebShellWindow should use the same thing to check if focus is suppressed and when unsuppressing both them should be called so that they can do "something". Or should there be focus suppressing service - that sounds too heavy.

Move nsFocusEventSuppressor out to nsContentUtils or nsLayoutUtils?

But nsFocusEventSuppressor is using viewmanager. That needs to be extended somehow - nsWebShellWindow should be notified when to suppress focus handling.

Right, add nsWebShellWindow hooks to nsFocusEventSuppressor.

And nsWebShellWindow is in a different library so that causes some extra complexity.

You could add methods to one of the interfaces on nsWebShellWindow so we get an instance and call them, even though they only use global data.

that is one ugly possibility.

this needs testing, especially on windows (which I don't have). Would be great to try Bug 395609 and this together to see if the assertion is still there. Anyone able to help here, please?

The patch passes mochitest, browser test and chrome test on linux. (Found bug 423138 while testing, but that isn't related to this bug)

(In reply to comment #14) > Would be great to try Bug 395609 and this together to see if the assertion is > still there. > Anyone able to help here, please? I've now rebuilt with your patch, I don't see any relevant assertion with your bug 395609. Also, I don't get the assertion with the testcase from bug 407152.

Comment on attachment 309630 [details] [diff] [review] this one Chris, since you made the first focussuppressor patch, does this look reasonable to you? Or roc, any comments?

Comment on attachment 309630 [details] [diff] [review] this one It's a bit icky having every focus suppresison observer check/remember the suppression count. Could you move the suppression counting logic into the nsFocusEventSuppressorService::Suppress()/Unsuppress(), so that observers are only notified when the suppression state changes, rather than every time someone calls Suppress()/Unsuppress()? If observers are added while focus is suppressed, you should notify the new observer immediately. You could then change the suppress counts in the observers to boolean flags.

This is a really heavyweight solution. My suggestion in comment #12 would have been considerably simpler. Was it really too ugly, given that compositor will make this obsolete?

IMO that would have been quite ugly and would have had to hardcode things to suppressor etc. And to decide which interface should have that (un)suppress method... When the not-in-use-blur-suppressor was added, empty methods had to be added to quite many places. This isn't so heavyweight. There are only 2 observers; static functions of nsViewManager and nsWebShellWindow. Or the heavyweight part is creating separate service - but that was the least ugly way I found. Btw, I think we need to suppress also NS_ACTIVATE and NS_DEACTIVATE :( Those are pretty much always dispatched right after NS_GOT/LOSTFOCUS and may cause some scripts to run. Could anyone test that on windows/mac? (Why is Windows processing events when deleting widgets :/ )

When an HWND with focus is deleted, Windows synchronously sends a focus event to the new HWND. I'm not aware that it also sends an ACTIVATE although it may do. I think it's overkill to create a full-fledged XPCOM service that supports an arbitarary number of listeners when there are only two listeners and we know exactly who they are. But if you insist it has to be this way, I'm not interested in fighting it.

(In reply to comment #22) > When an HWND with focus is deleted, Windows synchronously sends a focus event > to the new HWND. I'm not aware that it also sends an ACTIVATE although it may > do. NS_ACTIVATE/DEACTIVATE are gecko events, dispatched after GOTFOCUS/LOSTFOCUS. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=3.734&mark=4529-4534#4525 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=3.734&mark=4707,4708#4706 > I'm not interested in fighting it. I'm not interested in fighting either. I just couldn't find any better solution and this bug must be fixed.

It would be better if at least you let nsCSSFrameConstructor have direct access to the focussuppressor since they're in the same module, and let the focussuppressor have direct access to the view manager without registering a callback. Then you only need to store two callback pointers instead of using arrays. + if (widget) { + nsEventStatus status; + nsGUIEvent event(PR_TRUE, NS_GOTFOCUS, widget); + widget->DispatchEvent(&event, status); Is this the best way to do it? Seems to me it would be cleaner if we just called nsWebShellWindow::HandleEvent. + (*PR_CALLBACK FOCUS_EVENT_SUPPRESSOR_CALLBACK) Why is this all caps? And what Chris said about only notifying when the count changes to/from zero ... except you shouldn't need to worry about someone adding an observer while focus is suppressed, just add an assertion that that doesn't happen.

This has simpler service and some code for activate/deactivate handling in webshellwindow. Backing up for the night.

> NS_ACTIVATE/DEACTIVATE are gecko events, dispatched after GOTFOCUS/LOSTFOCUS. Hmm, but they're only sent if a WM_ACTIVATE event has just occurred, which it normally won't have in this situation. I suppose you could have a nasty situation where a WM_ACTIVATE occurs which sets gJustGot(De)activate and either directly or due to some Gecko event we destroy stuff and trigger a GOTFOCUS event... Maybe we do need another observer, in widget/windows, to save and restore gJustGot(De)activate? Maybe we could add the suppressor interface to nsIToolkit? That would remove the need for the service altogether.

I built attachment 309630 [details] [diff] [review] ("this one") on Windows, and when I run the bloat tests, I don't see any assertions failures, as dbaron mentioned in bug 395609 comment 65. None of the test cases for 399852 and its dependencies regressed either.

(In reply to comment #27) > I built attachment 309630 [details] [diff] [review] ("this one") on Windows, and when I run the bloat > tests, I don't see any assertions failures, as dbaron mentioned in bug 395609 > comment 65. None of the test cases for 399852 and its dependencies regressed > either. > But I do see the following assertion fail several times, both with and without the patch: ###!!! ASSERTION: No docshell tree item for mDOMNode: 'docShellTreeItem', file c:/work/src/head/mozilla/accessible/src/base/nsAccessNode.cpp, line 372 WARNING: NS_ENSURE_TRUE(docShellTreeItem) failed: file c:/work/src/head/mozilla/accessible/src/base/nsDocAccessible.cpp, line 711

Cleaned up a bit. Handling NS_ACTIVATE/DEACTIVATE may not be needed after all, if Windows just dispatches focus as it seems. This patch doesn't look too complicated, IMO. Enhancing the existing class for suppress handling (and making it a service), adding GOTFOCUS/LOSTFOCUS handling to nsWebShellWindow and suppress/unsuppress calls to docshell. + some DEBUG_smaug printfs

I think you can just remove the DEBUG_saari checks :-) What about this from earlier: + if (widget) { + nsEventStatus status; + nsGUIEvent event(PR_TRUE, NS_GOTFOCUS, widget); + widget->DispatchEvent(&event, status); Is this the best way to do it? Seems to me it would be cleaner if we just called nsWebShellWindow::HandleEvent. Can't you use NS_IMPL_ISUPPORTS2? (It hurts me that I remember that without looking.) +static nsAutoTArray<nsFocusEventSuppressorCallback, 2> sSuppressCBs; +static nsAutoTArray<nsFocusEventSuppressorCallback, 2> sUnsuppressCBs; Is this allowed, relying on static destructors? Why not just have 2 static arrays of 2 elements each and assert we never need more than that? Better still, simplify more and just have one callback function that takes a boolean parameter?

(In reply to comment #30) > I think you can just remove the DEBUG_saari checks :-) I could :) > + if (widget) { > + nsEventStatus status; > + nsGUIEvent event(PR_TRUE, NS_GOTFOCUS, widget); > + widget->DispatchEvent(&event, status); > > Is this the best way to do it? Seems to me it would be cleaner if we just > called nsWebShellWindow::HandleEvent. Really re-dispatching the event is IMO the right way to do it. What if widget::dispatch wants to do something to the event. > Can't you use NS_IMPL_ISUPPORTS2? (It hurts me that I remember that without > looking.) Could do. Doesn't matter. > +static nsAutoTArray<nsFocusEventSuppressorCallback, 2> sSuppressCBs; > +static nsAutoTArray<nsFocusEventSuppressorCallback, 2> sUnsuppressCBs; > > Is this allowed, relying on static destructors? Well, static destructor doesn't need to free anything here - in practice. In any case I don't know why this would cause problems. This isn't keeping strong ref or anything (like a static nsCOMPtr would) > Why not just have 2 static > arrays of 2 elements each and assert we never need more than that? Hardcoding array size, ugh. > Better > still, simplify more and just have one callback function that takes a boolean > parameter? That might simplify the code still a bit.

(In reply to comment #31) > Really re-dispatching the event is IMO the right way to do it. > What if widget::dispatch wants to do something to the event. Then it's already done it and you don't want to do it again.

Comment on attachment 310279 [details] [diff] [review] v3 Looks good to me.

Roc, will you have time to sr this any time soon?

Comment on attachment 310279 [details] [diff] [review] v3 If roc doesn't have time for this right now, could you jst look at this. This is blocking P1 blocker.

Comment on attachment 310279 [details] [diff] [review] v3 Not exactly code I generally work on, but looks good to me. sr=jst

This should be blocking1.9+, since this is blocking a P1 bug.

Comment on attachment 310279 [details] [diff] [review] v3 Trying to get this in to b5, so that I could hopefully re-land bug 395609 (and bug 420415 after that).

Comment on attachment 310279 [details] [diff] [review] v3 a=beltzner

Argh, sorry, I should have checked leaks already earlier. Anyway, this is a small change, which keeps leaks down. nsAutoTArray<nsFocusEventSuppressorCallback, 2> -> nsTArray<nsFocusEventSuppressorCallback>* sCallbacks and adding shutdown function.

I checked in attachment 310814 [details] [diff] [review]. No new leaks and tests and talos look ok.