Closed Bug 322636 Opened 15 years ago Closed 15 years ago

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

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: Kensie, Assigned: bzbarsky)

References

(Depends on 1 open bug)

Details

(Keywords: crash, fixed1.8.1, verified1.8.0.5)

Crash Data

Attachments

(4 files)

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.
Severity: normal → critical
Keywords: crash
Summary: ctrl-y/redo shortcut crashes firefox in find bar/fayt → [@ nsContentUtils::ContentIsDescendantOf ] ctrl-y/redo shortcut crashes firefox in find bar/fayt
Version: unspecified → 1.5 Branch
Ammendment to instruction 3 - hit escape to dismiss find bar if that's what you opened.

Also, talkback IDs TB13660724Z TB13660660Q
OS: Windows XP → All
Lucy found a bug! Lucy found a bug!
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.
Summary: [@ nsContentUtils::ContentIsDescendantOf ] ctrl-y/redo shortcut crashes firefox in find bar/fayt → ctrl-y/redo shortcut crashes firefox in find bar/fayt [@ nsContentUtils::ContentIsDescendantOf]
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]
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?
Blocks: 259454
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Attached patch patchSplinter Review
(In reply to comment #3)
> Does making nsBaseContentList hold a strong ref to mRootContent prevent this
> crash?
Yes, that seems to fix the crash.
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.
Assignee: nobody → general
Component: Find Toolbar / FastFind → DOM
Product: Firefox → Core
QA Contact: fast.find → ian
Version: 1.5 Branch → Trunk
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.
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.
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.
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...
Depends on: 240471
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.
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).
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(
);
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?
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.
Um.  Do the breakpoints survive a quit and restart of firefox (without quitting gdb)?  If so, can you do that?
(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?
> How would I know that? 

If gdb stops at them after the restart....
Keywords: helpwanted
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;
  }




What are the stacks to hitting those two constructors?  Can you possibly attach them in an attachment?
(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 :)
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.
Attached patch Patch to testSplinter Review
Does this fix the crashes?
(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.

That assertion is a different bug, see bug 328412.
(In reply to comment #26)
> That assertion is a different bug, see bug 328412.
OK, thanks :)
Attachment #221409 - Flags: superreview?(jst)
Attachment #221409 - Flags: review?(jst)
Attachment #221409 - Flags: approval1.8.0.5?
Attachment #221409 - Flags: approval-branch-1.8.1?(jst)
Assignee: general → bzbarsky
Keywords: helpwanted
Priority: -- → P1
Summary: ctrl-y/redo shortcut crashes firefox in find bar/fayt [@ nsContentUtils::ContentIsDescendantOf] → [FIX]ctrl-y/redo shortcut crashes firefox in find bar/fayt [@ nsContentUtils::ContentIsDescendantOf]
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 221409 [details] [diff] [review]
Patch to test

r+sr+a=jst
Attachment #221409 - Flags: superreview?(jst)
Attachment #221409 - Flags: superreview+
Attachment #221409 - Flags: review?(jst)
Attachment #221409 - Flags: review+
Attachment #221409 - Flags: approval-branch-1.8.1?(jst)
Attachment #221409 - Flags: approval-branch-1.8.1+
Fixed on trunk and 1.8 branch.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Comment on attachment 221409 [details] [diff] [review]
Patch to test

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #221409 - Flags: approval1.8.0.5? → approval1.8.0.5+
Fixed for 1.8.0.5
Keywords: fixed1.8.0.5
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
Crash Signature: [@ nsContentUtils::ContentIsDescendantOf]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.