372373 - Crash [@ nsFrameSelection::GetFrameForNodeOffset] when shift-tabbing in this testcase

Status: VERIFIED WORKSFORME

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, to reproduce the crash:
- Turn on caret browsing (F7)
- Press a couple of times shift-tab

Talkback ID: TB29844918H
nsFrameSelection::GetFrameForNodeOffset  [mozilla/layout/generic/nsselection.cpp, line 2650]
nsCaret::GetCaretFrameForNodeOffset  [mozilla/layout/base/nscaret.cpp, line 652]
nsCaret::DrawAtPositionWithHint  [mozilla/layout/base/nscaret.cpp, line 585]
nsCaret::DrawCaret  [mozilla/layout/base/nscaret.cpp, line 984]
nsCaret::StartBlinking  [mozilla/layout/base/nscaret.cpp, line 553]
nsEventStateManager::SetContentCaretVisible  [mozilla/content/events/src/nseventstatemanager.cpp, line 5072]
nsEventStateManager::ResetBrowseWithCaret  [mozilla/content/events/src/nseventstatemanager.cpp, line 5128]
nsEventStateManager::PreHandleEvent  [mozilla/content/events/src/nseventstatemanager.cpp, line 890]

This regressed between 2006-08-17 and 2006-08-18:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-08-17+04&maxdate=2006-08-18+06&cvsroot=%2Fcvsroot
I think a regression from bug 336637.

Comment 1

[Eli Friedman]

I tried debugging; I'm apparently crashing in nsCSSFrameConstructor::FindFrameWithContent on an invalid frame pointer.  Couldn't really get much further.  I'll try again later.

Comment 2

[Eli Friedman]

Okay, this is a little unbelievable, but the issue is that we're trying to handle an event during frame construction.  Changing the focus causes some stuff to happen so that we enter the event loop, and handle some event which triggers frame construction (at which point we're handling two events at the same time; the presshell code seems to be okay with this, though).

Frame construction destroys the subdocument frame, which hides its docshell, which in turn hides its content viewer, which in turn hides its widget, which in turn enters the event loop(!), which in turn calls HandleEvent, which tries to handle an event (an accessibility event, I think), which tries to find the current event frame, which crashes.

As you can imagine, trying to do anything related to the frame tree during frame construction is extremely likely to crash.  I'm not completely sure how we should deal with this, though.

Not sure who to CC...

Comment 3

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

So is this Windows only?

Comment 4

[Eli Friedman]

(In reply to comment #3)
> So is this Windows only?

I'm not sure; I've only tested on Windows.  That's not unlikely, though, because it relates to the event code.  On the other hand, it is a crash due to a pointer to a deallocated frame, so it may or may not crash depending on how lucky/unlucky you are.  (My debug build gets a completely different trace from Talkback, and it might be related to that.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> which in turn hides its widget, which
> in turn enters the event loop(!)

This part sounds Windows-only.  (And I was going to make that comment before seeing comment 3 and comment 4.)  If we could find a way to make the Windows widget code not do that, it would fix a whole bunch of Windows-only crashes.

Comment 6

[Ere Maijala (slow)]

The crash happens when processing a NS_GOTFOCUS message the widget layer sends due to receiving a WM_SETFOCUS from Windows. WM_SETFOCUS is received because the previously focused widget (a MozillaWindowClass under MozillaContentFrameWindowClass) is being hidden causing another widget (the MozillaContentFrameWindowClass) to receive focus. SO the flow seems to be this:

onblur removes style from the div -> MozillaWindowClass is hidden -> WM_SETFOCUS to MozillaContentFrameWindowClass -> NS_GOTFOCUS -> crash.

We can of course suppress NS_GOTFOCUS, but in which situations should it be done? 

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm...  So for trunk, would it be more profitable to just work on removing widgets for subframes?

Or would this be a problem with combobox widgets too, etc?

I'd think any time we're hiding a widget via the nsIWidget API we should not be entering the event loop...

Comment 8

[Eli Friedman]

(In reply to comment #7)
> Hmm...  So for trunk, would it be more profitable to just work on removing
> widgets for subframes?

I think so, if we can do it for 1.9.

> Or would this be a problem with combobox widgets too, etc?

I don't think popups can actually receive focus; not sure, though. XUL popups might be an issue, at least at some point in the future, but I can't come up with a testcase. (I tried putting textboxes into an XUL popup, but I don't think they actually properly got focus, i.e. you couldn't type in them.  Got some undescribably odd behavior putting a combobox into an XUL popup, but it didn't crash.)

Comment 9

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

I don't think we can entirely remove widgets for subframes for 1.9. We can hide them so that all painting comes through the root window --- and I desperately want to do that --- but removing them for focus purposes is additional work that I have no insight into the difficulty of. It's likely to affect accessiblity, for one thing.

Comment 10

[Eli Friedman]

I think we could prevent it from crashing by fixing http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1323&mark=9514-9515#9505
Would that be easier?  (I'm not completely sure of the implications... the comment was added a long time ago.)

Comment 11

[Eli Friedman]

Attachment: Like so

I might as well throw this out... although it's mostly untested, and I'm not even sure it's correct.  But it does prevent the crash.  Comments?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I assume you tested this wth the float testcases from the bug that the blame for that comment you're removing points to?

Comment 13

[Eli Friedman]

(In reply to comment #12)
> I assume you tested this wth the float testcases from the bug that the blame
> for that comment you're removing points to?

The comment is about 8 years old, so there's no associated bug.

Hmm, it looks like the comment is still relevant, though (see nsFloatCache and friends).  Not sure what to do about it.

Comment 14

[Martijn Wargers (dead)]

This has now become worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070417 Minefield/3.0a4pre

Comment 15

[Martijn Wargers (dead)]

And now it's crashing again in current trunk build.
Sorry, I guess I shouldn't have marked it worksforme, that quickly.

Comment 16

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

This might be scary in scope so we (I) should fix it for beta2.

Comment 17

[Daniel Holbert [:dholbert]]

Martijn (or anyone), can you still reproduce this? 

I don't see the crash in either of these builds:
 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111304 Minefield/3.0b2pre
 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110810 Firefox/3.0b1

I do, however, see this (maybe related?) issue:
 - Click in the text box
 - Press shift-tab, to highlight the iframe
The iframe and textbox both shift down a bit when the iframe becomes highlighted.

Comment 18

[Martijn Wargers (dead)]

Yeah, this is now worksforme again, marking worksforme.
The shifting is because the surrounding div gets his position:absolute removed.
I guess it's probably from some bizar margin collapsing css rule. The same happens on branch and Opera9, so it's probably correct behavior.

Comment 19

[Carsten Book [:Tomcat]]

verified using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3 ID:2008020514 - no crash on testcase- changing to verified