576174 - "ASSERTION: Losing track of existing primary frame" on layout/xul/base/src/crashtests/432058-1.xul

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

layout/xul/base/src/crashtests/432058-1.xul triggers this assertion.  It's not turning the tree orange because it's already marked as asserting in the manifest.

###!!! ASSERTION: Losing track of existing primary frame: '!aFrame || !mPrimaryFrame || aFrame == mPrimaryFrame', file ../../dist/include/nsIContent.h, line 875

nsIContent::SetPrimaryFrame(nsIFrame*) [nsIContent.h:876]
nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) [layout/base/nsCSSFrameConstructor.cpp:3893]
nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) [layout/base/nsCSSFrameConstructor.cpp:5487]
nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) [layout/base/nsCSSFrameConstructor.cpp:9552]
nsCSSFrameConstructor::CreateListBoxContent(nsPresContext*, nsIFrame*, nsIFrame*, nsIContent*, nsIFrame**, int, int, nsILayoutHistoryState*) [layout/base/nsCSSFrameConstructor.cpp:10623]
nsListBoxBodyFrame::GetFirstItemBox(int, int*) [layout/xul/base/src/nsListBoxBodyFrame.cpp:1154]
nsListBoxBodyFrame::CreateRows() [layout/xul/base/src/nsListBoxBodyFrame.cpp:1021]
nsListBoxBodyFrame::OnContentInserted(nsPresContext*, nsIContent*) [layout/xul/base/src/nsListBoxBodyFrame.cpp:1349]
NotifyListBoxBody [layout/base/nsCSSFrameConstructor.cpp:6846]
nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, int, int, nsILayoutHistoryState*, int) [layout/base/nsCSSFrameConstructor.cpp:6933]
nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, int, nsILayoutHistoryState*, int) [layout/base/nsCSSFrameConstructor.cpp:6867]
nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, int) [layout/base/nsCSSFrameConstructor.cpp:9170]
nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&) [layout/base/nsCSSFrameConstructor.cpp:8036]
nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::css::RestyleTracker&, int) [layout/base/nsCSSFrameConstructor.cpp:8125]
mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) [layout/base/RestyleTracker.cpp:156]
mozilla::css::RestyleTracker::ProcessRestyles() [layout/base/RestyleTracker.cpp:216]
nsCSSFrameConstructor::ProcessPendingRestyles() [layout/base/nsCSSFrameConstructor.cpp:11693]
PresShell::FlushPendingNotifications(mozFlushType) [layout/base/nsPresShell.cpp:4785]
nsDocument::FlushPendingNotifications(mozFlushType) [content/base/src/nsDocument.cpp:6123]
nsBoxObject::GetPresShell(int) [layout/xul/base/src/nsBoxObject.cpp:185]
nsBoxObject::GetFrame(int) [layout/xul/base/src/nsBoxObject.cpp:149]
nsBoxObject::GetOffsetRect(nsIntRect&) [layout/xul/base/src/nsBoxObject.cpp:197]
nsBoxObject::GetHeight(int*) [layout/xul/base/src/nsBoxObject.cpp:299]
NS_InvokeByIndex_P [xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179]
CallMethodHelper::Invoke() [js/src/xpconnect/src/xpcwrappednative.cpp:3028]
CallMethodHelper::Call() [js/src/xpconnect/src/xpcwrappednative.cpp:2312]
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [js/src/xpconnect/src/xpcwrappednative.cpp:2276]
XPCWrappedNative::GetAttribute(XPCCallContext&) [js/src/xpconnect/src/xpcprivate.h:2565]
XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*) [js/src/xpconnect/src/xpcwrappednativejsops.cpp:1833]
js_Invoke [js/src/jsinterp.cpp:654]
js_InternalInvoke [js/src/jsinterp.cpp:694]
js_InternalGetOrSet [js/src/jsinterp.cpp:730]
JSScopeProperty::get(JSContext*, JSObject*, JSObject*, long*) [js/src/jsscope.h:992]
js_NativeGet [js/src/jsobj.cpp:4758]
js_GetPropertyHelper [js/src/jsobj.cpp:4928]
js_Interpret [js/src/jsops.cpp:1485]
js_Invoke [js/src/jsinterp.cpp:664]
js_InternalInvoke [js/src/jsinterp.cpp:694]
JS_CallFunctionValue [js/src/jsapi.cpp:4634]
nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) [dom/base/nsJSEnvironment.cpp:2204]
nsJSEventListener::HandleEvent(nsIDOMEvent*) [dom/src/events/nsJSEventListener.cpp:228]
nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsPIDOMEventTarget*, unsigned int, nsCxPusher*) [content/events/src/nsEventListenerManager.cpp:1094]
nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsPIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) [content/events/src/nsEventListenerManager.cpp:1191]
nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsPIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) [content/events/src/nsEventListenerManager.h:146]
nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, int, nsCxPusher*) [content/events/src/nsEventDispatcher.cpp:213]
nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, int, nsCxPusher*) [content/events/src/nsEventDispatcher.cpp:343]
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsPIDOMEventTarget>*) [content/events/src/nsEventDispatcher.cpp:628]
DocumentViewerImpl::LoadComplete(unsigned int) [layout/base/nsDocumentViewer.cpp:1038]
nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) [docshell/base/nsDocShell.cpp:5757]
nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) [docshell/base/nsDocShell.cpp:5637]
nsDocLoader::FireOnStateChange(nsIWebProgress*, nsIRequest*, int, unsigned int) [uriloader/base/nsDocLoader.cpp:1305]
nsDocLoader::doStopDocumentLoad(nsIRequest*, unsigned int) [uriloader/base/nsDocLoader.cpp:940]
nsDocLoader::DocLoaderIsEmpty(int) [uriloader/base/nsDocLoader.cpp:807]
nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) [uriloader/base/nsDocLoader.cpp:703]
nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, unsigned int) [netwerk/base/src/nsLoadGroup.cpp:680]
nsDocument::DoUnblockOnload() [content/base/src/nsDocument.cpp:6948]
nsDocument::UnblockOnload(int) [content/base/src/nsDocument.cpp:6886]
nsBindingManager::DoProcessAttachedQueue() [content/xbl/src/nsBindingManager.cpp:994]
nsRunnableMethodImpl<void (nsBindingManager::*)(), true>::Run() [nsThreadUtils.h:348]
nsThread::ProcessNextEvent(int, int*) [xpcom/threads/nsThread.cpp:547]
NS_ProcessPendingEvents_P(nsIThread*, unsigned int) [nsThreadUtils.cpp:200]
nsBaseAppShell::NativeEventCallback() [widget/src/xpwidgets/nsBaseAppShell.cpp:127]
nsAppShell::ProcessGeckoEvents(void*) [widget/src/cocoa/nsAppShell.mm:395]
CoreFoundation + 0x3f0fb
CoreFoundation + 0x3cbbf
CoreFoundation + 0x3c094
CoreFoundation + 0x3bec1
HIToolbox + 0x34f9c
HIToolbox + 0x34d51
HIToolbox + 0x34bd6
AppKit + 0x48a89
-AppKit + 0x482ca
-AppKit + 0xa55b
nsAppShell::Run() [widget/src/cocoa/nsAppShell.mm:747]
nsAppStartup::Run() [toolkit/components/startup/src/nsAppStartup.cpp:192]
XRE_main [toolkit/xre/nsAppRunner.cpp:3624]
main [browser/app/nsBrowserApp.cpp:158]
firefox-bin + 0x148e

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is a real bug caught by the assert!

