348304 - crash, access after delete: nsMenuFrame can be deleted during nsMenuFrame::HandleEvent

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Andrew Miller]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: CVS Trunk as of August 9th, also Mozilla 1.5.0.6

There are several ways this can happen, one of which can be consistently reproduced with my testcase.

Backtrace looks like this:

#5  <signal handler called>
#6  0x75160006 in ?? ()
#7  0xb4f6e53c in nsMenuFrame::OpenMenuInternal (this=0x89ce3f8, aActivateFlag=1)
    at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:760
#8  0xb4f6edfb in nsMenuFrame::OpenMenu (this=0x89ce3f8, aActivateFlag=1) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:733
#9  0xb4f6a972 in nsMenuFrame::ToggleMenuState (this=0x89ce3f8) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:544
#10 0xb4f6f771 in nsMenuFrame::HandleEvent (this=0x89ce3f8, aPresContext=0x8a52690, aEvent=0xbff16040, aEventStatus=0xbff15c6c)
    at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:404
#11 0xb4d6f3a5 in nsPresShellEventCB::HandleEvent (this=0xbff15d1c, aVisitor=@0xbff15c60)
    at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:1484
#12 0xb5079d83 in nsEventTargetChainItem::HandleEventTargetChain (this=0x8b61ae8, aVisitor=@0xbff15c60, aFlags=6, aCallback=0xbff15d1c)
    at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:476
#13 0xb507a339 in nsEventDispatcher::Dispatch (aTarget=0x838e048, aPresContext=0x8a52690, aEvent=0xbff16040, aDOMEvent=0x0, aEventStatus=0xbff15e68,
    aCallback=0xbff15d1c, aTargetIsChromeHandler=0) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:639
#14 0xb4d5f473 in PresShell::HandleEventInternal (this=0x8a57968, aEvent=0xbff16040, aView=0x8a52c10, aStatus=0xbff15e68)
    at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:6229
#15 0xb4d60294 in PresShell::HandlePositionedEvent (this=0x8a57968, aView=0x8a52c10, aTargetFrame=0x89ce3f8, aEvent=0xbff16040, aEventStatus=0xbff15e68)
    at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:6104
#16 0xb4d6073b in PresShell::HandleEvent (this=0x8a57968, aView=0x8a52c10, aEvent=0xbff16040, aEventStatus=0xbff15e68)
    at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:5932
#17 0xb51fc7f4 in nsViewManager::HandleEvent (this=0x8a52b98, aView=0x8a52c10, aPoint=@0xbff15f1c, aEvent=0xbff16040, aCaptured=0)
    at /home/amil082/mozilla_trunk/mozilla/view/src/nsViewManager.cpp:1662
#18 0xb520245b in nsViewManager::DispatchEvent (this=0x8a52b98, aEvent=0xbff16040, aStatus=0xbff15fac)
    at /home/amil082/mozilla_trunk/mozilla/view/src/nsViewManager.cpp:1618
#19 0xb51f68af in HandleEvent (aEvent=0xbff16040) at /home/amil082/mozilla_trunk/mozilla/view/src/nsView.cpp:171
#20 0xb5ef914b in nsCommonWidget::DispatchEvent (this=0x8a4f0f0, aEvent=0xbff16040, aStatus=@0xbff16094)


Reproducible: Always

Steps to Reproduce:
1. Visit my testcase.
2. Click on the menu.
Actual Results:  
Crashes due to accessing freed memory.

Expected Results:  
The menu should just disappear.

Comment 1

[Andrew Miller]

Attachment: Testcase

Comment 2

[Andrew Miller]

I am attempting a fix for trunk by making a "kungFuDeathGrip" strong reference before calling HandleEvent.

Comment 3

[Boris Zbarsky [:bzbarsky]]

kungFuDeathGrip to what?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is bug 279703 relevant here?

Comment 5

[Andrew Miller]

