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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
XUL
--
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Andrew Miller, Assigned: smaug)

Tracking

({crash, verified1.8.0.9, verified1.8.1.1})

Trunk
mozilla1.8.1
x86
Linux
crash, verified1.8.0.9, verified1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -
blocking1.8.1.1 +
blocking1.8.0.7 -
blocking1.8.0.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(11 attachments, 7 obsolete attachments)

892 bytes, application/vnd.mozilla.xul+xml
Details
2.53 KB, application/vnd.mozilla.xul+xml
Details
239 bytes, text/html
Details
v2
63.99 KB, patch
Neil Deakin
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
63.91 KB, patch
Details | Diff | Splinter Review
2.61 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
973 bytes, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
2.20 KB, patch
Details | Diff | Splinter Review
58.44 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review
58.51 KB, patch
dbaron
: superreview+
Mike Schroepfer
: approval1.8.1-
Details | Diff | Splinter Review
4.58 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 233197 [details]
Testcase
(Reporter)

Comment 2

11 years ago
I am attempting a fix for trunk by making a "kungFuDeathGrip" strong reference before calling HandleEvent.
kungFuDeathGrip to what?
Flags: blocking1.9?
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?
Is bug 279703 relevant here?
Depends on: 279703
(Reporter)

Comment 5

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

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

Comment 6

11 years ago
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

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

Comment 8

11 years ago
Looks like smaug fixed this for nsPopupSetFrame.cpp in bug 343457.
(Assignee)

Comment 9

11 years ago
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 ;)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 10

11 years ago
Created attachment 233564 [details] [diff] [review]
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.

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Assignee: nobody → Olli.Pettay
Flags: blocking1.8.0.7? → blocking1.8.0.7+
(Assignee)

Comment 11

11 years ago
Created attachment 233778 [details]
file for a test case
(Assignee)

Comment 12

11 years ago
Created attachment 233779 [details]
Testcase
(Assignee)

Comment 13