We're reframing the first listitem due to the display change.  So we call nsListBoxBodyFrame::OnContentInserted for the first listitem, which calls GetFirstItemBox via nsListBoxBodyFrame::CreateRows.

Now the <hbox>'s frame is a child of the box frame for the listbox, and NOT a child of the nsListBoxBodyFrame.  So we find as our first kid the second listitem look up the content at its index - 1, find the <hbox> and decide to create frames for it.

We have primary frame checks in this situation in GetNextItemBox, as well as checks for the right parent frame, and checks for xul listitems, etc.  Weseem to have none of that in GetFirstItemBox.

Timothy, you want to take a look?

Comment 2

[Boris Zbarsky [:bzbarsky]]

And this is bad; it can leak frames.

Comment 3

[Timothy Nikkel (:tnikkel)]

I can take this, but not for a few weeks.

Comment 4

[Jesse Ruderman]

(In reply to comment #2)
> And this is bad; it can leak frames.

In the sense that a more complex testcase might trigger the "Some pres arena objects were not freed" assertion?  FWIW, I haven't seen that assertion in quite a while, so my fuzzer might be missing something.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> In the sense that a more complex testcase might trigger the "Some pres arena
> objects were not freed" assertion?

I'd think this testcase would trigger that, in fact....

Comment 6

[Jesse Ruderman]

It doesn't for me.

Comment 7

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

Should this be a security bug?

Comment 8

[Mats Palmgren (inactive)]

Attachment: Content tree in nsListBoxBodyFrame::GetFirstItemBox

"startContent" (red) is the content node that loses its existing frame

Comment 9

[Mats Palmgren (inactive)]

Attachment: fix v1

Do the same checks in nsListBoxBodyFrame::GetFirstItemBox as in GetNextItemBox.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 545639 [details] [diff] [review]
fix v1

r=me

Can you file a followup on adding a signature of IsXUL() that takes an nsIAtom like we have for HTML?

Comment 11

[Mats Palmgren (inactive)]

Filed bug 671847 for adding a signature of IsXUL().

http://hg.mozilla.org/integration/mozilla-inbound/rev/373edcdacf30

Comment 12

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/373edcdacf30