322636 - [FIX]ctrl-y/redo shortcut crashes firefox in find bar/fayt [@ nsContentUtils::ContentIsDescendantOf]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Majken Connor [:Kensie]]

Using the redo shortcut if you haven't used undo first, in the find bar, crashes firefox when you try typing again.

Steps to reproduce:
1. Open fayt by typing or ctrl-f for find
2. Hit the redo shortcut (ctrl-y in windows, or F3 if you have a Microsoft kb with the function lock off, heh)
3. Wait for the bar to disappear.
4. Type again to open fayt bar/ctrl-f for find (either of these will do no matter which one you did for 1)
5. Yay!

Doesn't crash if you undo first.

Comment 1

[Majken Connor [:Kensie]]

Ammendment to instruction 3 - hit escape to dismiss find bar if that's what you opened.

Also, talkback IDs TB13660724Z TB13660660Q

Comment 2

[#foxymonkies @ irc.m.o]

Lucy found a bug! Lucy found a bug!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Does making nsBaseContentList hold a strong ref to mRootContent prevent this crash?  I realize it'll leak in some cases; just trying to get an idea of where things stand.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Stack Signature	 nsContentUtils::ContentIsDescendantOf 983233a1
Product ID	Firefox15
Build ID	2005111116
Trigger Time	2006-01-06 18:35:30.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	FIREFOX.EXE + (00131caa)
Since Last Crash	105 sec
Total Uptime	1747166 sec
Trigger Reason	Access violation
Source File, No.	mozilla/content/base/src/nsContentUtils.cpp, 959
Stack Trace 	
nsContentUtils::ContentIsDescendantOf [content/base/src/nsContentUtils.cpp, 959]
nsGenericElement::RemoveChildAt [content/base/src/nsGenericElement.cpp, 2840]
nsGenericElement::RemoveChild [content/base/src/nsGenericElement.cpp, 3720]
DeleteElementTxn::DoTransaction [editor/libeditor/base/DeleteElementTxn.cpp, 120]
nsTransactionItem::DoTransaction [editor/txmgr/src/nsTransactionItem.cpp, 182]
nsTransactionManager::DoTransaction [editor/txmgr/src/nsTransactionManager.cpp, 132]
nsEditor::DoTransaction [editor/libeditor/base/nsEditor.cpp, 684]
nsEditor::DeleteNode [editor/libeditor/base/nsEditor.cpp, 1526]
nsTextEditRules::WillInsert [editor/libeditor/text/nsTextEditRules.cpp, 395]
nsTextEditRules::WillDoAction [editor/libeditor/text/nsTextEditRules.cpp, 302]
nsPlaintextEditor::InsertText [editor/libeditor/text/nsPlaintextEditor.cpp, 790]
nsTextControlFrame::SetValue [layout/forms/nsTextControlFrame.cpp, 3174]
nsTextControlFrame::InitEditor [layout/forms/nsTextControlFrame.cpp, 1742]
nsCSSFrameConstructor::CreateAnonymousFrames [layout/base/nsCSSFrameConstructor.cpp, 5810]
nsCSSFrameConstructor::CreateAnonymousFrames [layout/base/nsCSSFrameConstructor.cpp, 5683]
nsCSSFrameConstructor::ConstructHTMLFrame [layout/base/nsCSSFrameConstructor.cpp, 5617]
nsCSSFrameConstructor::ConstructFrameInternal [layout/base/nsCSSFrameConstructor.cpp, 7735]
nsCSSFrameConstructor::ConstructFrame [layout/base/nsCSSFrameConstructor.cpp, 7624]
nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp, 11977]
nsCSSFrameConstructor::ConstructXULFrame [layout/base/nsCSSFrameConstructor.cpp, 6309]
nsCSSFrameConstructor::ConstructFrameInternal [layout/base/nsCSSFrameConstructor.cpp, 7741]
nsCSSFrameConstructor::ConstructFrame [layout/base/nsCSSFrameConstructor.cpp, 7624]
nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp, 11977]
nsCSSFrameConstructor::ConstructXULFrame [layout/base/nsCSSFrameConstructor.cpp, 6309]
nsCSSFrameConstructor::ConstructFrameInternal [layout/base/nsCSSFrameConstructor.cpp, 7741]
nsCSSFrameConstructor::ConstructFrameInternal [layout/base/nsCSSFrameConstructor.cpp, 7689]
nsCSSFrameConstructor::ConstructFrame [layout/base/nsCSSFrameConstructor.cpp, 7624]
nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp, 11977]
nsCSSFrameConstructor::ConstructXULFrame [layout/base/nsCSSFrameConstructor.cpp, 6309]
nsCSSFrameConstructor::ConstructFrameInternal [layout/base/nsCSSFrameConstructor.cpp, 7741]
nsCSSFrameConstructor::ConstructFrame [layout/base/nsCSSFrameConstructor.cpp, 7624]
nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp, 11977]
nsCSSFrameConstructor::ConstructXULFrame [layout/base/nsCSSFrameConstructor.cpp, 6309]
nsCSSFrameConstructor::ConstructFrameInternal [layout/base/nsCSSFrameConstructor.cpp, 7741]
nsCSSFrameConstructor::ConstructFrame [layout/base/nsCSSFrameConstructor.cpp, 7624]
nsCSSFrameConstructor::ContentInserted [layout/base/nsCSSFrameConstructor.cpp, 9368]
nsCSSFrameConstructor::RecreateFramesForContent [layout/base/nsCSSFrameConstructor.cpp, 11857]
nsCSSFrameConstructor::MaybeRecreateFramesForContent [layout/base/nsCSSFrameConstructor.cpp, 11758]
nsCSSFrameConstructor::RestyleElement [layout/base/nsCSSFrameConstructor.cpp, 10407]
nsCSSFrameConstructor::ProcessOneRestyle [layout/base/nsCSSFrameConstructor.cpp, 13812]
nsCSSFrameConstructor::ProcessPendingRestyles [layout/base/nsCSSFrameConstructor.cpp, 13860]
PresShell::FlushPendingNotifications [layout/base/nsPresShell.cpp, 5326]
nsDocument::FlushPendingNotifications [content/base/src/nsDocument.cpp, 4253]
nsGenericHTMLElement::GetPrimaryFrameFor [content/html/content/src/nsGenericHTMLElement.cpp, 2259]
nsGenericHTMLElement::GetFormControlFrameFor [content/html/content/src/nsGenericHTMLElement.cpp, 2276]
nsHTMLInputElement::Select [content/html/content/src/nsHTMLInputElement.cpp, 1183]
XPTC_InvokeByIndex [xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, 102]
XPCWrappedNative::CallMethod [js/src/xpconnect/src/xpcwrappednative.cpp, 2139]
XPC_WN_CallMethod [js/src/xpconnect/src/xpcwrappednativejsops.cpp, 1444]
js_Invoke [js/src/jsinterp.c, 1177]
js_Interpret [js/src/jsinterp.c, 3523]
js_Invoke [js/src/jsinterp.c, 1197]
nsXPCWrappedJSClass::CallMethod [js/src/xpconnect/src/xpcwrappedjsclass.cpp, 1369]
nsXPCWrappedJS::CallMethod [js/src/xpconnect/src/xpcwrappedjs.cpp, 462]
SharedStub [xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, 147]
nsEventListenerManager::HandleEventSubType [content/events/src/nsEventListenerManager.cpp, 1685]
nsEventListenerManager::HandleEvent [content/events/src/nsEventListenerManager.cpp, 1786]
nsXULElement::HandleDOMEvent [content/xul/content/src/nsXULElement.cpp, 2153]
nsXULElement::HandleDOMEvent [content/xul/content/src/nsXULElement.cpp, 2174]
nsXULElement::HandleDOMEvent [content/xul/content/src/nsXULElement.cpp, 2174]
nsXULElement::HandleDOMEvent [content/xul/content/src/nsXULElement.cpp, 2174]
nsXULElement::HandleDOMEvent [content/xul/content/src/nsXULElement.cpp, 2174]
nsXULElement::HandleChromeEvent [content/xul/content/src/nsXULElement.cpp, 2833]
nsGlobalWindow::HandleDOMEvent [dom/src/base/nsGlobalWindow.cpp, 1574]

