399852 - Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with position: fixed, focusing and contenteditable

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk builds directly, or after a few reloads (reloads automatically).

Bug 391178 has a similar stacktrace, maybe it will be fixed by the patch in there?
http://crash-stats.mozilla.com/report/index/d6d009fd-7b17-11dc-9113-001a4bd43e5c
0  	nsCSSFrameConstructor::FindFrameWithContent(nsFrameManager*, nsIFrame*, nsIContent*, nsIContent*, nsFindFrameHint*)  	 mozilla/layout/base/nsCSSFrameConstructor.cpp:10857
1 	nsCSSFrameConstructor::FindPrimaryFrameFor(nsFrameManager*, nsIContent*, nsIFrame**, nsFindFrameHint*) 	mozilla/layout/base/nsCSSFrameConstructor.cpp:10968
2 	nsFrameManager::GetPrimaryFrameFor(nsIContent*, int) 	mozilla/layout/base/nsFrameManager.cpp:392
3 	PresShell::GetPrimaryFrameFor(nsIContent*) 	mozilla/layout/base/nsPresShell.cpp:4745
4 	nsFrameSelection::GetFrameForNodeOffset(nsIContent*, int, nsFrameSelection::HINT, int*) 	mozilla/layout/generic/nsSelection.cpp:2671
5 	nsCaret::GetCaretFrameForNodeOffset(nsIContent*, int, nsFrameSelection::HINT, unsigned char, nsIFrame**, int*) 	mozilla/layout/base/nsCaret.cpp:633
6 	nsCaret::DrawAtPositionWithHint(nsIDOMNode*, int, nsFrameSelection::HINT, unsigned char, int) 	mozilla/layout/base/nsCaret.cpp:569
7 	nsCaret::DrawCaret(int) 	mozilla/layout/base/nsCaret.cpp:965
8 	nsCaret::StartBlinking() 	mozilla/layout/base/nsCaret.cpp:536
9 	nsCaret::SetCaretVisible(int) 	mozilla/layout/base/nsCaret.cpp:227
10 	PresShell::SetCaretEnabled(int) 	mozilla/layout/base/nsPresShell.cpp:2652
etc..


It also crashes branch, so I'm marking this security sensitive for now.
Talkback ID: TB36971306E
GetFrameFromLine  [mozilla/layout/generic/nsBlockFrame.cpp, line 6897]
nsBlockFrame::GetFrameForPointUsing  [mozilla/layout/generic/nsBlockFrame.cpp, line 6972]
nsBlockFrame::GetFrameForPoint  [mozilla/layout/generic/nsBlockFrame.cpp, line 7008]
PresShell::HandleEvent  [mozilla/layout/base/nsPresShell.cpp, line 6212]
nsViewManager::HandleEvent  [mozilla/view/src/nsViewManager.cpp, line 2521]
nsViewManager::DispatchEvent  [mozilla/view/src/nsViewManager.cpp, line 2253]
HandleEvent  [mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 1319]
nsWindow::DispatchFocus  [mozilla/widget/src/windows/nsWindow.cpp, line 6515]
nsWindow::ProcessMessage  [mozilla/widget/src/windows/nsWindow.cpp, line 5034]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

The original testcase doesn't seem to crash anymore, but this one still does after 500ms.

Comment 2

[Daniel Holbert [:dholbert]]

Tried latest-trunk on Linux and Windows.  Crashes in Windows, not in Linux.

**Linux build**:  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007110504 Minefield/3.0a9pre

**Windows build**:  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110505 Minefield/3.0a9pre

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Taking bug. It looks like what is happening is that when the setAttribute('overflow:scroll; direction: rtl;') gets actioned, frames get deleted and recreated. The frame for the contenteditable <li> gets deleted, but when its surrounding <iframe> frame gets deleted, it calls nsWindow::Show(PR_FALSE) to hide the frame being deleted. This calls the win32 ShowWindow() function, which appears to be calling back into our nsWindow::WindowProc() with a focus event, which is trying to get a caret for the <li> nsIContent (since its contenteditable). This searches the frame tree, and is finding a reference in the prev-sibling of the <li> which has been freed and it's memory cleared, hence the crash. So I think what's happening is that the frame deletion isn't zeroing the next-sibling pointer of the frame-being-deleted's prev-sibling.

Comment 4

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

This is another case of focus events firing during frame destruction biting us in the bum. We really need kill that off at the source somehow.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, so we need to prevent native focus events from firing while we're in frame destruction.

So we can add to windows/nsWindow::ProcessMessage in the WM_SETFOCUS handler, code that checks to see if we're in curently processing frame destruction, and if so, we can defer the focus by posting another focus event using ::PostMessage().

This could again cause problems as perhaps Windows would think our window has focus when we do not. We'd also need to ensure that if we received a real WM_SETFOCUS while not in frame destruction before our deferred WM_SETFOCUS event came in, we'd have to prevent the deferred focus from being actioned, as it'd been overridden. I guess we'd need to do something similar for WM_KILLFOCUS too.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Ok, so we need to prevent native focus events from firing while we're in frame
> destruction.

Awesome... If we can get that implemented, that should fix (at least) two P2 1.9-blocking bugs: bug 377960 and bug 394818.

Comment 7

[Daniel Holbert [:dholbert]]

Can we elevate the priority of this to P2, as there are P2 crashes that depend on it?

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Ok, so we need to prevent native focus events from firing while we're in frame
> destruction.
> 
> So we can add to windows/nsWindow::ProcessMessage ...

FWIW: We'll need to implement something like this for Cocoa widgets, too, to fix bug 377960 and bug 394818.  (not necessarily as part of this bug's fix, though, as this one is Windows-only.)

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #5)
> Ok, so we need to prevent native focus events from firing while we're in frame
> destruction.

I've tested this approach, and it does stop the crash. Just need to tidy up the rough edges...

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - defer focus events while in DeletingFrameSubtree()

* Change nsBaseWidget to have a global flag to defer focus events.
* Change DeletingFrameSubtree() in nsCSSFrameConstructor.cpp to set the flag to defer focus events while its deleting frame subtree.
* Change nsWindow::ProcessMessage() to defer focus events when focus defer flag is on. If a non-deferred focus event is handled while deferred events are still pending, the deferred events are cancelled so as to not stomp the new focus.
* Patch passes Mochitests on WinXP.

Comment 11

[Ere Maijala (slow)]

Comment on attachment 291152 [details] [diff] [review]
Patch v1 - defer focus events while in DeletingFrameSubtree()

Minor things in the patch:

+    // Tells the widget not to defer focus events until later.

Make it "Tell" (the prevalent style) and remove "not" (right?).