11 years ago
Created attachment 233787 [details] [diff] [review]
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]
Attachment #233564 - Attachment is obsolete: true
Attachment #233787 - Flags: review?(roc)
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.  :(
(Assignee)

Comment 15

11 years ago
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.
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?
(Assignee)

Comment 17

11 years ago
The hack added in Bug 262031 is still needed.

Comment 18

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

Comment 19

11 years ago
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?

Updated

11 years ago
Target Milestone: --- → mozilla1.8.1
Attachment #233787 - Flags: superreview?(roc)
Attachment #233787 - Flags: review?(roc)
Attachment #233787 - Flags: review?(enndeakin)

Comment 20

11 years ago
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.
Attachment #233787 - Flags: review?(enndeakin) → review-
(Assignee)

Comment 21

11 years ago
(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

(Assignee)

Comment 22

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

There is return right after that.
(Assignee)

Comment 23

11 years ago
Created attachment 234495 [details] [diff] [review]
v2

+comments and tested http://www.xulplanet.com/ndeakin/tests/xts/menulist/menulist-append-toempty-select.xul
Attachment #233787 - Attachment is obsolete: true
Attachment #234495 - Flags: superreview?(roc)
Attachment #234495 - Flags: review?(enndeakin)
Attachment #233787 - Flags: superreview?(roc)
(Assignee)

Comment 24

11 years ago
(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.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 25

11 years ago
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?
Attachment #234495 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 26

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 28

11 years ago
(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

11 years ago
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.
Whiteboard: [sg:critical?] need sr=sicking
Comment on attachment 234495 [details] [diff] [review]
v2

Jonas, could you sr= this one?
Attachment #234495 - Flags: superreview?(roc) → superreview?(bugmail)
Attachment #234495 - Flags: superreview?(bugmail) → superreview?(dbaron)
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.
In particular all this menu code should live in content and be associated with menu elements.

Comment 33

11 years ago
(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".
+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.
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?).
+class nsASyncMenuGeneration : public nsRunnable

Same here, use a strong content reference and GetPrimaryFrameFor.

Let's avoid nsWeakFrame usage across event dispatch.
(Assignee)

Comment 37

11 years ago
(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)

Call GetType() to check and then do a static cast to nsMenuFrame?
The rest looks OK.
(Assignee)

Comment 40

11 years ago
There isn't GetType in nsMenuFrame

Comment 41

11 years ago
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.
You could GetPrimaryFrameFor, QI to nsIMenuFrame, if that succeeds cast to nsMenuFrame, no?
(Assignee)

Comment 43

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

Ah, true. Only nsMenuFrame implements nsIMenuFrame.
Whiteboard: [sg:critical?] need sr=sicking → [sg:critical?] need sr=dbaron
Or implement GetType.
Or expose what you need through nsIMenuFrame.
(Assignee)

Comment 46

11 years ago
(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.
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
Flags: blocking1.8.0.8+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
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
Attachment #234495 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 49

11 years ago
Created attachment 236646 [details] [diff] [review]
checked in
(Assignee)

Updated

11 years ago
Depends on: 351318
(Assignee)

Updated

11 years ago
Depends on: 351323
(Assignee)

Comment 50

11 years ago
Created attachment 236776 [details] [diff] [review]
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.
Attachment #236776 - Flags: superreview?(dbaron)
Attachment #236776 - Flags: review?(dbaron)
What patch caused this problem?  I don't see this code being touched in the previous patch.
(Assignee)

Comment 52

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

Comment 53

11 years ago
Created attachment 236780 [details] [diff] [review]
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().
Attachment #236776 - Attachment is obsolete: true
Attachment #236780 - Flags: superreview?(dbaron)
Attachment #236780 - Flags: review?(dbaron)
Attachment #236776 - Flags: superreview?(dbaron)
Attachment #236776 - Flags: review?(dbaron)
(Assignee)

Comment 54

11 years ago
Created attachment 236988 [details] [diff] [review]
for collapsed menus

And this fixes bug 351323. Simple fix but took too long to find out :(
Attachment #236988 - Flags: superreview?(dbaron)
Attachment #236988 - Flags: review?(dbaron)
(Assignee)

Comment 55

11 years ago
(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 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 ?:.
Attachment #236988 - Flags: superreview?(dbaron)
Attachment #236988 - Flags: superreview+
Attachment #236988 - Flags: review?(dbaron)
Attachment #236988 - Flags: review+
Attachment #236780 - Flags: superreview?(dbaron)
Attachment #236780 - Flags: superreview+
Attachment #236780 - Flags: review?(dbaron)
Attachment #236780 - Flags: review+
Whiteboard: [sg:critical?] need sr=dbaron → [sg:critical?]
(Assignee)

Comment 57

11 years ago
Checked in 
https://bugzilla.mozilla.org/attachment.cgi?id=236780 and
https://bugzilla.mozilla.org/attachment.cgi?id=236988.
(Assignee)

Comment 58

11 years ago
Created attachment 237186 [details] [diff] [review]
for 1.8.0. (contains regression fixes)
(Assignee)

Comment 59

11 years ago
Created attachment 237188 [details] [diff] [review]
for 1.8 (contains regression fixes)
Depends on: 351909
I think this caused bug 351909.
Keywords: crash
(Assignee)

Comment 61

11 years ago
Created attachment 237717 [details] [diff] [review]
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
Attachment #237717 - Flags: superreview?(bzbarsky)
Attachment #237717 - Flags: review?(bzbarsky)
drivers are confused by the lack of approval requests despite the presence of branch-specific patches. Can you nominate the appropriate patches please?
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.
Attachment #237717 - Flags: superreview?(bzbarsky)
Attachment #237717 - Flags: superreview+
Attachment #237717 - Flags: review?(bzbarsky)
Attachment #237717 - Flags: review+
(Assignee)

Comment 64

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

Checked in.

I'll update the branch patches and ask approval.
(Assignee)

Comment 65

11 years ago
Created attachment 237951 [details] [diff] [review]
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.
Attachment #237186 - Attachment is obsolete: true
Attachment #237951 - Flags: superreview?(dbaron)
Attachment #237951 - Flags: approval1.8.0.8?
(Assignee)

Comment 66

11 years ago
Created attachment 237952 [details] [diff] [review]
for 1.8. (all contains regression fixes)

Almost identical to 1.8.0 patch.
(Assignee)

Updated

11 years ago
Attachment #237188 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #237952 - Flags: approval1.8.1?
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)
Attachment #237951 - Flags: superreview?(dbaron) → superreview-
(And I realize a few of those comments apply to what landed on the trunk as well.  Could you make a supplementary trunk patch?) 
(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?
(Assignee)

Comment 70

11 years ago
Created attachment 238169 [details] [diff] [review]
for 1.8.0
Attachment #237951 - Attachment is obsolete: true
Attachment #238169 - Flags: approval1.8.0.8?
Attachment #237951 - Flags: approval1.8.0.8?
(Assignee)

Comment 71

11 years ago
Created attachment 238170 [details] [diff] [review]
for 1.8
Attachment #237952 - Attachment is obsolete: true
Attachment #238170 - Flags: approval1.8.1?
Attachment #237952 - Flags: approval1.8.1?
(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.

(Assignee)

Comment 73

11 years ago
Created attachment 238175 [details] [diff] [review]
for trunk changing mContent to mWeakFrame

Branch patches have this already.
Attachment #238175 - Flags: superreview?(dbaron)
Attachment #238175 - Flags: review?(dbaron)
Attachment #238175 - Flags: superreview?(dbaron)
Attachment #238175 - Flags: superreview+
Attachment #238175 - Flags: review?(dbaron)
Attachment #238175 - Flags: review+

Comment 74

11 years ago
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.   
Flags: blocking1.8.1.1?
Flags: blocking1.8.1-
Flags: blocking1.8.1+

Updated

11 years ago
Attachment #238170 - Flags: approval1.8.1? → approval1.8.1-

Comment 75

11 years ago
Hmm - given that this also fixes 351909 should we reconsider?  Can we get a better assessment of risk for this patch?
Flags: blocking1.8.1- → blocking1.8.1?
(Assignee)

Comment 76

11 years ago
(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. 

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1-
Restoring lost blocking flag
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.8? → blocking1.8.0.9?
(Assignee)

Comment 78

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #238170 - Flags: approval1.8.1.1?
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
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
(Assignee)

Comment 80

11 years ago
Ah, sorry. This is fixed on trunk and I haven't heard any new regressions.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Attachment #238170 - Flags: superreview?
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
Attachment #238169 - Flags: superreview?(dbaron)
Attachment #238170 - Flags: superreview? → superreview?(dbaron)
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).
Attachment #238169 - Flags: superreview?(dbaron) → superreview+
Attachment #238170 - Flags: superreview?(dbaron) → superreview+
Attachment #238169 - Flags: approval1.8.0.9? → approval1.8.0.9+
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.
Attachment #238170 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Comment 84

11 years ago
fixed1.8.1.1, fixed1.8.0.9
Keywords: fixed1.8.0.9, fixed1.8.1.1

Comment 85

11 years ago
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).
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security

Updated

11 years ago
Depends on: 365018

Comment 86

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

Updated

10 years ago
Depends on: 367988
Depends on: 368501
Flags: blocking1.9? → in-testsuite?

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.