(In reply to comment #3)
> kungFuDeathGrip to what?

Okay, I see the problem, frames are not refcounted so another solution is needed.

Comment 6

[Andrew Miller]

For future reference:
The stack-trace leading up to deletion of the menu frame, when within the menu frame, is:

(gdb) bt
#0  0xb4fe30fa in ~nsMenuFrame (this=0x8b65a6c) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:233
#1  0xb4e28640 in nsFrame::Destroy (this=0x8b65a6c) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsFrame.cpp:652
#2  0xb4e979ee in nsSplittableFrame::Destroy (this=0x8b65a6c) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsSplittableFrame.cpp:73
#3  0xb4e166b7 in nsContainerFrame::Destroy (this=0x8b65a6c) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsContainerFrame.cpp:164
#4  0xb4fb24ee in nsBoxFrame::Destroy (this=0x8b65a6c) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1092
#5  0xb4fe2f31 in nsMenuFrame::Destroy (this=0x8b65a6c) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:354
#6  0xb4e2f674 in nsFrameList::DestroyFrames (this=0x8ad10a0) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsFrameList.cpp:60
#7  0xb4e16670 in nsContainerFrame::Destroy (this=0x8ad106c) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsContainerFrame.cpp:157
#8  0xb4fb24ee in nsBoxFrame::Destroy (this=0x8ad106c) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1092
#9  0xb4e2f674 in nsFrameList::DestroyFrames (this=0x8b63994) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsFrameList.cpp:60
#10 0xb4e16670 in nsContainerFrame::Destroy (this=0x8b63960) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsContainerFrame.cpp:157
#11 0xb4fb24ee in nsBoxFrame::Destroy (this=0x8b63960) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1092
#12 0xb4e3f718 in nsXULScrollFrame::Destroy (this=0x8b63960) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsGfxScrollFrame.cpp:1003
#13 0xb4e2f674 in nsFrameList::DestroyFrames (this=0x8ad0f40) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsFrameList.cpp:60
#14 0xb4e16670 in nsContainerFrame::Destroy (this=0x8ad0f0c) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsContainerFrame.cpp:157
#15 0xb4fb24ee in nsBoxFrame::Destroy (this=0x8ad0f0c) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1092
#16 0xb4fd820a in nsMenuPopupFrame::Destroy (this=0x8ad0f0c) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:1992
#17 0xb4e2f674 in nsFrameList::DestroyFrames (this=0x8ad02e8) at /home/amil082/mozilla_trunk/mozilla/layout/generic/nsFrameList.cpp:60
#18 0xb4fe2e2d in nsMenuFrame::DestroyPopupFrames (this=0x8ad0270, aPresContext=0x8291a60) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:325
#19 0xb4fe2f26 in nsMenuFrame::Destroy (this=0x8ad0270) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:353
#20 0xb4fb23d6 in nsBoxFrame::RemoveFrame (this=0x8af2a18, aListName=0x0, aOldFrame=0x8ad0270) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsBoxFrame.cpp:1148
#21 0xb4db8814 in nsFrameManager::RemoveFrame (this=0x8aa2f4c, aParentFrame=0x8af2a18, aListName=0x0, aOldFrame=0x8ad0270) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsFrameManager.cpp:681
#22 0xb4d76fad in nsCSSFrameConstructor::ContentRemoved (this=0x8aa2728, aContainer=0x8ad5460, aChild=0x8a876e8, aIndexInContainer=1, aInReinsertContent=0) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:10059
#23 0xb4d745d8 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x8aa2728, aContent=0x8a876e8) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:11646
#24 0xb4d74bb1 in nsCSSFrameConstructor::RestyleElement (this=0x8aa2728, aContent=0x8a876e8, aPrimaryFrame=0x8ad0270, aMinHint=0) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:10540
#25 0xb4d74e41 in nsCSSFrameConstructor::ProcessOneRestyle (this=0x8aa2728, aContent=0x8a876e8, aRestyleHint=eReStyle_Self, aChangeHint=0) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:13351
#26 0xb4d7505f in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x8aa2728) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp:13404
#27 0xb4ddee7d in PresShell::FlushPendingNotifications (this=0x8aa2f30, aType=Flush_Frames) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:5198
#28 0xb4fc8f96 in nsBoxObject::GetFrame (this=0x8abf9f4, aFlushLayout=0) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsBoxObject.cpp:133
#29 0xb4fc6c64 in nsMenuBoxObject::SetActiveChild (this=0x8abf9f0, aResult=0x8a87854) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuBoxObject.cpp:119
#30 0xb7da7885 in XPTC_InvokeByIndex () at xptiprivate.h:123
#31 0xb7125f72 in XPCWrappedNative::CallMethod (ccx=@0xbfe84f14, mode=XPCWrappedNative::CALL_SETTER) at /home/amil082/mozilla_trunk/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2162
#32 0xb71353cb in XPCWrappedNative::SetAttribute (ccx=@0xbfe84f14) at xpcprivate.h:1983
#33 0xb7134711 in XPC_WN_GetterSetter (cx=0x87b80a0, obj=0x8aab2f8, argc=1, argv=0x8b87248, vp=0xbfe85060) at /home/amil082/mozilla_trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1473
#34 0xb7e53fda in js_Invoke (cx=0x87b80a0, argc=1, flags=2) at /home/amil082/mozilla_trunk/mozilla/js/src/jsinterp.c:1350
#35 0xb7e54423 in js_InternalInvoke (cx=0x87b80a0, obj=0x8aab2f8, fval=146732144, flags=0, argc=1, argv=0xbfe85bc8, rval=0xbfe85bc8) at /home/amil082/mozilla_trunk/mozilla/js/src/jsinterp.c:1448
#36 0xb7e54763 in js_InternalGetOrSet (cx=0x87b80a0, obj=0x8aab2f8, id=145486616, fval=146732144, mode=JSACC_WRITE, argc=1, argv=0xbfe85bc8, rval=0xbfe85bc8) at /home/amil082/mozilla_trunk/mozilla/js/src/jsinterp.c:1508
#37 0xb7e8b581 in js_SetProperty (cx=0x87b80a0, obj=0x8aab2f8, id=145486616, vp=0xbfe85bc8) at /home/amil082/mozilla_trunk/mozilla/js/src/jsobj.c:3620
#38 0xb7e65cf6 in js_Interpret (cx=0x87b80a0, pc=0x88b8b8d "6", result=0xbfe85fac) at /home/amil082/mozilla_trunk/mozilla/js/src/jsinterp.c:3811
#39 0xb7e54061 in js_Invoke (cx=0x87b80a0, argc=1, flags=2) at /home/amil082/mozilla_trunk/mozilla/js/src/jsinterp.c:1369
#40 0xb7e54423 in js_InternalInvoke (cx=0x87b80a0, obj=0x8aab558, fval=146732096, flags=0, argc=1, argv=0x8b87218, rval=0xbfe861c4) at /home/amil082/mozilla_trunk/mozilla/js/src/jsinterp.c:1448
#41 0xb7e1731c in JS_CallFunctionValue (cx=0x87b80a0, obj=0x8aab558, fval=146732096, argc=1, argv=0x8b87218, rval=0xbfe861c4) at /home/amil082/mozilla_trunk/mozilla/js/src/jsapi.c:4418
#42 0xb52833c5 in nsJSContext::CallEventHandler (this=0x8671660, aTarget=0x8a876e8, aScope=0x8a58920, aHandler=0x8bef440, aargv=0x8ab8178, arv=0xbfe86324) at /home/amil082/mozilla_trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:1706
#43 0xb52f9761 in nsJSEventListener::HandleEvent (this=0x8a96080, aEvent=0x88afd00) at /home/amil082/mozilla_trunk/mozilla/dom/src/events/nsJSEventListener.cpp:211
#44 0xb5235a47 in nsXBLPrototypeHandler::ExecuteHandler (this=0x8b62340, aReceiver=0x88bfa10, aEvent=0x88afd00) at /home/amil082/mozilla_trunk/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:510
#45 0xb522f10c in nsXBLEventHandler::HandleEvent (this=0x8aa46f8, aEvent=0x88afd00) at /home/amil082/mozilla_trunk/mozilla/content/xbl/src/nsXBLEventHandler.cpp:84
#46 0xb50c00ca in nsEventListenerManager::HandleEventSubType (this=0x8aa4688, aListenerStruct=0x8aa4740, aListener=0x8aa46f8, aDOMEvent=0x88afd00, aCurrentTarget=0x8a876e8, aSubType=1, aPhaseFlags=2) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventListenerManager.cpp:1648
#47 0xb50c0c5c in nsEventListenerManager::HandleEvent (this=0x8aa4688, aPresContext=0x8291a60, aEvent=0xbfe86d58, aDOMEvent=0xbfe86b58, aCurrentTarget=0x8a876e8, aFlags=2, aEventStatus=0xbfe86b5c) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventListenerManager.cpp:1749
#48 0xb50ec7da in nsEventTargetChainItem::HandleEvent (this=0x8654d78, aVisitor=@0xbfe86b50, aFlags=2) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:355
#49 0xb50eccde in nsEventTargetChainItem::HandleEventTargetChain (this=0x8654ec8, aVisitor=@0xbfe86b50, aFlags=6, aCallback=0x0) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:454
#50 0xb50ed339 in nsEventDispatcher::Dispatch (aTarget=0x8a87780, aPresContext=0x8291a60, aEvent=0xbfe86d58, aDOMEvent=0x0, aEventStatus=0xbfe86db4, aCallback=0x0, aTargetIsChromeHandler=0) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:639
#51 0xb4dd3028 in PresShell::HandleDOMEventWithTarget (this=0x8aa2f30, aTargetContent=0x8a87780, aEvent=0xbfe86d58, aStatus=0xbfe86db4) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:6272
#52 0xb4fdf5ae in nsMenuFrame::OnCreate (this=0x8ad0270) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:1690
#53 0xb4fe14f0 in nsMenuFrame::OpenMenuInternal (this=0x8ad0270, aActivateFlag=1) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:750
#54 0xb4fe1dfb in nsMenuFrame::OpenMenu (this=0x8ad0270, aActivateFlag=1) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:733
#55 0xb4fdd972 in nsMenuFrame::ToggleMenuState (this=0x8ad0270) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:544
#56 0xb4fe2771 in nsMenuFrame::HandleEvent (this=0x8ad0270, aPresContext=0x8291a60, aEvent=0xbfe875a0, aEventStatus=0xbfe871cc) at /home/amil082/mozilla_trunk/mozilla/layout/xul/base/src/nsMenuFrame.cpp:404
#57 0xb4de23a5 in nsPresShellEventCB::HandleEvent (this=0xbfe8727c, aVisitor=@0xbfe871c0) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:1484
#58 0xb50ecd83 in nsEventTargetChainItem::HandleEventTargetChain (this=0x8654d48, aVisitor=@0xbfe871c0, aFlags=6, aCallback=0xbfe8727c) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:476
#59 0xb50ed339 in nsEventDispatcher::Dispatch (aTarget=0x8a876e8, aPresContext=0x8291a60, aEvent=0xbfe875a0, aDOMEvent=0x0, aEventStatus=0xbfe873c8, aCallback=0xbfe8727c, aTargetIsChromeHandler=0) at /home/amil082/mozilla_trunk/mozilla/content/events/src/nsEventDispatcher.cpp:639
#60 0xb4dd2473 in PresShell::HandleEventInternal (this=0x8aa2f30, aEvent=0xbfe875a0, aView=0x8ac63d8, aStatus=0xbfe873c8) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:6229
#61 0xb4dd3294 in PresShell::HandlePositionedEvent (this=0x8aa2f30, aView=0x8ac63d8, aTargetFrame=0x8ad0270, aEvent=0xbfe875a0, aEventStatus=0xbfe873c8) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:6104
#62 0xb4dd373b in PresShell::HandleEvent (this=0x8aa2f30, aView=0x8ac63d8, aEvent=0xbfe875a0, aEventStatus=0xbfe873c8) at /home/amil082/mozilla_trunk/mozilla/layout/base/nsPresShell.cpp:5932
#63 0xb526f7f4 in nsViewManager::HandleEvent (this=0x8ac6360, aView=0x8ac63d8, aPoint=@0xbfe8747c, aEvent=0xbfe875a0, aCaptured=0) at /home/amil082/mozilla_trunk/mozilla/view/src/nsViewManager.cpp:1662
#64 0xb527545b in nsViewManager::DispatchEvent (this=0x8ac6360, aEvent=0xbfe875a0, aStatus=0xbfe8750c) at /home/amil082/mozilla_trunk/mozilla/view/src/nsViewManager.cpp:1618
#65 0xb52698af in HandleEvent (aEvent=0xbfe875a0) at /home/amil082/mozilla_trunk/mozilla/view/src/nsView.cpp:171
#66 0xb5f6c14b in nsCommonWidget::DispatchEvent (this=0x8291c88, aEvent=0xbfe875a0, aStatus=@0xbfe875f4) at /home/amil082/mozilla_trunk/mozilla/widget/src/gtk2/nsCommonWidget.cpp:216
#67 0xb5f5ff99 in nsWindow::OnButtonPressEvent (this=0x8291c88, aWidget=0x816a610, aEvent=0x83ec8f8) at /home/amil082/mozilla_trunk/mozilla/widget/src/gtk2/nsWindow.cpp:1895
#68 0xb5f600a4 in button_press_event_cb (widget=0x816a610, event=0x83ec8f8) at /home/amil082/mozilla_trunk/mozilla/widget/src/gtk2/nsWindow.cpp:4197
#69 0xb7ae202c in _gtk_marshal_BOOLEAN__BOXED () from /usr/lib/libgtk-x11-2.0.so.0
#70 0xb77435c5 in IA__g_closure_invoke (closure=0x83a3788, return_value=0x8b65a6c, n_param_values=146168428, param_values=0x8b65a6c, invocation_hint=0x8b65a6c) at gclosure.c:490
#71 0xb775581f in signal_emit_unlocked_R (node=0x8146180, detail=0, instance=0x816a610, emission_return=0xbfe87810, instance_and_params=0xbfe87880) at gsignal.c:2438
#72 0xb77546ca in IA__g_signal_emit_valist (instance=0x816a610, signal_id=0, detail=0, var_args=0xbfe87a10 "(zè¿øÈ>\b\020¦\026\bAC¼·\020¦\026\b n\020\b") at gsignal.c:2207
#73 0xb7754b76 in IA__g_signal_emit (instance=0x8b65a6c, signal_id=146168428, detail=146168428) at gsignal.c:2241
#74 0xb7bc416f in gtk_widget_activate () from /usr/lib/libgtk-x11-2.0.so.0
#75 0xb7ae0767 in gtk_propagate_event () from /usr/lib/libgtk-x11-2.0.so.0
#76 0xb7ae0bc8 in gtk_main_do_event () from /usr/lib/libgtk-x11-2.0.so.0
#77 0xb796cb2d in _gdk_events_queue () from /usr/lib/libgdk-x11-2.0.so.0
#78 0xb76d9a72 in g_main_dispatch (context=0x807e690) at gmain.c:1916
#79 0xb76daae8 in IA__g_main_context_dispatch (context=0x807e690) at gmain.c:2466
#80 0xb76dae20 in g_main_context_iterate (context=0x807e690, block=1, dispatch=1, self=0x8117920) at gmain.c:2547
#81 0xb76db06d in IA__g_main_context_iteration (context=0x807e690, may_block=146168428) at gmain.c:2606
#82 0xb5f69429 in nsAppShell::ProcessNextNativeEvent (this=0x81445d8, mayWait=1) at /home/amil082/mozilla_trunk/mozilla/widget/src/gtk2/nsAppShell.cpp:144
#83 0xb5f86cf3 in nsBaseAppShell::DoProcessNextNativeEvent (this=0x81445d8, mayWait=1) at /home/amil082/mozilla_trunk/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:136
#84 0xb5f871e3 in nsBaseAppShell::OnProcessNextEvent (this=0x81445d8, thr=0x8080ec0, mayWait=1, recursionDepth=0) at /home/amil082/mozilla_trunk/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:231
#85 0xb7d90542 in nsThread::ProcessNextEvent (this=0x8080ec0, mayWait=1, result=0xbfe87cc0) at /home/amil082/mozilla_trunk/mozilla/xpcom/threads/nsThread.cpp:469
#86 0xb7d18a93 in NS_ProcessNextEvent_P (thread=0x8080ec0, mayWait=1) at nsThreadUtils.cpp:225
#87 0xb5f87392 in nsBaseAppShell::Run (this=0x81445d8) at /home/amil082/mozilla_trunk/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:153
#88 0xb5f0a7a7 in nsAppStartup::Run (this=0x81738d8) at /home/amil082/mozilla_trunk/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:171
#89 0xb7f11db3 in XRE_main (argc=1, argv=0xbfe88104, aAppData=0x80497e0) at /home/amil082/mozilla_trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2387
#90 0x0804862c in main (argc=1, argv=0x804a548) at /home/amil082/mozilla_trunk/mozilla/browser/app/nsBrowserApp.cpp:61

Comment 7

[Neil Deakin]

For trunk, the work I'm doing for bug 279703 will fix this anyway.

Comment 8

[[e-mail]]

Looks like smaug fixed this for nsPopupSetFrame.cpp in bug 343457.

Comment 9

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

I'm looking at this.
This crash is easy to fix, but I'm also trying to figure out if there could be
a safe (but ugly) way to fix 'all' similar crashes even without bug 279703.
Bug 279703 is definately needed for 1.9.

I have some code working now, but with a regression, so if I get that
running properly I'll post a patch, hopefully during this weekend.
If I don't get it running, I'll post anyway a patch to fix this particular
crash.

Or, enndeakin if you have anything we could use here, please tell us ;)