+class FocusEvent {
+class FocusEventComparator {
+void nsIWidget::SetDeferFocusEvents(PRBool aSupress) {
+PRBool nsIWidget::GetDeferFocusEvents() {

The brace should always be alone on the next line. 

The patch seems to work, but is there no sensible way to do this in XP code like in nsWebShellWindow? Make it queue the focus events and fire them when deferring ends? And all in the same order they were received?

One thing I'm worried is that this could break focus/activation by messing with the message order. The gotfocus/activate/lostfocus/deactivate stuff is so delicate..

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #11)

> The patch seems to work, but is there no sensible way to do this in XP code
> like in nsWebShellWindow? Make it queue the focus events and fire them when
> deferring ends? And all in the same order they were received?

Hmm, we could do the buffering in nsWebShellWindow::HandleEvent(nsGUIEvent *aEvent), and then stash the aEvents away while we're deferring. I'm a bit worried about what would happen if aEvent->widget went away while we were deferring focus. Is there a way I can check if the widget is still alive, for example is there a registry of widgets somewhere?

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - platform independent version

This patch follows a similar vein to patch v1, but is platform independent. It fixes a bunch of bugs similar to this one as well ().

* Intercept focus/blur events in nsViewManager::DispatchEvent(), defer them if necessary. (I tried intercepting in nsWebShellWindow, but it didn't seem to be called for all events that came through view manager).
* Event deferring is implemented in nsBaseWidget, as pure virtual functions in nsIWidget.
* Event deferring is used in nsCSSFrameConstructor.cpp via the nsAutoDeferFocusEvents class, which turns on deferring in its constructor, and disables deferring in its destructor.
* Focus/blur events are deferred in nsCSSFrameConstructor::ProcessOneRestyle(). Patch v1 deferred only in ::DeletingFrameSubtree(). We need to be less specific when deferring events in order to catch both the MacOS and Windows bugs.
* Deferred events are executed in an nsRunnable in the event thread.
* I had to add a "size of nativeMsg" field to nsGuiEvent, as I need to make a copy of that field because it's sometimes stored on the stack, and I need to know how big to make the copy in a platform indepedent way.

Comment 14

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

Do we need the nativeMsg data from the focus event? It's not really clear that we can safely copy the nativeMsg data and use the copy later. It might contain references to things that die.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #14)
> Do we need the nativeMsg data from the focus event? It's not really clear that
> we can safely copy the nativeMsg data and use the copy later. It might contain
> references to things that die.
> 

On Windows nativeMsg is an nsPluginEvent, which has lParam, wParam, and an event id, so it's all values, so copy safe: http://mxr.mozilla.org/seamonkey/source/modules/plugin/base/public/nsplugindefs.h#322

On Mac it's all values, so copyable:
http://developer.apple.com/DOCUMENTATION/Carbon/Reference/Event_Manager/Reference/reference.html#//apple_ref/doc/c_ref/EventRecord

On Linux it's a GdkEventKey. This contains some pointers which may cause problems (gchar* and GdkWindow*)
http://library.gnome.org/devel/gdk/unstable/gdk-Event-Structures.html#GdkEventKey

We don't need this fix for Linux, so perhaps we should #def it to only work on Windows and Mac? Actually this code should never be run on Linux, because the focus events are asynchronous, so no focus events should need to be deferred on Linux. Maybe we should #def it out on Linux anyway to be sure?

Comment 16

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

If we just don't copy the nativeMsg data, leaving it null in the deferred event, does that cause problems?

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #16)
> If we just don't copy the nativeMsg data, leaving it null in the deferred
> event, does that cause problems?
> 

Ok, I've tried this, the test cases for this bug and blocked bugs don't crash and  mochitests pass on both mac and windows. So it's probably safe enough to not bother about the nativeMsg.

Comment 18

[Ere Maijala (slow)]

Comment on attachment 292537 [details] [diff] [review]
Patch v2 - platform independent version

I like it. r+ with the following minor things addressed:

1. Please change the name of FlushDeferredEvents to DispatchDeferredEvents ("flush" could be interpreted in a wrong way). 

2. The test here isn't really needed:

+            if (widget->HasDeferredEvents()) {
+              widget->ClearDeferredEvents();
+            }

Just make the call unconditional and we'll save a few cycles.

3. A typo in "received" here:

+            // We've receieved a non-deferred focus/blur, which overrides

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - With Ere's changes

This is patch v2 with Eli's requested changes. sr?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 293004 [details] [diff] [review]
Patch v3 - With Ere's changes

Sorry! s/Eli/Ere/

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 293004 [details] [diff] [review]
Patch v3 - With Ere's changes

Roc, I think this is more your territory than mine. If you're unable to sr, let me know and I can try, but it'll take me a while to get through this as I know very little about most of this code.

Comment 22

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

Chris, could you post a new patch with the nativemsg copying removed? That would simplify the review significantly. Thanks...

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch without nativeMsg

(In reply to comment #22)
> Chris, could you post a new patch with the nativemsg copying removed? That
> would simplify the review significantly. Thanks...
> 

Ok, here it is, patch without nativeMsg copying.

Comment 24

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

+ 
+     // Tell the widget whether to defer focus events until later.

+     virtual void ClearDeferredEvents()=0;
+ 
+

Remove extra lines

I wonder if it would be better to fire the deferred events synchronously from SetDeferFocusEvents(PR_FALSE) instead of off an XPCOM event. That would be less disruptive and probably remove the need for HasDeferredEvents/ClearDeferredEvents.

+    while (!gDeferredEvents->IsEmpty()) {
+      nsDeferredEvent *event = gDeferredEvents->ElementAt(0);
+      gDeferredEvents->RemoveElementAt(0);
+      nsEventStatus status;
+      event->widget->DispatchEvent(event, status);
+      delete event;
+    }

RemoveElementAt(0) is O(N) in the number of events, so it would be better to avoid using it. nsTObserverArray is probably the way to go here:
http://mxr.mozilla.org/seamonkey/source/xpcom/glue/nsTObserverArray.h

You probably should put nsAutoDeferFocusEvents in nsIWidget and have it just take an nsIWidget* parameter in the constructor.

Comment 25

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

Stephen, this may affect your focus patch(es).

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Yes, it probably will (in fact it may help).  Thanks for letting me
know.

I'll try this patch (its latest version) and see what happens.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I wonder if it would be better to fire the deferred events synchronously

That would work if we're sure things are always safe when we stop deferring.  A few issues I do have with the patch:

1)  Using that auto helper on a widget that's already deferring events will execute them too early.  Should this be a counter instead of a boolean?
2)  Are other frame destruction scenarios (e.g. removing a node from the DOM) not affected?  Why only do this in ProcessOneRestyle?

It would make more sense to me to suspend/reenable focus events every time we start/stop a frame constructor outermost update or something.

Comment 28

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

> 1)  Using that auto helper on a widget that's already deferring events will
> execute them too early.  Should this be a counter instead of a boolean?

We could just have an assertion that these aren't nested.

> It would make more sense to me to suspend/reenable focus events every time we
> start/stop a frame constructor outermost update or something.

Yeah.

> That would work if we're sure things are always safe when we stop deferring.

Well, that's the question and I'm not sure of the answer. At least the callers are prepared to deal with arbitrary frame construction/destruction. Not sure whether the callers are prepared to deal with arbitrary presshell destruction or script execution.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> We could just have an assertion that these aren't nested.

If we can assert that, yes.

Note that I suspect frame construction can in fact nest (e.g. constructing a subdocument frame constructs the frame tree inside the subdocument, right?).  So if we do this around frame construction in general, I don't think we can assert such a thing.

I don't think that callers of ProcessPendingRestyles are able to deal with arbitrary script execution, necessarily, though that should be simple to check.  Callers of ContentRemoved would also be a problem, right?

If this runs script, ideally, we'd do this the way we do it for script execution and xbl constructors: at the end of the document update.  That's guaranteed safe.  With focus, though, we could run into fun issues, as usual...

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #27)
> It would make more sense to me to suspend/reenable focus events every time we
> start/stop a frame constructor outermost update or something.

What if we enabled deferred-focus in nsCSSFrameConstructor::BeginUpdate(), and disable it in nsCSSFrameConstructor::EndUpdate() when mUpdateCount==1. By using mUpdateCount we handle the nesting problem, and this should handle other cases as well as the ProcessOneRestyle() one. Would that be enough?

Comment 31

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

I think the possibility of running arbitrary script during ContentRemoved kills that idea. Forget it and let's carry on...

Why do you need nsBaseWidget::FlushDeferredEvents? You could post the XPCOM event when you add the first deferred event to the array.

Why do you need HasDeferredEvents? nsViewManager could just call ClearDeferredEvents unconditionally.

You could probably simplify things by making the deferred events array an array of nsDeferredEvent instead of an array of pointers. It would use a little more memory but only for a short time. Instead of calling gDeferredEvents->Clear() when you empty it, just delete gDeferredEvents and reset it to null.

This is a rather risky change but I think it's the right direction.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> What if we enabled deferred-focus in nsCSSFrameConstructor::BeginUpdate()

That's what I suggested in comment 27, yes.  What I suggest in comment 29 at the end is even better.

Doing some focus stuff sync and some async means focus can end up in the wrong place, no?  Or do we handle that?

Comment 33

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

We handle that by clearing the deferred focus event list when a non-deferred focus event is processed by the viewmanager.

Uh, except that it looks like the first deferred focus event to be re-fired will clear the rest. Oops.

I guess we need to record in the event whether this is a deferred event being replayed, and we only clear the deferred events when we receive a focus event that is not a replay. Ugh, that sounds horrible and fragile.

Maybe the thing to do here is instead of replaying events, just record that we suppressed a focus change, and then when we're ready we "reboot focus" by checking what has changed and synthesizing a focus change to whatever currently has focus, including both LOSTFOCUS and GOTFOCUS events. In fact, maybe all native focus events should be handled that way...

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #33)
> We handle that by clearing the deferred focus event list when a non-deferred
> focus event is processed by the viewmanager.
> 
> Uh, except that it looks like the first deferred focus event to be re-fired
> will clear the rest. Oops.

Yeah, I just discovered that, and fixed it, but found that we one of the deferred focus events later in the queue was focusing a destroyed frame, and we were crashing. So I'm now doubtful this approach is going to work.

> Maybe the thing to do here is instead of replaying events, just record that we
> suppressed a focus change, and then when we're ready we "reboot focus" by
> checking what has changed and synthesizing a focus change to whatever currently
> has focus, including both LOSTFOCUS and GOTFOCUS events. In fact, maybe all
> native focus events should be handled that way...
> 

Ok, here's my understanding of your proposed solution:

* We keep track of what View has focus.
* We supress focus changes while messing with the frame tree, while still tracking what View the system thinks is focused, and what View was focused when we started to supress focus events.
* When we turn off focus-supression, if focus has changed, we send LOST_FOCUS to the losing-focus View, and GOT_FOCUS to the receiving-focus View.

Comment 35

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

Yeah.

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5 - "reboot" focus

* We keep track of what View has focus.
* We supress focus changes while messing with the frame tree, while still tracking what View the system thinks is focused, and what View was focused when we started to supress focus events.
* When we turn off focus-supression, if focus has changed, we send LOST_FOCUS to the widget of the losing-focus View, and GOT_FOCUS to the widget of the receiving-focus View.

Comment 37

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

+// Declared in nsViewManager.cpp
+extern nsView *gCurrentFocusView;
+extern nsView *gFocusedViewBeforeSuppression;

Put these in nsViewManager.h, as static fields of the nsViewManager class.

+          nsGUIEvent event(PR_TRUE, NS_LOSTFOCUS, widget);
+          widget->DispatchEvent(&event, status);

This could focus things. So after this, we need to check whether we should still fire GOTFOCUS on gCurrentFocusView.

+      // Send NS_GOTFOCUS to the widget that we think should be focused.
+      if (gCurrentFocusView) {
+        widget = gCurrentFocusView->GetWidget();
+        if (widget) {
+          nsGUIEvent event(PR_TRUE, NS_LOSTFOCUS, widget);

NS_GOTFOCUS?

+        if (nsViewManager::GetSuppressFocusEvents()) {
+          break;
+        }

Don't need braces around a single-line 'break'

+    case NS_GOTFOCUS:
+      {
+        gCurrentFocusView = nsView::GetViewFor(aEvent->widget);
+        if (nsViewManager::GetSuppressFocusEvents()) {
+          break;
+        }
+
+      }
+    case NS_LOSTFOCUS:
+      {
+        if (nsViewManager::GetSuppressFocusEvents()) {
+          break;
+        }
+      }

These fall-throughs are insidious. For example we're checking GetSuppressFocusEvents twice. Better to find a way to avoid falling through.

+      static_cast<nsViewManager*>(mViewManager.get())->SetSuppressFocusEvents(PR_TRUE);

Add SetSuppressFocusEvents to nsIViewManager so you don't need the cast or .get().

Also, assert that events are not current suppressed or handle reentry by saving the current suppress state and restoring it on exit.

We should do this in document update start/end like Boris suggested in comment #29/#32.

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v6 - reboot focus with requested changes

As patch v5 with changes as requested in comment #37.

Comment 39

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

+  sSuppressFocusEvents = aSuppress;

Move this up to the previous else-if block.

+  virtual void SetSuppressFocusEvents(PRBool aSuppress)=0;
+  virtual PRBool GetSuppressFocusEvents()=0;

Probably should document that SetSuppressFocusEvents(PR_FALSE) can run arbitrary code and destroy the view manager, so the caller needs to be prepared for this.

+  // Prevent the event handler from processing focus events while
+  // we change the frame tree. Fix for bug 399852.
+  nsAutoFocusEventSuppressor dontSpazMyFrameTree(mPresShell);

We don't need this in ProcessOneRestyle because the only call to it is guarded by BeginUpdate/EndUpdate, so we don't need nsAutoFocusEventSuppressor at all.

But actually I think the BeginUpdate/EndUpdate calls that Boris wants to hook are the ones from nsIDocumentObserver, implemented by PresShell. In fact I think you should probably hook both.

"UnSuppress" should be "Unsupress" because "Un" isn't a word :-)

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v7

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 296073 [details] [diff] [review]
Patch v7

(In reply to comment #39)
> We don't need this in ProcessOneRestyle because the only call to it is guarded
> by BeginUpdate/EndUpdate, so we don't need nsAutoFocusEventSuppressor at all.
> 
> But actually I think the BeginUpdate/EndUpdate calls that Boris wants to hook
> are the ones from nsIDocumentObserver, implemented by PresShell. In fact I
> think you should probably hook both.

It looks like these ones in PresShell already defer to the frame constructor anyway:
http://mxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3020

Comment 42

[Martijn Wargers (dead)]

Checking in view/public/nsIViewManager.h;
/cvsroot/mozilla/view/public/nsIViewManager.h,v  <--  nsIViewManager.h
new revision: 3.103; previous revision: 3.102
done
Checking in view/src/nsView.cpp;
/cvsroot/mozilla/view/src/nsView.cpp,v  <--  nsView.cpp
new revision: 3.253; previous revision: 3.252
done
Checking in view/src/nsViewManager.cpp;
/cvsroot/mozilla/view/src/nsViewManager.cpp,v  <--  nsViewManager.cpp
new revision: 3.463; previous revision: 3.462
done
Checking in view/src/nsViewManager.h;
/cvsroot/mozilla/view/src/nsViewManager.h,v  <--  nsViewManager.h
new revision: 3.175; previous revision: 3.174
done
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstru
ctor.cpp
new revision: 1.1449; previous revision: 1.1448
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstruct
or.h
new revision: 1.252; previous revision: 1.251
done

Checked into trunk.

Comment 43

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

This caused numerous test failures so I backed it out for Chris Double (the sheriff).

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

The mochitests which are failing are:

/tests/content/events/test/test_bug238987.html 
/tests/content/html/content/test/test_bug388558.html
/tests/content/html/document/test/test_bug391777.html
/tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html

Comment 45

[Jeff Walden [:Waldo]]

Something's bogus about at least some of those; the 391777 one is a pure-DOM test, for example.  I haven't looked at any of the others.

Comment 46

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ho humm, it looks like 391777 is failing on my WinXP box even without my patch in... But the others only fail with my patch in, so there is something wrong with my patch...

Comment 47

[Martijn Wargers (dead)]

Sorry, I watched the tree, I thought it sticked.

Comment 48

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v8 - Patch v7 with bug fixed

The problem with the old patch is here:

+void nsViewManager::SetSuppressFocusEvents(PRBool aSuppress)
+{
+  if (sSuppressFocusEvents && !aSuppress) {
+    // We're turning off suppression, synthesize LOSTFOCUS/GOTFOCUS.
+    if (GetCurrentlyFocusedView() != GetViewFocusedBeforeSuppression()) {
+
+      // Turn off suppresion before we send blur/focus events.
+      sSuppressFocusEvents = aSuppress;
+

So I'm only turning off focus suppression if (GetCurrentlyFocusedView() != GetViewFocusedBeforeSuppression()), which isn't always the case. Funnily enough this bug doesn't show up on Vista, but it does on WinXP.

This new patch is the same as the previous, except that the above is changed to:

+void nsViewManager::SetSuppressFocusEvents(PRBool aSuppress)
+{
+  if (sSuppressFocusEvents && !aSuppress) {
+    // We're turning off suppression, synthesize LOSTFOCUS/GOTFOCUS.
+    // Turn off suppresion before we send blur/focus events.
+    sSuppressFocusEvents = aSuppress;
+    if (GetCurrentlyFocusedView() != GetViewFocusedBeforeSuppression()) {
+

So we're ready to check in again...

Comment 49

[Daniel Holbert [:dholbert]]

"Patch with bug fixed" (attachment 296897 [details] [diff] [review]) checked into trunk.  Closing.

Checking in view/public/nsIViewManager.h;
/cvsroot/mozilla/view/public/nsIViewManager.h,v  <--  nsIViewManager.h
new revision: 3.105; previous revision: 3.104
done
Checking in view/src/nsView.cpp;
/cvsroot/mozilla/view/src/nsView.cpp,v  <--  nsView.cpp
new revision: 3.255; previous revision: 3.254
done
Checking in view/src/nsViewManager.cpp;
/cvsroot/mozilla/view/src/nsViewManager.cpp,v  <--  nsViewManager.cpp
new revision: 3.465; previous revision: 3.464
done
Checking in view/src/nsViewManager.h;
/cvsroot/mozilla/view/src/nsViewManager.h,v  <--  nsViewManager.h
new revision: 3.177; previous revision: 3.176
done
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1452; previous revision: 1.1451
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision: 1.254; previous revision: 1.253
done

Comment 50

[Daniel Holbert [:dholbert]]

Checkin from comment 49 broke these mochitests on mac:

*** 43148 ERROR FAIL | Checking form username | got "", expected "tempuser1" | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html
*** 43149 ERROR FAIL | Checking form password | got "", expected "temppass1" | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html
*** 43185 ERROR FAIL | Checking form username | got "testuser2", expected "zzzuser4" | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html
*** 43194 ERROR FAIL | Checking form username | got "zzzuser4", expected "testuser2" | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html

Brief Log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1200442800.1200444047.26511.gz

Full Log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1200442800.1200444047.26511.gz&fulltext=1

Backing out and reopening.

Comment 51

[Daniel Holbert [:dholbert]]

The Linux unittest box just finished its cycle, and it seems that the last patch broke the same mochitests on Linux as well:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1200443257.1200447179.4904.gz

(plus some other mochitests timed out, though I'm not sure if that's related)

Comment 52

[Daniel Holbert [:dholbert]]

(In reply to comment #50)
> Checkin from comment 49 broke these mochitests on mac:
(In reply to comment #51)
> The Linux unittest box just finished its cycle, and it seems that the last
> patch broke the same mochitests on Linux as well:

... but interestingly, the Windows test box seems seems to have been fine with the patch.

Comment 53

[Chris Pearce [:cpearce (Not reading bugmail)]]

Looks like a real focus event is coming in while focus suppression is on, presumably that's the cause of the problem. I don't see a lost-focus event being sent though. The mochitest is calling that a lot...

Maybe this "reboot focus" approach won't work on the Mac/Linux.

Comment 54

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

Why not? Is the problem that the test is .focus()ing an element while focus suppression is on (how can that happen?) and then synchronously trying to send key events or something, which are going to the wrong place since we deferred the focus change?

Comment 55

[Chris Pearce [:cpearce (Not reading bugmail)]]

It looks like a JS timer is run which is calling nsDOMWindowUtils::SendKeyEvent() which causes the JS set_selectedIndex() to run on a combobox, which causes a reflow (drop down box opening?) which calls nsViewManager::SetViewVisibility, calling nsCocoaWindow::Show(), which is dispatching a focus, which is being dropped, and so some subsequent events go to the wrong target. So the question is therefore, how can a JS timer be being run while focus suppression is still on...

Comment 56

[Johnny Stenback (:jst)]

JS timeouts can run any time JS is enabled and we process events. We do some work to try to avoid running them in a window while modal dialogs are open, but other than that they can run pretty much any time.

Comment 57

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

Sure, but we shouldn't be processing events inside a Begin/EndUpdate.

Comment 58

[Chris Pearce [:cpearce (Not reading bugmail)]]

Here's the stack trace of where the event is dropped:

#0	0x1553c3e2 in nsViewManager::DispatchEvent at nsViewManager.cpp:1226
#1	0x155321a9 in HandleEvent at nsView.cpp:168
#2	0x33bccb54 in nsChildView::DispatchEvent at nsChildView.mm:1371
#3	0x33bccd91 in -[ChildView sendFocusEvent:] at nsChildView.mm:2129
#4	0x33bcac72 in -[ChildView becomeFirstResponder] at nsChildView.mm:4181
#5	0x932d74a3 in -[NSWindow makeFirstResponder:]
#6	0x933087ec in -[NSWindow _selectFirstKeyView]
#7	0x932ae361 in -[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:]
#8	0x932ade7e in -[NSWindow orderWindow:relativeTo:]
#9	0x93386628 in -[NSWindow orderFront:]
#10	0x33bc409c in nsCocoaWindow::Show at nsCocoaWindow.mm:576
#11	0x15534430 in nsView::SetVisibility at nsView.cpp:463
#12	0x1553901b in nsViewManager::SetViewVisibility at nsViewManager.cpp:1736
#13	0x15312743 in nsMenuPopupFrame::AdjustView at nsMenuPopupFrame.cpp:328
#14	0x1531a685 in nsPopupSetFrame::DoLayout at nsPopupSetFrame.cpp:213
#15	0x152eee50 in nsIFrame::Layout at nsBox.cpp:561
#16	0x152f5826 in nsSprocketLayout::Layout at nsSprocketLayout.cpp:523
#17	0x152f1d9a in nsBoxFrame::DoLayout at nsBoxFrame.cpp:945
#18	0x152eee50 in nsIFrame::Layout at nsBox.cpp:561
#19	0x152f6c0b in nsStackLayout::Layout at nsStackLayout.cpp:292
#20	0x152f1d9a in nsBoxFrame::DoLayout at nsBoxFrame.cpp:945
#21	0x152eee50 in nsIFrame::Layout at nsBox.cpp:561
#22	0x152f00b4 in nsBoxFrame::Reflow at nsBoxFrame.cpp:756
#23	0x152ec609 in nsRootBoxFrame::Reflow at nsRootBoxFrame.cpp:236
#24	0x1518dc9c in nsContainerFrame::ReflowChild at nsContainerFrame.cpp:755
#25	0x152110c9 in ViewportFrame::Reflow at nsViewportFrame.cpp:286
#26	0x1515efcc in PresShell::DoReflow at nsPresShell.cpp:6181
#27	0x15164ce0 in PresShell::ProcessReflowCommands at nsPresShell.cpp:6286
#28	0x15164fbb in PresShell::DoFlushPendingNotifications at nsPresShell.cpp:4506
#29	0x151651c2 in PresShell::FlushPendingNotifications at nsPresShell.cpp:4444
#30	0x15370c39 in nsDocument::FlushPendingNotifications at nsDocument.cpp:4907
#31	0x15302107 in nsBoxObject::GetPresShell at nsBoxObject.cpp:155
#32	0x1530213c in nsBoxObject::GetFrame at nsBoxObject.cpp:127
#33	0x15302308 in nsBoxObject::GetOffsetRect at nsBoxObject.cpp:171
#34	0x1530254a in nsBoxObject::GetHeight at nsBoxObject.cpp:269
#35	0x01380aac in NS_InvokeByIndex_P at xptcinvoke_unixish_x86.cpp:179
#36	0x346ae6c6 in XPCWrappedNative::CallMethod at xpcwrappednative.cpp:2344
#37	0x346eae81 in XPCWrappedNative::GetAttribute at xpcwrappednativejsops.cpp:2195
#38	0x346b6483 in XPC_WN_GetterSetter at xpcwrappednativejsops.cpp:1512
#39	0x01063746 in js_Invoke at jsinterp.c:1023
#40	0x01063a94 in js_InternalInvoke at jsinterp.c:1096
#41	0x01063cdf in js_InternalGetOrSet at jsinterp.c:1159
#42	0x010933c3 in js_NativeGet at jsobj.c:3523
#43	0x01093e7e in js_GetProperty at jsobj.c:3666
#44	0x01073638 in js_Interpret at jsinterp.c:3508
#45	0x010637d1 in js_Invoke at jsinterp.c:1040
#46	0x01063a94 in js_InternalInvoke at jsinterp.c:1096
#47	0x01063cdf in js_InternalGetOrSet at jsinterp.c:1159
#48	0x01094211 in js_SetProperty at jsobj.c:3765
#49	0x01073f86 in js_Interpret at jsinterp.c:3589
#50	0x010637d1 in js_Invoke at jsinterp.c:1040
#51	0x346a9225 in nsXPCWrappedJSClass::CallMethod at xpcwrappedjsclass.cpp:1457
#52	0x346a1833 in nsXPCWrappedJS::CallMethod at xpcwrappedjs.cpp:567
#53	0x01380d66 in PrepareAndDispatch at xptcstubs_unixish_x86.cpp:93
#54	0x01381054 in nsXPTCStubBase::Stub11 at xptcstubsdef.inc:13
#55	0x32e3eed1 in nsAutoCompleteController::HandleKeyNavigation at nsAutoCompleteController.cpp:443
#56	0x328ff552 in nsFormFillController::KeyPress at nsFormFillController.cpp:690
#57	0x153d89f9 in DispatchToInterface at nsEventListenerManager.cpp:184
#58	0x153dbf6b in nsEventListenerManager::HandleEvent at nsEventListenerManager.cpp:1180
#59	0x15400e2b in nsEventTargetChainItem::HandleEvent at nsEventDispatcher.cpp:206
#60	0x15400ffd in nsEventTargetChainItem::HandleEventTargetChain at nsEventDispatcher.cpp:264
#61	0x154018ae in nsEventDispatcher::Dispatch at nsEventDispatcher.cpp:479
#62	0x151618af in PresShell::HandleEventInternal at nsPresShell.cpp:5822
#63	0x15163ee5 in PresShell::HandleEvent at nsPresShell.cpp:5622
#64	0x1553bbd4 in nsViewManager::HandleEvent at nsViewManager.cpp:1383
#65	0x1553cb12 in nsViewManager::DispatchEvent at nsViewManager.cpp:1339
#66	0x155321a9 in HandleEvent at nsView.cpp:168
#67	0x33bccb54 in nsChildView::DispatchEvent at nsChildView.mm:1371
#68	0x155418bb in nsDOMWindowUtils::SendKeyEvent at nsDOMWindowUtils.cpp:276
#69	0x01380aac in NS_InvokeByIndex_P at xptcinvoke_unixish_x86.cpp:179
#70	0x346ae6c6 in XPCWrappedNative::CallMethod at xpcwrappednative.cpp:2344
#71	0x346b622a in XPC_WN_CallMethod at xpcwrappednativejsops.cpp:1480
#72	0x01063746 in js_Invoke at jsinterp.c:1023
#73	0x0107579a in js_Interpret at jsinterp.c:3858
#74	0x010637d1 in js_Invoke at jsinterp.c:1040
#75	0x01063a94 in js_InternalInvoke at jsinterp.c:1096
#76	0x010209dd in JS_CallFunctionValue at jsapi.c:4938
#77	0x1554dd92 in nsJSContext::CallEventHandler at nsJSEnvironment.cpp:1967
#78	0x15574eac in nsGlobalWindow::RunTimeout at nsGlobalWindow.cpp:7374
#79	0x155753de in nsGlobalWindow::TimerCallback at nsGlobalWindow.cpp:7705
#80	0x01372cc0 in nsTimerImpl::Fire at nsTimerImpl.cpp:400
#81	0x01372edf in nsTimerEvent::Run at nsTimerImpl.cpp:487
#82	0x0136ec07 in nsThread::ProcessNextEvent at nsThread.cpp:510
#83	0x01314d7f in NS_ProcessPendingEvents_P at nsThreadUtils.cpp:180
#84	0x33be0863 in nsBaseAppShell::NativeEventCallback at nsBaseAppShell.cpp:112
#85	0x33bbf9a9 in nsAppShell::ProcessGeckoEvents at nsAppShell.mm:294
#86	0x9082cf1a in CFRunLoopRunSpecific
#87	0x9082ca56 in CFRunLoopRunInMode
#88	0x92de7878 in RunCurrentEventLoopInMode
#89	0x92de6f82 in ReceiveNextEventCommon
#90	0x92de6dd9 in BlockUntilNextEventMatchingListInMode
#91	0x9328d485 in _DPSNextEvent
#92	0x9328d076 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
#93	0x93286dfb in -[NSApplication run]
#94	0x33bbed5c in nsAppShell::Run at nsAppShell.mm:565
#95	0x32ec134d in nsAppStartup::Run at nsAppStartup.cpp:181
#96	0x00210cea in XRE_main at nsAppRunner.cpp:3207
#97	0x00002798 in main at nsBrowserApp.cpp:158

BeginUpdate() is called as part of the reflow caused by the call to  GetFrame(PR_TRUE) in (#33) nsBoxObject::GetOffsetRect(). So the patch is doing what it's supposed to be doing - blocking a focus while we're in the middle of a reflow... Does that GetFrame(PR_TRUE) need to be called with PR_TRUE?

Comment 59

[Chris Pearce [:cpearce (Not reading bugmail)]]

Maybe we need to move where we start/stop suppressing focus events? If the scope of when things are suppressed was narrower we may avoid problems like this...

Comment 60

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

This should be OK, when reflow finishes we should end the update and set focus to the right place. Then subsequent stuff should work fine. So what's going wrong?

Comment 61

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #58)

What happens at the top of this stack (a focus event getting sent to a
child widget when its parent (top-level) widget becomes active,
outside of the context of a call to nsIWidget::SetFocus()) is
something that no longer happens (on OS X) as of my patch for bug
403232.

Since you're testing on OS X, Chris, you might want to try again with
today's nightly (or with code pulled since my patch landed, just
before 9am PST 2008-01-17).

Comment 62

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #61)
> (In reply to comment #58)
> 
> What happens at the top of this stack (a focus event getting sent to a
> child widget when its parent (top-level) widget becomes active,
> outside of the context of a call to nsIWidget::SetFocus()) is
> something that no longer happens (on OS X) as of my patch for bug
> 403232.
> 
> Since you're testing on OS X, Chris, you might want to try again with
> today's nightly (or with code pulled since my patch landed, just
> before 9am PST 2008-01-17).
> 

Ok, with the lasted Mac build and Patch V8, we now pass the mochitests. I guess this is still bad on Linux though...

Comment 63

[Chris Pearce [:cpearce (Not reading bugmail)]]

The mochitest does still fail on Linux with the patch, but passes on Windows and Mac.

Comment 64

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

Does Linux fail because nsDOMWindowUtils::SendKeyEvent (which is used in 
mochitests) creates an artificial event at unusual time? I think normal 
user-initiated events aren't created when there is a script running.

Comment 65

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is madness:

[09:33]	<dholbert>	{#developers} cpearce: that mochitest still fails on my linux box after your latest posted patch
[09:34]	<dholbert>	{#developers} cpearce: Cool, no prob. Incidentally, that particular test didn't fail when I ran the mochitest suite, but it did fail when I ran it individually
[09:34]	<dholbert>	{#developers} cpearce: so maybe it's sporadic... Idunno

I get the same result; the mochitests only fail when I run them all (or at least the toolkit mochitests. They don't fail when run individually. I get a popup window not closing in content/tests/widgets/window_tooltip.xul.

Comment 66

[Daniel Holbert [:dholbert]]

(In reply to comment #65)
> This is madness:
> ...
> I get the same result; the mochitests only fail when I run them all (or at
> least the toolkit mochitests. They don't fail when run individually.

Actually, it's more madness than you thought:
  I actually was saying that I got the *opposite* behavior of what you're describing, at least for test tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html (the one that broke on the last checkin)

That particular test worked on a full mochitest run, but failed when I ran it individually.

Comment 67

[Martijn Wargers (dead)]

(In reply to comment #65)
> [09:33] <dholbert>      {#developers} cpearce: that mochitest still fails on 
> I get the same result; the mochitests only fail when I run them all (or at
> least the toolkit mochitests. They don't fail when run individually. I get a
> popup window not closing in content/tests/widgets/window_tooltip.xul.

This reminds me of the mochitests in bug 280959, see bug 280959, comment 71 and further. Maybe you're dealing with the same kind of bug here?

Comment 68

[Chris Pearce [:cpearce (Not reading bugmail)]]

I can stop the tests failing if I change them so that they're not doing things in onfocus events, and instead doing their tests in a setTimeout(_,100) instead.

Comment 69

[Chris Pearce [:cpearce (Not reading bugmail)]]

I occasionally see this error message when the mochitests lock up:

JavaScript error: , line 0: uncaught exception: Permission denied to get property XULElement.firstChild

Does this mean anything to anyone?

Comment 70

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

jst would know

Comment 71

[Mike Schroepfer]

Moving TM to b4

Comment 72

[Johnny Stenback (:jst)]

(In reply to comment #69)
> I occasionally see this error message when the mochitests lock up:
> 
> JavaScript error: , line 0: uncaught exception: Permission denied to get
> property XULElement.firstChild
> 
> Does this mean anything to anyone?
> 

Hmm, assuming that the error is generated from the onfocus handler it would seem to me that the onfocus handler either is comiled with the wrong principals (unlikely, but not impossible), or the onfocus handler fires after a page has been unloaded and it runs on the context of the new page, or it fires on a node that was moved from one document to another and the nodes principal changed but the onfocus event handler didn't get recompiled (i.e. it was added with addEventListener() or what not, and this would need to be chrome, or code with elevated privileges doing the actual move, and the right move to trigger something like this).

It's really sort of hard to say for sure w/o seeing this in a debugger.

Comment 73

[Johnny Stenback (:jst)]

A good place to break would be in NS_ScriptErrorReporter(), but be aware that it's called fairly often, so you'd need to conditionally break on the error message you're interested in or something like that.

Comment 74

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, the call stack from the first one of these is:

#0  NS_ScriptErrorReporter (cx=0x97818f8, 
    message=0x92bc6c0 "uncaught exception: Permission denied to get property XULElement.firstChild", report=0xbfd98058)
    at /scratch/cpearce/src/browser/mozilla/dom/src/base/nsJSEnvironment.cpp:379
#1  0xb7d9c244 in js_ReportErrorAgain (cx=0x97818f8, 
    message=0x92bc670 "uncaught exception: Permission denied to get property XULElement.firstChild", reportp=0xbfd98058) at /scratch/cpearce/src/browser/mozilla/js/src/jscntxt.c:1228
#2  0xb7d9b58e in ReportError (cx=0x97818f8, 
    message=0x92bc670 "uncaught exception: Permission denied to get property XULElement.firstChild", reportp=0xbfd98058) at /scratch/cpearce/src/browser/mozilla/js/src/jscntxt.c:855
#3  0xb7d9c0b9 in js_ReportErrorNumberVA (cx=0x97818f8, flags=0, 
    callback=0xb7d9c579 <js_GetErrorMessage>, userRef=0x0, errorNumber=147, charArgs=1, 
    ap=0xbfd980f0 "\ufffd\ufffd+\t@5\ufffd\ufffd\030)\ufffd\t")
    at /scratch/cpearce/src/browser/mozilla/js/src/jscntxt.c:1178
#4  0xb7d9117e in JS_ReportErrorNumber (cx=0x97818f8, 
    errorCallback=0xb7d9c579 <js_GetErrorMessage>, userRef=0x0, errorNumber=147)
    at /scratch/cpearce/src/browser/mozilla/js/src/jsapi.c:5377
#5  0xb7dc2295 in js_ReportUncaughtException (cx=0x97818f8)
    at /scratch/cpearce/src/browser/mozilla/js/src/jsexn.c:1350
#6  0xb7d8fcdd in JS_CallFunctionValue (cx=0x97818f8, obj=0xb05bdc40, fval=-1336168704, 
    argc=1, argv=0x9a82930, rval=0xbfd98244)
    at /scratch/cpearce/src/browser/mozilla/js/src/jsapi.c:4940
#7  0xb62fa242 in nsJSContext::CallEventHandler (this=0x97818c0, aTarget=0x8e45ec0, 
    aScope=0xb05dc180, aHandler=0xb05baf00, aargv=0x974f218, arv=0xbfd98418)
    at /scratch/cpearce/src/browser/mozilla/dom/src/base/nsJSEnvironment.cpp:1944
#8  0xb63729fc in nsJSEventListener::HandleEvent (this=0x90662d0, aEvent=0x93ee638)
    at /scratch/cpearce/src/browser/mozilla/dom/src/events/nsJSEventListener.cpp:235
#9  0xb62a8527 in nsXBLPrototypeHandler::ExecuteHandler (this=0x877fde0, aTarget=0x8e45ec0, 
    aEvent=0x93ee638)
    at /scratch/cpearce/src/browser/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:361
#10 0xb62a1f02 in nsXBLEventHandler::HandleEvent (this=0x881b8b0, aEvent=0x93ee638)
    at /scratch/cpearce/src/browser/mozilla/content/xbl/src/nsXBLEventHandler.cpp:86
#11 0xb6110ecb in nsEventListenerManager::HandleEventSubType (this=0x8fba710, 
    aListenerStruct=0x8fba7f0, aListener=0x881b8b0, aDOMEvent=0x93ee638, 
    aCurrentTarget=0x8e45ec0, aPhaseFlags=6)
    at /scratch/cpearce/src/browser/mozilla/content/events/src/nsEventListenerManager.cpp:1081
#12 0xb6111328 in nsEventListenerManager::HandleEvent (this=0x8fba710, 
    aPresContext=0x8bc7bf0, aEvent=0xbfd98a08, aDOMEvent=0xbfd98998, 
    aCurrentTarget=0x8e45ec0, aFlags=6, aEventStatus=0xbfd9899c)
    at /scratch/cpearce/src/browser/mozilla/content/events/src/nsEventListenerManager.cpp:1183
#13 0xb61401a5 in nsEventTargetChainItem::HandleEvent (this=0x98b58a8, 
    aVisitor=@0xbfd98990, aFlags=6)
    at /scratch/cpearce/src/browser/mozilla/content/events/src/nsEventDispatcher.cpp:206
#14 0xb614045d in nsEventTargetChainItem::HandleEventTargetChain (this=0x98b5a68, 
    aVisitor=@0xbfd98990, aFlags=6, aCallback=0x0)
    at /scratch/cpearce/src/browser/mozilla/content/events/src/nsEventDispatcher.cpp:264
#15 0xb6140cea in nsEventDispatcher::Dispatch (aTarget=0x8e45ec0, aPresContext=0x8bc7bf0, 
    aEvent=0xbfd98a08, aDOMEvent=0x0, aEventStatus=0xbfd98a74, aCallback=0x0)
    at /scratch/cpearce/src/browser/mozilla/content/events/src/nsEventDispatcher.cpp:479
#16 0xb601e4cf in nsXULPopupManager::FirePopupShowingEvent (this=0x83ee718, 
    aPopup=0x8e45ec0, aMenu=0x0, aPresContext=0x8bc7bf0, aPopupType=ePopupTypeTooltip, 
    aIsContextMenu=0, aSelectFirstItem=0)
    at /scratch/cpearce/src/browser/mozilla/layout/xul/base/src/nsXULPopupManager.cpp:902
#17 0xb601ec73 in nsXULPopupManager::ShowPopupAtScreen (this=0x83ee718, aPopup=0x8e45ec0, 
    aXPos=19, aYPos=108, aIsContextMenu=0, aTriggerEvent=0x0)
    at /scratch/cpearce/src/browser/mozilla/layout/xul/base/src/nsXULPopupManager.cpp:433
#18 0xb5fef04d in nsXULTooltipListener::LaunchTooltip (this=0x88c98e0)
    at /scratch/cpearce/src/browser/mozilla/layout/xul/base/src/nsXULTooltipListener.cpp:515
#19 0xb5fef238 in nsXULTooltipListener::ShowTooltip (this=0x88c98e0)
    at /scratch/cpearce/src/browser/mozilla/layout/xul/base/src/nsXULTooltipListener.cpp:414
#20 0xb5fef5a6 in nsXULTooltipListener::sTooltipCallback (aTimer=0x8e58800, 
    aListener=0x88c98e0)
    at /scratch/cpearce/src/browser/mozilla/layout/xul/base/src/nsXULTooltipListener.cpp:736
#21 0xb7d0c210 in nsTimerImpl::Fire (this=0x8e58800)
    at /scratch/cpearce/src/browser/mozilla/xpcom/threads/nsTimerImpl.cpp:400
#22 0xb7d0c437 in nsTimerEvent::Run (this=0xb0439438)
    at /scratch/cpearce/src/browser/mozilla/xpcom/threads/nsTimerImpl.cpp:488
#23 0xb7d05fa7 in nsThread::ProcessNextEvent (this=0x809c870, mayWait=1, result=0xbfd98d30)
    at /scratch/cpearce/src/browser/mozilla/xpcom/threads/nsThread.cpp:510
#24 0xb7c8c413 in NS_ProcessNextEvent_P (thread=0x809c870, mayWait=1)
    at nsThreadUtils.cpp:227
#25 0xb598b318 in nsBaseAppShell::Run (this=0x83ac5d0)
    at /scratch/cpearce/src/browser/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
#26 0xb530277d in nsAppStartup::Run (this=0x8453a18)
    at /scratch/cpearce/src/browser/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:181
#27 0xb7eb0bef in XRE_main (argc=5, argv=0xbfd993f4, aAppData=0x804c988)
    at /scratch/cpearce/src/browser/mozilla/toolkit/xre/nsAppRunner.cpp:3229
#28 0x08048f11 in main (argc=5, argv=0xbfd993f4)
    at /scratch/cpearce/src/browser/mozilla/browser/app/nsBrowserApp.cpp:158


This occurs twice with from two different nsTimerEvents.

Comment 75

[Chris Pearce [:cpearce (Not reading bugmail)]]

This happens when running this test:

http://mxr.mozilla.org/seamonkey/source/toolkit/content/tests/widgets/test_tooltip.xul

This creates one of these xul windows:

http://mxr.mozilla.org/seamonkey/source/toolkit/content/tests/widgets/window_tooltip.xul 

Which I guess is trigging a timer from this file:

http://mxr.mozilla.org/seamonkey/source/toolkit/content/tests/widgets/popup_shared.js

Comment 76

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ho hum, it looks like the mochitest for bug 396064 is screwing up on Linux and leaving focus suppression on. If you run the mochitest by itself, and then start clicking around (say in the search bar) the focus event is dropped in nsViewManager::DispatchEvent(). If you run layout mochitests, tests after 396064 start going bad, and are dropping focus events more often than I think they should (cos the focus suppression is left on somehow).

For reference, here's the mochitest:

http://mxr.mozilla.org/seamonkey/source/layout/base/tests/test_bug396024.html

Comment 77

[Martijn Wargers (dead)]

Hi Chris, I wrote that test. I guess it's an annoying test, since it already prevented a patch earlier from being checked in, but it seems to me that the focus suppression needs fixing somewhere, right?

Comment 78

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah, one problem with the patch is that each frame constructor has only one
nsFocusSuppressor object. When this suppresses, it backs up the old suppression
state to a member variable, so that it can restore it on unsuppress. However if
suppress gets called twice (or more), the second time it backs up the
suppression state, it overwrites the previously backed up state, so the first
call can't be undone correctly. This causes focus suppression to be left on,
and this is probably what's breaking stuff. The solution is to use a global
counter instead of backing up the state.

Fixing this does stop the mochitest that was failing, but I get
/tests/toolkit/content/tests/widgets/test_tooltip.xul locking up when I run
full mochitests on linux with my "fixed" patch, and without my patch this
doesn't happen. So I think something else may be going on too...

Comment 79

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, so here's what's happening...

The password autocomplete now passes for my, but test_tooltip.xul locks up sometimes. You must run test_tooltip.xul from the main mochitest/toolkit page, you can't start the test by itself directly.

It looks like the timer that starts the test is either not executing, or is failing to be set.

Comment 80

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, so the actual problem with the patch was what I addressed in comment #78. 

The reason I've been getting other mochitests locking up is because I've been running mochitest with the mouse in the wrong place! It seems that EventUtils.js:synthesizeMouse() (which calls nsIDOMWindowUtils::sendMouseEvent()) does not work properly when the mouse is hovering and visible over the window upon which the events are being sent to. I'll file a bug to address this. It happens on Windows as well as Linux, possibly Mac too. This happens with and without my patch.

To get around this problem, you just run the Mochitests with the mouse outside of the browser window (but the window should still be focused). All the mochitests will pass (with and without my patch). If you have the mouse cursor inside the window, the toolkit mochitests will very likely fail as they rely on sendMouseEvent(). 

Comment 81

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v9 - re-entrancy problem fixed

As v8, but fixes reentrancy problem identified in comment #78, but replacing the "old suppresion state" field of nsFocusSuppressor with a global counter.

Comment 82

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've filed Bug 416276 for the "mouse position affect mochitest" bug.

Comment 83

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

+#include "nsViewManager.h"

Why do we need that? Seems like we don't.

I think at this point it makes sense to break nsIViewManager::SetSuppressFocusEvent into two methods, SuppressFocusEvents and UnsuppressFocusEvents. Their code is basically completely separate.

Other than that, this looks good.

Comment 84

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v10 - v9 with comments addressed

This is v9 with the comments nsViewManager::SetSuppressFocusEvent() split up, and unneeded #include removed.

Comment 85

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

Sorry, I mean split SetSuppressFocusEvents into two methods with no parameters, all the way up to and including nsIViewManager.

Comment 86

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v11 - split into suppress/unsuppress methods

Comments should now be addressed...

Comment 87

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

Comment on attachment 302506 [details] [diff] [review]
Patch v11 - split into suppress/unsuppress methods

+  if (!sSuppressCount) {
+    // We're turning off suppression, synthesize LOSTFOCUS/GOTFOCUS.
+    if (GetCurrentlyFocusedView() != GetViewFocusedBeforeSuppression()) {

Saves indentation if you make this an early exit

  if (sSuppressCount > 0 ||
      GetCurrentlyFocusedView() == GetViewFocusedBeforeSuppression())
    return;

Comment 88

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v12

As patch v11, but with a fail-fast return added to nsViewManager::UnsuppressFocusEvents() to reduce indentation.

r+/sr+ roc.

Comment 89

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

relanded

Comment 90

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

backed out again, lots of mochitest failures on centos5:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1202781000.1202785124.29867.gz&fulltext=1

Comment 91

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

This might also be related,
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1202782231.1202783568.25880.gz&fulltext=1

*** 11772 ERROR FAIL | Error thrown during test: uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: http://localhost:8888/tests/docshell/test/navigation/NavigationUtils.js :: searchForFinishedFrames :: line 207"  data: no] | got 0, expected 1 | /tests/docshell/test/navigation/test_reserved.html

but that could also be random.

Comment 92

[Martijn Wargers (dead)]

That fail error was what bug 416622 was about.

Comment 93

[Chris Pearce [:cpearce (Not reading bugmail)]]

The only way that I can reproduce failures in these mochitests is by leaving the mouse over the browser window while the mochitests are running. But when I do that, I get failures both with and without my patch. When I don't do this, they don't fail, with or without my patch. So I dunno what's going on here...

Comment 94

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

Get Matthew and/or Karl to try.

Comment 95

[Chris Pearce [:cpearce (Not reading bugmail)]]

Matthew wasn't able to reproduce the errors. However, his patch for bug 297080 does resolve some of the issues with leaving the mouse over the browser window while running the mochitests, as I mentioned in comment 93.

It would be interesting to retry landing my patch after Matthew's patch for bug 297070 lands, though I'm not entirely confident that it will stop the failures.

After a lot of testing, the only way that I can reproduce the *exact* same errors in mochitests in the *exact* same order as given in the test log is by having the mochitest window not focused while the tests are running. However this happens with and without my patch. So perhaps my patch is somehow causing a focus event to be dropped on the unit test server, but for some reason it's not being dropped on our local machines. I note that when I run the mochitests on Linux locally only one focus event is deferred over all 40k-odd mochitests, somewhere in the first 15K or so, whereas the tests which are failing are in the very last bunch of tests run.

So I propose that I modify my patch to print debug info when it defers a focus event, and we check that in, let the tests run, then back it out, and look at the error log. We can then determine whether my patch is causing the mochitests window to lose focus, which would cause these tests to fail in this way.

Comment 96

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

Yeah, let's do that.

Comment 97

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch with extra logging for tinderbox

This is the patch with more logging. It logs to stdout when it suppresses a focus/blur event, and at other interesting times. It also verifies that the document in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html is focused before it runs a test. Interestingly, the first time it isn't as the test is run onLoad, which happens before onFocus.

Comment 98

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

I landed the "extra logging" patch to collect data when centos5 failed.

It didn't fail.

Here's the log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1203415020.1203418400.20580.gz&fulltext=1

Comment 99

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

I'm going to leave this in. I checked in a patch to #ifdef out the logging printfs that Chris added. If that causes test failures to reappear, I will kill someone.

Comment 100

[Martijn Wargers (dead)]

Thank you all for you perseverance!

This is verified fixed for me, with today's trunk build.

Comment 101

[Justin Dolske [:Dolske]]

Is the logging in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html still needed / useful?

When I run the tests locally on OS X, (via perl runtests.pl --test-path=toolkit/components/passwordmgr/test/) I get a failure in the test for:

ok(document.activeElement == uname, "Username should be focused.");

But that's the only failure. I'm guessing this might be a side-effect from having to click "Run Tests" (focus shift?) when launching them this way?

Comment 102

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #101)
> Is the logging in
> toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html still
> needed / useful?

No, the logging is not needed anymore, that can be safely removed.


> When I run the tests locally on OS X, (via perl runtests.pl
> --test-path=toolkit/components/passwordmgr/test/) I get a failure in the test
> for:
> 
> ok(document.activeElement == uname, "Username should be focused.");
> 
> But that's the only failure. I'm guessing this might be a side-effect from
> having to click "Run Tests" (focus shift?) when launching them this way?
> 

Yeah, I also found that it would fail if I had the mochitests --autorun, but somehow it passes on tinderbox.

Comment 103

[Justin Dolske [:Dolske]]

Ok, thanks. I backed out the debug logging.

Checking in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html;
  new revision: 1.6; previous revision: 1.5

Comment 104

[Daniel Veditz [:dveditz]]

This does not crash the 1.8 branch -- should be OK to unhide this bug right?

I do get an assertion on that branch:
###!!! ASSERTION: Adding child where we already have a child?  This will likely misbehave: 'Error', file /Users/daniel/dev/ff2/mozilla/xpfe/components/shistory/src/nsSHEntry.cpp, line 580

Comment 105

[Mats Palmgren (inactive)]

Can we open this bug now so we can land the test please?

Comment 106

[Daniel Veditz [:dveditz]]

ouch, my usual queries have missed a backlog of really old bugs. thanks for bringing this one up.

Comment 107

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92861edacace

Comment 108

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/92861edacace