Open Bug 112294 Opened 18 years ago Updated 1 year ago

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

Categories

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

x86
All
defect
Not set

Tracking

()

ASSIGNED
mozilla1.1alpha

People

(Reporter: madhur, Assigned: emaijala+moz)

References

Details

Attachments

(2 files, 12 obsolete files)

268 bytes, text/html
Details
22.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
Attached file testcase
Target Milestone: --- → mozilla1.1
the onblur event is getting triggered 2 times on win2000 build id : 
2002-03-18-05trunk
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.
Summary: calling an alert box from window.onblur triggers the event 4 times → calling an alert box from window.onblur triggers the event multiple times
QA Contact: madhur → rakeshmishra
dupe of bug 134293?
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)
QA Contact: rakeshmishra → trix
Bug 210240 (security-sensitive) is probably a dup.
.
Assignee: joki → saari
QA Contact: trix → ian
Blocks: 339467
Assignee: saari → events
See comment 6 and dup if you can.

/be
Flags: blocking1.9?
This looks really old? Why should it be a blocker? Should we rather mark bug 210240 as a blocker?
Not a blocker, this has been around forever. Would be awesome if someone could fix it though.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Assignee: events → emaijala
Attached patch Partial fix (obsolete) — Splinter Review
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.
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Blocks: 392148
Attached patch Complete patch v1 (obsolete) — Splinter Review
Attachment #287864 - Attachment is obsolete: true
I'll need to do some additional testing before requesting reviews.
Status: NEW → ASSIGNED
Attached patch Complete patch v1.1 (obsolete) — Splinter Review
Oops, the previous version was missing a crucial change in nsWindowWatcher.cpp.
Attachment #322425 - Attachment is obsolete: true
Attached patch Complete patch v1.1 for review (obsolete) — Splinter Review
I haven't noticed any side effects. Neil, would you be able to review?
Attachment #323728 - Flags: review?(neil)
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 on attachment 323728 [details] [diff] [review]
Complete patch v1.1 for review

True, still need to work some more on it.
Attachment #323728 - Flags: review?(neil)
Attached patch Complete patch v1.2 (obsolete) — Splinter Review
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.
Attachment #322565 - Attachment is obsolete: true
Attachment #323728 - Attachment is obsolete: true
Blocks: 261074
Comment on attachment 326370 [details] [diff] [review]
Complete patch v1.2

Double focus stuff would continue in bug 261074. Otherwise this should be ok.
Attachment #326370 - Flags: review?(neil)
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 on attachment 326370 [details] [diff] [review]
Complete patch v1.2

OK, this looks good on Windows XP at least.
Attachment #326370 - Flags: review?(neil) → review+
Ok, fixed the nit.
Attachment #326370 - Attachment is obsolete: true
Attachment #328562 - Flags: superreview?(roc)
Attachment #328562 - Flags: review+
+        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.
(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. 

(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.
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.
Attachment #330095 - Flags: superreview?(roc)
Attachment #328562 - Attachment is obsolete: true
Attachment #328562 - Flags: superreview?(roc)
Attached patch Mochitest for the case (obsolete) — Splinter Review
Roc, would this be ok for a test?
Attachment #330096 - Flags: superreview?(roc)
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?
Attachment #330095 - Flags: review?(Olli.Pettay)
Attachment #330095 - Flags: superreview?(roc) → superreview+
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 on attachment 330096 [details] [diff] [review]
Mochitest for the case

awaiting response
Attachment #330096 - Flags: superreview?(roc)
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?
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.
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 on attachment 330096 [details] [diff] [review]
Mochitest for the case

This gets too complicated. Let's just do this.
Attachment #330096 - Flags: superreview+
Attachment #330096 - Flags: review+
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...
Works ok on Linux. So r=me if that lastFocusedPresContext issue is solved.
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.
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.
Attached patch Patch v1.3 (obsolete) — Splinter Review
Thanks for catching that. Made the pointer strong. Assuming reviews..
Attachment #330803 - Flags: superreview+
Attachment #330803 - Flags: review+
Comment on attachment 330803 [details] [diff] [review]
Patch v1.3

Looks ok to me.
Checked in, taking shelter..
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
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.
I backed this out:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ed6642a02b2d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry about that. I couldn't get a network connection to fix it in timely fashion. Will get back to it.
Bug 447613 was apparently caused by this checkin, and was resolved by the backout in comment 44.
So the testcase didn't actually work and the patch caused another problem. Back to debugging..
Status: REOPENED → ASSIGNED
Attached patch Patch v1.4 (obsolete) — Splinter Review
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?
Attachment #330095 - Attachment is obsolete: true
Attachment #330096 - Attachment is obsolete: true
Attachment #330803 - Attachment is obsolete: true
Attachment #330953 - Flags: superreview?(roc)
Attachment #330095 - Flags: review?(Olli.Pettay)
Attachment #330953 - Flags: superreview?(roc) → superreview+
Attachment #330953 - Flags: review?(neil)
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 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.
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.
Neil, any conclusion?
Comment on attachment 330953 [details] [diff] [review]
Patch v1.4

I crashed this too.
Attachment #330953 - Attachment is obsolete: true
Attachment #330953 - Flags: review?(neil)
Attached file Crash stack (obsolete) —
Smaug, any idea how the patch would cause this? Looks like DocumentViewerImpl is holding a bad reference to mPresContext.
Attached file Another crash, another stack (obsolete) —
Another crash stack. Different place, but again pres context.
I think I finally noticed the problem. gLastFocusedPresContext is a weak pointer, so must not use swap..
And because it's weak, is it actually safe to assign it to a nsCOMPtr?
Ah, indeed swap shouldn't be used.
It is ok to assing to nsCOMPtr
Attached patch Patch v1.5Splinter Review
Fixed patch. Again.
Attachment #331599 - Attachment is obsolete: true
Attachment #331600 - Attachment is obsolete: true
Attachment #331659 - Flags: review?
Attachment #331659 - Flags: review? → review?(Olli.Pettay)
Comment on attachment 331659 [details] [diff] [review]
Patch v1.5

r=me
But did you say you still need to fix the tests?
Attachment #331659 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #331659 - Flags: superreview?(roc)
Attachment #331659 - Flags: superreview?(roc) → superreview+
The patch is now in trunk. I haven't been able to make the test case work reliably yet.
I'm unable to simulate the alert and other cirmustances properly in a mochitest. What shall we do (roc?)?
Try using an actual alert() in the child window and dismissing it by synthesizing a keyboard event via setTimeout in the original window?
Depends on: 449749
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...
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.
QA Contact: ian → events
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.