Closed Bug 421209 Opened 12 years ago Closed 12 years ago

need to suppress GOTFOCUS dispatch to nsWebShellWindow

Categories

(Core :: Document Navigation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: roc, Assigned: smaug)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Flags: blocking1.9?
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.
Group: security
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.
Attached patch something like this (obsolete) — Splinter Review
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?
Attached patch this one (obsolete) — Splinter Review
Attachment #309628 - Attachment is obsolete: true
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?
Attachment #309630 - Flags: review?(chris)
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.
Attached patch backup (obsolete) — Splinter Review
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
Attached patch v2 (obsolete) — Splinter Review
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
Attachment #309630 - Attachment is obsolete: true
Attachment #309848 - Attachment is obsolete: true
Attachment #309940 - Flags: superreview?(roc)
Attachment #309940 - Flags: review?(chris)
Attachment #309630 - Flags: superreview?(roc)
Attachment #309630 - Flags: review?(chris)
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.
Attached patch v3Splinter Review
Attachment #309940 - Attachment is obsolete: true
Attachment #310279 - Flags: superreview?(roc)
Attachment #310279 - Flags: review?(chris)
Attachment #309940 - Flags: superreview?(roc)
Attachment #309940 - Flags: review?(chris)
Comment on attachment 310279 [details] [diff] [review]
v3

Looks good to me.
Attachment #310279 - Flags: review?(chris) → review+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta5
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.
Attachment #310279 - Flags: review?(jst)
Comment on attachment 310279 [details] [diff] [review]
v3

Not exactly code I generally work on, but looks good to me. sr=jst
Attachment #310279 - Flags: superreview?(roc)
Attachment #310279 - Flags: superreview+
Attachment #310279 - Flags: review?(jst)
This should be blocking1.9+, since this is blocking a P1 bug.
Flags: blocking1.9? → blocking1.9+
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).
Attachment #310279 - Flags: approval1.9b5?
Comment on attachment 310279 [details] [diff] [review]
v3

a=beltzner
Attachment #310279 - Flags: approval1.9b5? → approval1.9b5+
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.
Assignee: roc → Olli.Pettay
I checked in attachment 310814 [details] [diff] [review].
No new leaks and tests and talos look ok.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.