Comment 5

[Martijn Wargers (dead)]

This regressed between 2005-06-23 and 2005-06-24:
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=2005-06-23+07%3A00%3A00&maxdate=2005-06-24+06%3A00%3A00&cvsroot=%2Fcvsroot
Most likely a regression from bug 259454.

How do I nsBaseContentList hold a strong ref to mRootContent?

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

(In reply to comment #3)
> Does making nsBaseContentList hold a strong ref to mRootContent prevent this
> crash?
Yes, that seems to fix the crash.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  So let's try to figure out how to do that without leaking.. Perhaps we need yet another constructor arg (for whether to hold a strong ref or a weak ref)... or something.  I'm not sure how to handle a node needing to stay alive because someone's holding its childNodes list or something.

Comment 8

[Martijn Wargers (dead)]

I don't want to sound naggy, but maybe this should block a next 1.8.0.x release?
The fact that one is able to crash Firefox by using certain key combinations makes Firefox feel kinda dumb.

Comment 9

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

So I have a plan for this on trunk, which is to add an observer mechanism where you can get notified when mutations happen in a certain subtree, as well as when the root of that subtree goes away.

However that will never be portable to the 1.8.* branch, so we need to figure something else out there.

Maybe we could do some miniature version of that where you can listen to an element going away. We could simply stick an array in nsDOMSlots which the element walks through in its dtor.

Comment 10

[Majken Connor [:Kensie]]

isn't the problem that it doesn't know redo should be "greyed out" like another context menu would?  The find bar is the only part that has this problem, other parts of the browser don't try to redo if there is no redo available.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So this is basically looking like bug 240471, right?  Note that per comments in that bug, this crash means the content list is being created in C++, not JS.

Also, I can't seem to reproduce this. Could someone who can tell me what sort of nsContentList we're looking at here?  Putting breakpoints in the two nsContentList constructors that are conditional on mRootContent not being null might work, maybe...

Comment 12

[Martijn Wargers (dead)]

Attachment: Backtrace from debug build

I'm not sure what you're asking in comment 11, Boris.
If you provide exacts steps (putting what code into what spot), I could try and dig it up for you.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Load the page.  In gdb, do something like this:

(gdb) break 'nsContentList::nsContentList(nsIDocument*, int (*)(nsIContent*, int, nsIAtom*, nsAString_internal const&), nsAString_internal const&, nsIContent*, int, nsIAtom*, int, int)' 
Breakpoint 1 at 0xb647028e: file ../../../../mozilla/content/base/src/nsContentList.cpp, line 356.
(gdb) break 'nsContentList::nsContentList(nsIDocument*, nsIAtom*, int, nsIContent*, int)' 
Breakpoint 2 at 0xb646fffa: file ../../../../mozilla/content/base/src/nsContentList.cpp, line 330.

Then do the steps to reproduce.  When the breakpoints get hit for the first time do:

(gdb) condition 2 (aRootContent != 0)

(if breakpoint 2 gets hit; same for 1 if 1 gets hit, with s/2/1/).

Then see which of the calls are not coming from JS things like .childNodes or .getElementsByTagName... (that is note coming from GetChildNodes or GetElementsByTagName, of if coming from those they were not called through XPConnect).

Comment 14

[Martijn Wargers (dead)]

Well, the breakpoints don't seem to get hit.
I've done this:
(gdb) break 'nsContentList::nsContentList(nsIDocument*, int (*)(nsIContent*, in
t, nsIAtom*, nsAString_internal const&), nsAString_internal const&, nsIContent*
, int, nsIAtom*, int, int)'
Breakpoint 1 at 0x5906c7e: file c:/mozilla/mozilla/content/base/src/nsContentLis
t.cpp, line 364.
(gdb) break 'nsContentList::nsContentList(nsIDocument*, nsIAtom*, int, nsIConte
nt*, int)'
Breakpoint 2 at 0x5906a30: file c:/mozilla/mozilla/content/base/src/nsContentLis
t.cpp, line 337.

