Closed Bug 399852 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect, P1, critical)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: martijn.martijn, Assigned: cpearce)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:investigate] [dbaron-1.9:RsCc])

Crash Data

Attachments

(3 files, 12 obsolete files)

345 bytes, text/html
Details
13.27 KB, patch
cpearce
: review+
cpearce
: superreview+
Details | Diff | Splinter Review
16.79 KB, patch
Details | Diff | Splinter Review
Attached file testcase (obsolete) —
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]
Flags: blocking1.9?
Blocks: 355843
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RsCc]
Attached file testcase
The original testcase doesn't seem to crash anymore, but this one still does after 500ms.
Attachment #284923 - Attachment is obsolete: true
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
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.
Assignee: nobody → chris
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.
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.

(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.
Blocks: 377960, 394818
Can we elevate the priority of this to P2, as there are P2 crashes that depend on it?
(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.)
(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...
* 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.
Attachment #291152 - Flags: review?
Attachment #291152 - Flags: review? → review?(emaijala)
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..
(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?
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.
Attachment #291152 - Attachment is obsolete: true
Attachment #292537 - Flags: review?(emaijala)
Attachment #291152 - Flags: review?(emaijala)
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.
(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?
If we just don't copy the nativeMsg data, leaving it null in the deferred event, does that cause problems?
Blocks: 387632
(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 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
Attachment #292537 - Flags: review?(emaijala) → review+
Attached patch Patch v3 - With Ere's changes (obsolete) — Splinter Review
This is patch v2 with Eli's requested changes. sr?
Attachment #292537 - Attachment is obsolete: true
Attachment #293004 - Flags: superreview?(jst)
Attachment #293004 - Flags: review+
Comment on attachment 293004 [details] [diff] [review]
Patch v3 - With Ere's changes

Sorry! s/Eli/Ere/
Attachment #293004 - Attachment description: Patch v3 - With Eli's changes → Patch v3 - With Ere's changes
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.
Attachment #293004 - Flags: superreview?(jst) → superreview?(roc)
Chris, could you post a new patch with the nativemsg copying removed? That would simplify the review significantly. Thanks...
Attached patch Patch without nativeMsg (obsolete) — Splinter Review
(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.
Attachment #293004 - Attachment is obsolete: true
Attachment #293780 - Flags: superreview?(roc)
Attachment #293780 - Flags: review+
Attachment #293004 - Flags: superreview?(roc)
+ 
+     // 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.
Stephen, this may affect your focus patch(es).
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.
> 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.
> 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.
> 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...
(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?
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.
> 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?
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...
(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.


Attached patch Patch v5 - "reboot" focus (obsolete) — Splinter Review
* 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.
Attachment #293780 - Attachment is obsolete: true
Attachment #295722 - Flags: review?(roc)
Attachment #293780 - Flags: superreview?(roc)
+// 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.
As patch v5 with changes as requested in comment #37.
Attachment #295722 - Attachment is obsolete: true
Attachment #295862 - Flags: review?(roc)
Attachment #295722 - Flags: review?(roc)
+  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 :-)
Attached patch Patch v7 (obsolete) — Splinter Review
Attachment #296073 - Flags: review?
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
Attachment #296073 - Flags: review? → review?(roc)
Attachment #295862 - Attachment is obsolete: true
Attachment #295862 - Flags: review?(roc)
Keywords: checkin-needed
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: in-testsuite?
This caused numerous test failures so I backed it out for Chris Double (the sheriff).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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.
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...
Sorry, I watched the tree, I thought it sticked.
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...
Attachment #296073 - Attachment is obsolete: true
Attachment #296897 - Flags: superreview+
Attachment #296897 - Flags: review+
Keywords: checkin-needed
"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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
(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.
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.
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?
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...
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.
Sure, but we shouldn't be processing events inside a Begin/EndUpdate.
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?
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...
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?
(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).
Attachment #296897 - Attachment description: Patch with bug fixed → Patch v8 - Patch v7 with bug fixed
(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...
The mochitest does still fail on Linux with the patch, but passes on Windows and Mac.
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.
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.
(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.
(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?
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.
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?
Moving TM to b4
Target Milestone: --- → mozilla1.9beta4
(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.
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.
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.

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
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?
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...
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.
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(). 
As v8, but fixes reentrancy problem identified in comment #78, but replacing the "old suppresion state" field of nsFocusSuppressor with a global counter.
Attachment #296897 - Attachment is obsolete: true
Attachment #302065 - Flags: review?(roc)
I've filed Bug 416276 for the "mouse position affect mochitest" bug.
+#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.
This is v9 with the comments nsViewManager::SetSuppressFocusEvent() split up, and unneeded #include removed.
Attachment #302065 - Attachment is obsolete: true
Attachment #302463 - Flags: review?(roc)
Attachment #302065 - Flags: review?(roc)
Sorry, I mean split SetSuppressFocusEvents into two methods with no parameters, all the way up to and including nsIViewManager.
Comments should now be addressed...
Attachment #302463 - Attachment is obsolete: true
Attachment #302506 - Flags: review?(roc)
Attachment #302463 - Flags: review?(roc)
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;
Attachment #302506 - Flags: superreview+
Attachment #302506 - Flags: review?(roc)
Attachment #302506 - Flags: review+
Attached patch Patch v12Splinter Review
As patch v11, but with a fail-fast return added to nsViewManager::UnsuppressFocusEvents() to reduce indentation.

r+/sr+ roc.
Attachment #302506 - Attachment is obsolete: true
Attachment #302512 - Flags: superreview+
Attachment #302512 - Flags: review+
Keywords: checkin-needed
relanded
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
That fail error was what bug 416622 was about.
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...
Whiteboard: [dbaron-1.9:RsCc] → [sg:investigate] [dbaron-1.9:RsCc]
Get Matthew and/or Karl to try.
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.
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.
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
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Thank you all for you perseverance!

This is verified fixed for me, with today's trunk build.
Status: RESOLVED → VERIFIED
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?
(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.
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
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
Crash Signature: [@ nsCSSFrameConstructor::FindFrameWithContent]
Can we open this bug now so we can land the test please?
Flags: needinfo?(dveditz)
ouch, my usual queries have missed a backlog of really old bugs. thanks for bringing this one up.
Group: core-security
Flags: needinfo?(dveditz)
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92861edacace
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.