112294 - calling an alert box from window.onblur triggers the event multiple times

Status: NEW

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Madhur Bhatia]

NS6.2 - build: 2001-10-22-0.9.4 branch
NS6.2.1 - build : 2001-11-26-6.2.1 rtm

the window.onblur JS event is triggered 4 times. Happens on all platforms.

Comment 1

[Madhur Bhatia]

Attachment: testcase

Comment 2

[Madhur Bhatia]

the onblur event is getting triggered 2 times on win2000 build id : 
2002-03-18-05trunk

Comment 3

[Madhur Bhatia]

as mentioned in comment #2,
on win2000  ---- BuildID : 2002-03-28-06trunk
the onblur event called for the window element is trigerred 2 times.

Now, i tested the MFCembed build for 2002-03-27 
I am unable to dimiss the alert box called by the onblur event for the window 
element. The event goes into a loop.

cc'ing mdunn and chrisd.

Comment 4

[Daniel Wang]

dupe of bug 134293?

Comment 5

[Daniel Wang]

A test suite has been created to track bug 58441, bug 134293,
bug 105129, bug 131843, bug 50221, bug 134321, bug 122311,
and bug 112294:

http://bugzilla.mozilla.org/attachment.cgi?id=100778&action=view

People who have reported problems in the bugs mentioned
please perform all the test in this suite. All bugs will be resolved
as duplicate of the appropriate bug unless someone can reproduce
a problem with onblur/onfocus style change (no alert involved)

Comment 6

[Jesse Ruderman]

Bug 210240 (security-sensitive) is probably a dup.

Comment 7

[Pham]

.

Comment 8

[Brendan Eich [:brendan]]

See comment 6 and dup if you can.

/be

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This looks really old? Why should it be a blocker? Should we rather mark bug 210240 as a blocker?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not a blocker, this has been around forever. Would be awesome if someone could fix it though.

Comment 11

[Ere Maijala (slow)]

Attachment: Partial fix

Just for the record, here is a partial fix. It will break the infinite loop and a an assertion that's caused when deactivation, focus shift or such causes activation which will unsuppress focus controller and then tries to unsuppress again. This does not yet fix another issue: if the alert is caused by the last window being closed, a modal loop is entered but the dialog is not displayed. I'll try to fix that too.

Comment 12

[Ere Maijala (slow)]

Attachment: Complete patch v1

Comment 13

[Ere Maijala (slow)]

I'll need to do some additional testing before requesting reviews.

Comment 14

[Ere Maijala (slow)]

Attachment: Complete patch v1.1

Oops, the previous version was missing a crucial change in nsWindowWatcher.cpp.

Comment 15

[Ere Maijala (slow)]

Attachment: Complete patch v1.1 for review

I haven't noticed any side effects. Neil, would you be able to review?

Comment 16

[[e-mail]]

Comment on attachment 323728 [details] [diff] [review]
Complete patch v1.1 for review

This patch certainly fixes the infinite alert case, although I still get two alerts, and focus is still lost from the window, so after switching away, switching to the alert, closing the alert, closing the second alert, I have to switch away and switch back again before I can type.

Comment 17

[Ere Maijala (slow)]

Comment on attachment 323728 [details] [diff] [review]
Complete patch v1.1 for review

True, still need to work some more on it.

Comment 18

[Ere Maijala (slow)]

Attachment: Complete patch v1.2

This is finally something. The only remaining issue is that if the blur event is caused by switching to another window, dismissing the dialog will cause another one. This is apparently because of how Windows juggles the focus as the document also gets focus before the second blur. I'll need to test this one a bit, though.

Comment 19

[Ere Maijala (slow)]

Comment on attachment 326370 [details] [diff] [review]
Complete patch v1.2

Double focus stuff would continue in bug 261074. Otherwise this should be ok.

Comment 20

[[e-mail]]

Comment on attachment 326370 [details] [diff] [review]
Complete patch v1.2

>+          nsCOMPtr<nsIDocument> lastFocusedDocument = gLastFocusedDocument;
>+          nsPresContext* lastFocusedPresContext = gLastFocusedPresContext;
>+          mCurrentTarget = nsnull;
>+          NS_IF_RELEASE(gLastFocusedDocument);
>+          gLastFocusedPresContext = nsnull;
Nit: use
nsCOMPtr<nsIDocument> lastFocusedDocument;
lastFocusedDocument.swap(gLastFocusedDocument);
to save on an addref/release pair.

Comment 21

[[e-mail]]

Comment on attachment 326370 [details] [diff] [review]
Complete patch v1.2

OK, this looks good on Windows XP at least.

Comment 22

[Ere Maijala (slow)]

Attachment: Complete patch v1.2b with the nit fixed