The breakpoints seem to get set just fine.
But after following the steps to reproduce, I crash directly (and with a different stack it seems):
Program received signal SIGSEGV, Segmentation fault.
0x05908e04 in nsContentList::IsContentAnonymous(nsIContent*) (this=0xf64fd78,
    aContent=0x106f2740)
    at c:/mozilla/mozilla/content/base/src/nsContentList.cpp:1028
1028      return mRootContent->GetBindingParent() != aContent->GetBindingParent(
);

Comment 15

[Boris Zbarsky [:bzbarsky]]

Mmmmhm.   What if you set the breakpoints before loading the page?  Or possibly even set the breakpoints, open a new window, and reproduce in the new window?

Comment 16

[Martijn Wargers (dead)]

Nope, again my debug build segfaults, the breakpoints aren't hit.
I've tried it now by setting the breakpoints and then opening a new window.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Um.  Do the breakpoints survive a quit and restart of firefox (without quitting gdb)?  If so, can you do that?

Comment 18

[Martijn Wargers (dead)]

(In reply to comment #17)
> Um.  Do the breakpoints survive a quit and restart of firefox (without quitting
> gdb)?  If so, can you do that?
How would I know that? I did what you ask, but I still get no assertions and crash immediately. Maybe someone else gets better results?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> How would I know that? 

If gdb stops at them after the restart....

Comment 20

[Régis Caspar]

I get some assertion here: 
###!!! ASSERTION: No first node!: 'mFirst', file c:/mozilla/mozilla/content/base/src/nsContentIterator.cpp, line 960
(4 times)

Also, step to reproduce here is 
1. CTRL-F then type something then CTRL-Y

I break at nsContentList::nsContentList(nsIDocument *aDocument,
                             nsContentListMatchFunc aFunc,
                             const nsAString& aData,
                             nsIContent* aRootContent,
                             PRBool aDeep,
                             nsIAtom* aMatchAtom,
                             PRInt32 aMatchNameSpaceId,
                             PRBool aFuncMayDependOnAttr) 
when typing CRTL-F (no assertion)

Then nsContentList::nsContentList(nsIDocument *aDocument, 
                             nsIAtom* aMatchAtom,
                             PRInt32 aMatchNameSpaceId,
                             nsIContent* aRootContent,
                             PRBool aDeep) 
when hitting CTRL-Y (no assertion)

Then I got the crash here:
nsIContent* GetParent() const
  {
    return NS_LIKELY(mParentPtrBits & PARENT_BIT_PARENT_IS_CONTENT) ?
           NS_REINTERPRET_CAST(nsIContent*,
                               mParentPtrBits & ~kParentBitMask) :
           nsnull;
  }

Comment 21

[Boris Zbarsky [:bzbarsky]]

What are the stacks to hitting those two constructors?  Can you possibly attach them in an attachment?

Comment 22

[Régis Caspar]

Attachment: Detailled stack, vars, stderrs+stdout

(In reply to comment #21)
> What are the stacks to hitting those two constructors?  Can you possibly attach
> them in an attachment?
Here it is :)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Regis, thanks!

So here's the relevant part:

> I then hit CTRL-Y 

>	firefox.exe!nsContentList::nsContentList(nsIDocument * aDocument=0x035372c8, nsIAtom * aMatchAtom=0x027b4b08, int aMatchNameSpaceId=0xffffffff, nsIContent * aRootContent=0x03b9a220, int aDeep=0x00000001)  Line 337	C++
> 	firefox.exe!NS_GetContentList(nsIDocument * aDocument=0x035372c8, nsIAtom * aMatchAtom=0x027b4b08, int aMatchNameSpaceId=0xffffffff, nsIContent * aRootContent=0x03b9a220)  Line 294 + 0x2d bytes	C++
> 	firefox.exe!nsGenericElement::GetElementsByTagName(const nsAString_internal & aTagname={...}, nsIDOMNodeList * * aReturn=0x0012e688)  Line 1438 + 0x21 bytes	C++
> 	firefox.exe!nsGenericHTMLElement::GetElementsByTagName(const nsAString_internal & aTagname={...}, nsIDOMNodeList * * aReturn=0x0012e688)  Line 417 + 0x14 bytes	C++
> 	firefox.exe!nsHTMLDivElement::GetElementsByTagName(const nsAString_internal & name={...}, nsIDOMNodeList * * _retval=0x0012e688)  Line 59 + 0x14 bytes	C++
> 	firefox.exe!nsTextEditRules::DidRedo(nsISelection * aSelection=0x03b9a500, unsigned int aResult=0x00000000)  Line 1105 + 0x39 bytes	C++

We end up keeping this list alive for a while (see gCachedContentList), so it can outlive the div in question....

As a stopgap for branch (and trunk till we make this stuff suck less), we should probably only stash in gCachedContentList lists which have a null mRootContent.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to test

Does this fix the crashes?

Comment 25

[Régis Caspar]

(In reply to comment #24)
> Does this fix the crashes?
Yes it does here. 
I still get the assertion "###!!! ASSERTION: No first node!: 'mFirst', file c:/mozilla/mozilla/content/base/src/nsContentIterator.cpp, line 960" though.

Comment 26

[Martijn Wargers (dead)]

That assertion is a different bug, see bug 328412.

Comment 27

[Régis Caspar]

(In reply to comment #26)
> That assertion is a different bug, see bug 328412.
OK, thanks :)

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 221409 [details] [diff] [review]
Patch to test

r+sr+a=jst

Comment 29

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk and 1.8 branch.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 221409 [details] [diff] [review]
Patch to test

approved for 1.8.0 branch, a=dveditz for drivers

Comment 31

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.0.5

Comment 32

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.5) Gecko/20060620 Firefox/1.5.0.5