Comment 10

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

Attachment: an *ugly* patch

This looks terrible, but should fix crashes.
Includes 3 async calls, so in those cases functionality may have changed, though
I haven't seen any problems related to those. And the async calls really are
needed. SetAttr in Init() or DoLayout() is not good.

Still testing and thinking some better way to fix this for branches.

Comment 11

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

Attachment: file for a test case

Comment 12

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

Attachment: Testcase

Comment 13

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

Attachment: v1

This is still ugly as %&#¤, but helps in *many* cases.
I went trough everything in layout/xul and tried to catch all problematic cases. Possibly I've missed some. 
This patch should be safe otherwise except that those 3 async calls may
change something. I haven't seen any problems (in FF nor TB). (The
regression I saw in the early version of the patch was just an accidental removal of one line...)

Fortunately other parts of layout is better, not modifying dom in bad places.

[Someone really should have reviewed hyatt's code properly when he was writing XUL]

Comment 14

[Boris Zbarsky [:bzbarsky]]

So the async menu generation is almost certain to cause issues. :(  At the very least, look up the blame for various bugs (several different patches layered on each other) that led to the sync processing of menugenerated changes in nsCSSFrameConstructor::AtributeChanged and test with those testcases?

Similar issues with menuactive, actually.  :(

Comment 15

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

I tested Bug 262031, Bug 287616 and Bug 287616 and didn't see any regressions.
The patch doesn't change how AttributeChanged is handled. Weakframes are used
there just to prevent crashes.
Async calls change how initialization and reflow work.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Huh.  I seemed to recall that the problem was that code set menugenerated and then expected to immediately get info from the menu...  But if the bugs don't regress, that's great!

Can you also remove the frame constructor hack then?  Or is it still needed?

Comment 17

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

The hack added in Bug 262031 is still needed.

Comment 18

[Neil Deakin]

This patch doesn't seem to be changing to async menu generation, except in one case used only for <menulist>.

Comment 19

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

Neil D., you may have some testcases (for bug 279703). If you have, could you 
either test the patch or send testcases to me? Or perhaps attach them to  	bug 279703?

Comment 20

[Neil Deakin]

Comment on attachment 233787 [details] [diff] [review]
v1

>+  nsASyncMenuInitialization(nsIFrame* aFrame)
...
>+      menu->UpdateMenuType(mFrame->GetPresContext());
>+      if (mFrame) {
>+        menu->BuildAcceleratorText();

I think you meant to use:
  if (mFrame->IsAlive())

Also nsWeakFrame doesn't seem to have a GetPresContext() member from what I can see.


> nsMenuFrame::~nsMenuFrame()
...
>+  nsWeakFrame weakFrame(this);

Will the weak frame stuff work or be useful while in the destructor for what we're holding a weak pointer too?

> NS_IMETHODIMP 
> nsMenuFrame::HandleEvent(nsPresContext* aPresContext, 
>                              nsGUIEvent*     aEvent,
>                              nsEventStatus*  aEventStatus)
> {
>   NS_ENSURE_ARG_POINTER(aEventStatus);
>+  nsWeakFrame weakFrame(this);

Only two of the weakFrame.IsAlive calls you added in this function are needed, as the others are all at the end of the conditional blocks.

> NS_IMETHODIMP
> nsMenuFrame::ToggleMenuState()
>+      NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);
>       mMenuParent->SetActive(PR_FALSE);
>+      NS_ENSURE_TRUE(weakFrame.IsAlive(), NS_OK);

Note that mMenuParent is also a pointer to a frame.

>   UpdateMenuSpecialState(aPresContext);

What about the UnSetAttr call in UpdateMenuSpecialState?

> PRBool
> nsMenuFrame::SizeToPopup(nsBoxLayoutState& aState, nsSize& aSize)
>-        MarkAsGenerated();
>-        frame = mPopupFrames.FirstChild();
>-        // No child - just return
>-        if (!frame) return PR_FALSE;
>+        nsCOMPtr<nsIContent> child;
>+        GetMenuChildrenElement(getter_AddRefs(child));
>+        if (child &&
>+            !nsContentUtils::HasNonEmptyAttr(child, kNameSpaceID_None,
>+                                             nsXULAtoms::menugenerated)) {
>+          nsCOMPtr<nsIRunnable> ev =
>+            new nsASyncMenuGeneration(this);
>+          NS_DispatchToCurrentThread(ev);
>+        }
>+        return PR_FALSE;
>       }

Make sure to test menulists for this change as it switches to asynchronous generation. The menulist should appear as wide as the popup's contents. Make sure that http://www.xulplanet.com/ndeakin/tests/xts/menulist/menulist-append-toempty-select.xul
still resizes the menu button even without ever opening the popup.

>+  nsWeakFrame weakFrame(this);
>   mContent->SetAttr(kNameSpaceID_None, nsXULAtoms::left, left, PR_FALSE);
>+  if (!weakFrame.IsAlive()) {
>+    return;
>+  }
>   mContent->SetAttr(kNameSpaceID_None, nsXULAtoms::top, top, PR_FALSE);
>+  if (!weakFrame.IsAlive()) {
>+    return;
>+  }
> 
>+  MoveToInternal(aLeft, aTop);
>+}
>+

We should find a way to not have this move happen twice, but not for this bug.

Comment 21

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

(In reply to comment #20)
> (From update of attachment 233787 [details] [diff] [review] [edit])
> >+  nsASyncMenuInitialization(nsIFrame* aFrame)
> ...
> >+      menu->UpdateMenuType(mFrame->GetPresContext());
> >+      if (mFrame) {
> >+        menu->BuildAcceleratorText();
> 
> I think you meant to use:
>   if (mFrame->IsAlive())
> 

if (mFrame)  is the same as if (mFrame.IsAlive())

and mFrame->GetPresContext()  is the same as mFrame->GetFrame()->GetPresContext().
Though, my bad that I'm using both styles.

> 
> > nsMenuFrame::~nsMenuFrame()
> ...
> >+  nsWeakFrame weakFrame(this);
> 
> Will the weak frame stuff work or be useful while in the destructor for what
> we're holding a weak pointer too?

That weakframe is in ::Destroy(), not in destructor

Comment 22

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

(In reply to comment #20)> 
> What about the UnSetAttr call in UpdateMenuSpecialState?
> 

There is return right after that.

Comment 23

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

Attachment: v2

+comments and tested http://www.xulplanet.com/ndeakin/tests/xts/menulist/menulist-append-toempty-select.xul

Comment 24

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

(In reply to comment #20)
> 
> Note that mMenuParent is also a pointer to a frame.
> 

For mMenuParent I added just NS_ENSURE_TRUE, since weakFrame is in those cases
anyway alive, so mMenuParent should point to a valid frame or nsnull.

Comment 25

[Neil Deakin]

Comment on attachment 234495 [details] [diff] [review]
v2

> nsSplitterFrameInner::MouseDrag
>-            mOuter->mContent->SetAttr(kNameSpaceID_None, nsXULAtoms::substate,
>+            nsCOMPtr<nsIContent> outer = mOuter->mContent;
>+            outer->SetAttr(kNameSpaceID_None, nsXULAtoms::substate,
>                                       NS_LITERAL_STRING("after"),
>                                       PR_TRUE);

Make sure that all four of these SetAttr calls are indented properly.

> void
> nsTreeBodyFrame::InvalidateScrollbars(const ScrollParts& aParts)
aParts.mColumnsScrollableView->View()->GetBounds();
>-    nsIContent* scrollbar = aParts.mHScrollbarContent;
>+    

Remove this extra spacing.


Is there a plan for fixing this issue in a better way, so we don't need all these weak frame checks?

Comment 26

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

(In reply to comment #25)
> 
> Is there a plan for fixing this issue in a better way, so we don't need all
> these weak frame checks?
>
Bug 279703, right? ;)
(In which I guess most of the popup/menu handling should be moved from frames to
boxobjects) 

Comment 27

[Neil Deakin]

(In reply to comment #26)
> > Is there a plan for fixing this issue in a better way, so we don't need all
> > these weak frame checks?
> >
> Bug 279703, right? ;)

Yes, but I meant for the other non-popup files you changed in this patch.

Comment 28

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

(In reply to comment #27)
> Yes, but I meant for the other non-popup files you changed in this patch.
> 
I'm not aware of any such plan. Should we move all code which
modifies dom to boxobjects. That way we keep the flexibility
of not having everything in dom elements (== not having specific
XUL element implementations) but since boxobjects are refcounted it is easier to
handle situations when display type changes or frame gets deleted...
Perhaps something to design/implement for 1.9.

Comment 29

[Neil Deakin]

I think there is general interest in reducing the code used in box objects instead. Note also that the box object you get for a particular content node isn't currently related to the type of frame created for it.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 234495 [details] [diff] [review]
v2

Jonas, could you sr= this one?

Comment 31

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

IMHO the special behaviour of certain XUL elements should be placed in content. Either we should have different classes of XUL elements (preferred) or we should allow XUL elements to delegate some behaviour to an inner box-object-like object.

IMHO the user-visible box objects that we currently have are an unnecessary complication of the API.

Comment 32

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

In particular all this menu code should live in content and be associated with menu elements.

Comment 33

[[e-mail]]

(In reply to comment #32)
>In particular all this menu code should live in content and be associated with
>menu elements.
That's unworkable. <button>s and <toolbarbutton>s are "menu elements" if they have type="menu" or type="menu-button", while <menulist>s are "menu elements" unless they have editable="true". Oh, and there's something called an iconic statusbar panel which is also a "menu element".

Comment 34

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

+class nsASyncMenuInitialization : public nsRunnable

I think here it would be better to hold a reference to the XUL element and use GetPrimaryFrameFor to re-get the frame when it's needed.

Comment 35

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

That is a sad API.

Well then, put the menu behaviour in a C++ object that hangs off the XUL element (as a content property?).

Comment 36

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

+class nsASyncMenuGeneration : public nsRunnable

Same here, use a strong content reference and GetPrimaryFrameFor.

Let's avoid nsWeakFrame usage across event dispatch.

Comment 37

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

(In reply to comment #34)
> +class nsASyncMenuInitialization : public nsRunnable
> 
> I think here it would be better to hold a reference to the XUL element and use
> GetPrimaryFrameFor to re-get the frame when it's needed.
> 

Can't do it here, because the frame must be nsMenuFrame (which is not
an interface to which one could QI)

Comment 38

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

Call GetType() to check and then do a static cast to nsMenuFrame?

Comment 39

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

The rest looks OK.

Comment 40

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

There isn't GetType in nsMenuFrame

Comment 41

[Neil Deakin]

Note that my work for bug 279703 removes a lot of code from the menu related frames and moves it into a separate singleton object, and actually makes this bug unnecessary.

Comment 42

[Boris Zbarsky [:bzbarsky]]

You could GetPrimaryFrameFor, QI to nsIMenuFrame, if that succeeds cast to nsMenuFrame, no?

Comment 43

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

(In reply to comment #42)
> You could GetPrimaryFrameFor, QI to nsIMenuFrame, if that succeeds cast to
> nsMenuFrame, no?
> 

Ah, true. Only nsMenuFrame implements nsIMenuFrame.

Comment 44

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

Or implement GetType.

Comment 45

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

Or expose what you need through nsIMenuFrame.

Comment 46

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

(In reply to comment #45)
> Or expose what you need through nsIMenuFrame.
> 

Can't change interfaces.
I'll QI to nsIMenuFrame and cast to nsMenuFrame.

Comment 47

[Daniel Veditz [:dveditz]]

Too late to get this one into 1.8.0.7 -- please land on trunk (after reviews, of course) and we'll try for 1.8.0.8

Comment 48

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

Comment on attachment 234495 [details] [diff] [review]
v2

sr=dbaron, although I hope we come up with a better solution to these problems in the long term

Comment 49

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

Attachment: checked in

Comment 50

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

Attachment: Fixes keyboard shorcuts on menus

Arg, don't know how I missed this.
aNotify must not be true when BuildAcceleratorText is called in 
AttributeChanged. That causes endless loop. (Actually it should be true also there, but that is different bug, and not related to this one.)

aNotify must be true when called in nsASyncMenuInitialization so that it causes
a new reflow for the frame.

Comment 51

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

What patch caused this problem?  I don't see this code being touched in the previous patch.

Comment 52

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

nsASyncMenuInitialization is the reason.
Part of the menu initialization must be done async, since it causes
some DOM mutations :(

Comment 53

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

Attachment: Fixes keyboard shorcuts on menus, v2

This is safer. BuildAcceleratorText() can be called synchronously since it doesn't
notify content changes. nsASyncMenuInitialization needs to be used only for UpdateMenuType().

Comment 54

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

Attachment: for collapsed menus

And this fixes bug 351323. Simple fix but took too long to find out :(

Comment 55

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

(In reply to comment #54)
> Created an attachment (id=236988) [edit]
> for collapsed menus
> 
> And this fixes bug 351323. Simple fix but took too long to find out :(
> 

The idea in this patch is that menu doesn't get marked generated if it is collapsed, since otherwise the size won't be the right one.
Menu will be marked generated whenever coming back to normal visibility state.

Comment 56

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

Comment on attachment 236988 [details] [diff] [review]
for collapsed menus

r+sr=dbaron, although you could put the IsCollapsed check before the CallQueryInterface and avoid the ?:.

Comment 57

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

Checked in 
https://bugzilla.mozilla.org/attachment.cgi?id=236780 and
https://bugzilla.mozilla.org/attachment.cgi?id=236988.

Comment 58

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

Attachment: for 1.8.0. (contains regression fixes)

Comment 59

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

Attachment: for 1.8 (contains regression fixes)

Comment 60

[Boris Zbarsky [:bzbarsky]]

I think this caused bug 351909.

Comment 61

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

Attachment: to fix regression bug 351909

Bz, this is what you suggested in bug 351909.
Adding mDocument member to make sure that block and unblock are always
called for the same document

Comment 62

[Daniel Veditz [:dveditz]]

drivers are confused by the lack of approval requests despite the presence of branch-specific patches. Can you nominate the appropriate patches please?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237717 [details] [diff] [review]
to fix regression bug 351909

>Index: layout/xul/base/src/nsMenuFrame.cpp
>+    nsIPresShell* shell = doc ? doc->GetShellAt(0) : shell;

s/shell;/nsnull;/

>+    nsIFrame* frame = shell ? shell->GetPrimaryFrameFor(mContent) : nsnull;

This is ok, but it'd make more sense to me to save the current frame on construction of the event and here only do stuff if we still have the same frame...

With that, looks good.  Either way on the frame thing, but fix the shell/nsnull typo.

Comment 64

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

Comment on attachment 237717 [details] [diff] [review]
to fix regression bug 351909

Checked in.

I'll update the branch patches and ask approval.

Comment 65

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

Attachment: for 1.8.0. (all contains regression fixes)

David, could you check that PLEvent firing is ok to you.
Otherwise the changes to trunk patch are smaller. Mainly just removing
some parts (for example related to trees) because those aren't in branches.

Comment 66

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

Attachment: for 1.8. (all contains regression fixes)

Almost identical to 1.8.0 patch.

Comment 67

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

Comment on attachment 237951 [details] [diff] [review]
for 1.8.0. (all contains regression fixes)

>+struct nsASyncMenuInitialization : public PLEvent
>+{
>+  nsASyncMenuInitialization(nsIContent* aContent)
>+    : mContent(aContent)
>+  {
>+  }
>+
>+  void HandleEvent() {
>+    nsIDocument* doc = mContent ? mContent->GetCurrentDoc() : nsnull;
>+    ENSURE_TRUE(doc);
>+
>+    nsIPresShell* shell = doc->GetShellAt(0);
>+    ENSURE_TRUE(shell);

I think the pres shell probably needs to be a member variable of this event as well, since we could have menu frames created in a non-first shell.  That said, can you instead make an nsWeakFrame member of the event (instead of both content and pres shell) and not bother with all this?

>+    nsIFrame* frame = nsnull;
>+    shell->GetPrimaryFrameFor(mContent, &frame);
>+    ENSURE_TRUE(frame);
>+
>+    nsIMenuFrame* imenu = nsnull;
>+    CallQueryInterface(frame, &imenu);
>+    ENSURE_TRUE(imenu);
>+
>+    nsMenuFrame* menu = NS_STATIC_CAST(nsMenuFrame*, imenu);
>+    menu->UpdateMenuType(menu->GetPresContext());
>+  }
>+

private?

>+  nsCOMPtr<nsIContent> mContent;
>+};

>+static void PR_CALLBACK HandleASyncMenuInitialization(nsASyncMenuInitialization* aEvent)
>+{
>+  aEvent->HandleEvent();
>+}

Make this take a PLEvent* and put the cast inside, using NS_STATIC_CAST.  It also needs to return a void* and should return null.

>+static void PR_CALLBACK DestroyASyncMenuInitialization(nsASyncMenuInitialization* aEvent)
>+{
>+  delete aEvent;
>+}

Likewise.

>+      PL_InitEvent(initialization, nsnull,
>+                   (PLHandleEventProc) ::HandleASyncMenuInitialization,
>+                   (PLDestroyEventProc) ::DestroyASyncMenuInitialization);

Please don't cast function pointers.  It's quite dangerous.

>+      if (NS_FAILED(eventQueue->PostEvent(initialization))) {
>+        PL_DestroyEvent(initialization);
>+      }
>+    }
>+  }
>   return rv;
> }


FWIW, I also checked that nsMenuFrame:
 * can't be constructed with a scrollframe around it (mayBeScrollable is false in ConstructXULFrame)
 * is added to the primary frame map (like everything in ConstructXULFrame)


r=dbaron with those changes (I only looked at the PLEvent code)

Comment 68

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

(And I realize a few of those comments apply to what landed on the trunk as well.  Could you make a supplementary trunk patch?) 

Comment 69

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

(In reply to comment #36)
> +class nsASyncMenuGeneration : public nsRunnable
> 
> Same here, use a strong content reference and GetPrimaryFrameFor.
> 
> Let's avoid nsWeakFrame usage across event dispatch.

Roc, for what it's worth, I just gave smaug the opposite advice, at the very least because these elements could be in a printed document.  Any objections?

Comment 70

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

Attachment: for 1.8.0

Comment 71

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

Attachment: for 1.8

Comment 72

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

(In reply to comment #69)
> Roc, for what it's worth, I just gave smaug the opposite advice, at the very
> least because these elements could be in a printed document.  Any objections?

No, you have a good point.

Comment 73

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

Attachment: for trunk changing mContent to mWeakFrame

Branch patches have this already.

Comment 74

[Mike Schroepfer]

Given how close we are to RC1 for FF2 - we are going to push to 1.8.1.1 to make sure we have enough bake time.   

Comment 75

[Mike Schroepfer]

Hmm - given that this also fixes 351909 should we reconsider?  Can we get a better assessment of risk for this patch?

Comment 76

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

(In reply to comment #75)
> Hmm - given that this also fixes 351909 should we reconsider?  Can we get a
> better assessment of risk for this patch?
> 

Bug 351909 was a regression from this bug and only in trunk.
But whenever this bug gets fixed also on branches, it must be
checked that Bug 351909 is not there again. 

Comment 77

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 78

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

Note to myself, haven't seen any new regressions and went through all
menu related bugs which were filed after 2006-09-03. None of them
looked like a regression from this.

Comment 79

[Daniel Veditz [:dveditz]]

Why is this not marked "fixed" on trunk? Forgot to update bug? forgot to check-in reviewed patch? waiting for other fixes?

We'd like this fix in the next branch security/stability updates

Comment 80

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

Ah, sorry. This is fixed on trunk and I haven't heard any new regressions.

Comment 81

[Daniel Veditz [:dveditz]]

Comment on attachment 238169 [details] [diff] [review]
for 1.8.0

Since an earlier version got a sr-minus from dbaron I'd like him to look this over and give his OK that 1) his concerns were addressed, and 2) it's reasonable and safe for a security release

Comment 82

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

Comment on attachment 238169 [details] [diff] [review]
for 1.8.0

Yep, these look fine (although it might be nice to make some of the member variables private).

Comment 83

[Daniel Veditz [:dveditz]]

Comment on attachment 238170 [details] [diff] [review]
for 1.8

approved for 1.8/1.8.0 branches, a=dveditz for drivers.

Please land soon so we've got bake-time on these fairly large patches.

Comment 84

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

fixed1.8.1.1, fixed1.8.0.9

Comment 85

[Jay Patel [:jay]]

v.fixed on Trunk/1.9, as well as 1.8.0 and 1.8.1 branches, with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20061128 Minefield/3.0a1
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre 
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

No crash with second testcase (XUL menu just disappears as expected).

Comment 86

[Michael Ventnor]

This has caused a Mac regression regarding preference menu flickering: Bug 365018