Ok, fixed the nit.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        if (parentChrome)
+        {
+          nsCOMPtr<nsIBaseWindow> parentWindow(do_GetInterface(parentTreeOwner));
+          nsCOMPtr<nsIWidget> parentWidget;
+          if (parentWindow)
+            parentWindow->GetMainWidget(getter_AddRefs(parentWidget));
+          if (parentWidget)
+            parentWidget->IsVisible(parentVisible);            
+        }

Can this be shared with nsXULWindow somehow?

Can this be tested with mochitests? We really really need more focus tests.

Comment 24

[Ere Maijala (slow)]

(In reply to comment #23)
> +        if (parentChrome)
> +        {
> +          nsCOMPtr<nsIBaseWindow>
> parentWindow(do_GetInterface(parentTreeOwner));
> +          nsCOMPtr<nsIWidget> parentWidget;
> +          if (parentWindow)
> +            parentWindow->GetMainWidget(getter_AddRefs(parentWidget));
> +          if (parentWidget)
> +            parentWidget->IsVisible(parentVisible);            
> +        }
> 
> Can this be shared with nsXULWindow somehow?

There's only four lines that could be shared. Worth it?

> Can this be tested with mochitests? We really really need more focus tests.

Probably at least to some extent, but I know shamefully little about mochitests to know for sure. 

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #24)
> (In reply to comment #23)
> > +        if (parentChrome)
> > +        {
> > +          nsCOMPtr<nsIBaseWindow>
> > parentWindow(do_GetInterface(parentTreeOwner));
> > +          nsCOMPtr<nsIWidget> parentWidget;
> > +          if (parentWindow)
> > +            parentWindow->GetMainWidget(getter_AddRefs(parentWidget));
> > +          if (parentWidget)
> > +            parentWidget->IsVisible(parentVisible);            
> > +        }
> > 
> > Can this be shared with nsXULWindow somehow?
> 
> There's only four lines that could be shared. Worth it?

Not sure. See how it looks.

> > Can this be tested with mochitests? We really really need more focus tests.
> 
> Probably at least to some extent, but I know shamefully little about
> mochitests to know for sure. 

We'd better try.

Comment 26

[Ere Maijala (slow)]

Attachment: Patch v1.2c with the removal of the assertion added back

The previous patch was missing the removal off the extraneous assertion from nsFocusController.cpp. Otherwise I didn't change it and didn't find a meaningful way to combine the parent window checks.

Comment 27

[Ere Maijala (slow)]

Attachment: Mochitest for the case

Roc, would this be ok for a test?

Comment 28

[Ere Maijala (slow)]

Comment on attachment 330095 [details] [diff] [review]
Patch v1.2c with the removal of the assertion added back

Smaug, could you try this and perhaps the test on Linux?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The test looks good, but I think it would be a bit better if the focusing and closing of the popup windows happened asynchronously off a timer, in case there is asynchronous processing going on to show the window or change window focus.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 330096 [details] [diff] [review]
Mochitest for the case

awaiting response

Comment 31

[Ere Maijala (slow)]

Comment on attachment 330096 [details] [diff] [review]
Mochitest for the case

I can do that if I don't check the number of times onfocus was called. But I'd like to keep it there. Isn't a real alert dialog also synchronous?

I can add some delay before checking results to make sure onblur has been executed. Would that be ok?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A real alert dialog runs the event loop, but this doesn't as far as I know.

I don't know why doing the focus and close asynchronously would prevent you from counting the onfocus events.

Comment 33

[Ere Maijala (slow)]

Right, but a real alert wouldn't return from onblur until the dialog is dismissed.

Of course I can count the focus provided there is enough delay to make sure it has happened before checking. Or did you mean doing focus, then close, then checks and finish?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 330096 [details] [diff] [review]
Mochitest for the case

This gets too complicated. Let's just do this.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 330095 [details] [diff] [review]
Patch v1.2c with the removal of the assertion added back

>+          // Clear our global variables before firing the event to prevent
>+          // duplicate blur events (bug 112294).
>+          nsCOMPtr<nsIDocument> lastFocusedDocument;
>+          lastFocusedDocument.swap(gLastFocusedDocument);
>+          nsPresContext* lastFocusedPresContext = gLastFocusedPresContext;
>+          mCurrentTarget = nsnull;
>+          gLastFocusedPresContext = nsnull;
What keeps lastFocusedPresContext alive while/after dispatching blur to |document|?
Or is it possible that it points to a deleted object when dispatching blur to |window|?

Now compiling this on linux...

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Works ok on Linux. So r=me if that lastFocusedPresContext issue is solved.

Comment 37

[Ere Maijala (slow)]

I can't see the changes make it any different from what it was before. Before gLastFocusedPresContext was set null right after dispatching the message.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

gLastFocusedPresContext is set null if the prescontext it points to is deleted.
So if prescontext is deleted when dispatching blur to document, gLastFocusedPresContext is nsnull (and dispatching with null prescontext is possible). But as far as I see, nothing makes sure that lastFocusedPresContext
points to a prescontext which actually is alive after dispatching blur to document.

Comment 39

[Ere Maijala (slow)]

Attachment: Patch v1.3

Thanks for catching that. Made the pointer strong. Assuming reviews..

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 330803 [details] [diff] [review]
Patch v1.3

Looks ok to me.

Comment 41

[Ere Maijala (slow)]

Checked in, taking shelter..

Comment 42

[(not currently active) Ted Mielczarek]

This mochitest has been failing on the linux unittest boxes since you checked it in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1216799721.1216802670.26738.gz

Comment 43

[(not currently active) Ted Mielczarek]

In fact, this caused orange on every single unit test box, most of them continuously since last night. You either need to fix this test ASAP or back out.

Comment 44

[(not currently active) Ted Mielczarek]

I backed this out:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ed6642a02b2d

Comment 45

[Ere Maijala (slow)]

Sorry about that. I couldn't get a network connection to fix it in timely fashion. Will get back to it.

Comment 46

[aja+bugzilla]

Bug 447613 was apparently caused by this checkin, and was resolved by the backout in comment 44.

Comment 47

[Ere Maijala (slow)]

So the testcase didn't actually work and the patch caused another problem. Back to debugging..

Comment 48

[Ere Maijala (slow)]

Attachment: Patch v1.4

Otherwise the same but including a fix for the stupid mistake in nsWindow.cpp. mIsVisible was used in the function to see what the previous state was so setting it early confused the function. 

I'll fix the test case separately. Let's get this in first, ok?

Comment 49

[Martijn Wargers (dead)]

Print preview was broken too in the 2008-07-23 build. In the hourly build there wasn't a problem anymore, so I assume it was caused by the patch in this bug. Just a fyi, to make sure the new patch doesn't give this problem.

Comment 50

[[e-mail]]

Comment on attachment 330953 [details] [diff] [review]
Patch v1.4

I managed to crash with this patch applied :-( I'll try updating to see if I had a bad pull.

Comment 51

[Ere Maijala (slow)]

Ok, let's see how it goes. I haven't had a single crash, but better be safe. If you manage to crash again, it would be great to have a stack trace.

Comment 52

[Ere Maijala (slow)]

Neil, any conclusion?

Comment 53

[Ere Maijala (slow)]

Comment on attachment 330953 [details] [diff] [review]
Patch v1.4

I crashed this too.

Comment 54

[Ere Maijala (slow)]

Attachment: Crash stack

Smaug, any idea how the patch would cause this? Looks like DocumentViewerImpl is holding a bad reference to mPresContext.

Comment 55

[Ere Maijala (slow)]

Attachment: Another crash, another stack

Another crash stack. Different place, but again pres context.

Comment 56

[Ere Maijala (slow)]

I think I finally noticed the problem. gLastFocusedPresContext is a weak pointer, so must not use swap..

Comment 57

[Ere Maijala (slow)]

And because it's weak, is it actually safe to assign it to a nsCOMPtr?

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Ah, indeed swap shouldn't be used.
It is ok to assing to nsCOMPtr

Comment 59

[Ere Maijala (slow)]

Attachment: Patch v1.5

Fixed patch. Again.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 331659 [details] [diff] [review]
Patch v1.5

r=me
But did you say you still need to fix the tests?

Comment 61

[Ere Maijala (slow)]

Comment on attachment 331659 [details] [diff] [review]
Patch v1.5

I need to fix the test for this bug. It will be another patch. The existing tests should be fine.

Comment 62

[Ere Maijala (slow)]

The patch is now in trunk. I haven't been able to make the test case work reliably yet.

Comment 63

[Ere Maijala (slow)]

I'm unable to simulate the alert and other cirmustances properly in a mochitest. What shall we do (roc?)?

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try using an actual alert() in the child window and dismissing it by synthesizing a keyboard event via setTimeout in the original window?

Comment 65

[Boris Zbarsky [:bzbarsky]]

Can someone explain to me what the point of the window watcher change was, here?  That change looks wrong to me, since it will cause whatever is being opened to be opened as chrome even if the chrome flag is not set...

Comment 66

[Ere Maijala (slow)]

To fix the issue mentioned in comment #11: if the alert is caused by the last
window being closed, a modal loop is entered but the dialog is not displayed.

Comment 67

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I see the alert dialog only once on Win11 and Linux. It seems that this has already fixed by the tab-modal dialog or something. We need to check this on macOS too, though.