Closed Bug 64025 Opened 25 years ago Closed 24 years ago

[PATCH]Crash when changing the Show Tooltips pref

Categories

(SeaMonkey :: Preferences, defect, P1)

Product:
SeaMonkey
Component:
Preferences
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: stdowa+bugzilla, Assigned: attinasi)

Details

(Keywords: crash)

Attachments

(5 files)

bt
18.86 KB, text/plain
Details
Possible fix for this crash, wfm on linux
760 bytes, patch
Details | Diff | Splinter Review
6 stack traces; 4 of one, 1 of another, 1 of a third.
12.33 KB, text/plain
Details
Patch that makes mShowTooltips static and only registers the pref change callback once.
4.21 KB, patch
Details | Diff | Splinter Review
New Patch: addresses reviewers comments and calls into prefs less often
3.08 KB, patch
Details | Diff | Splinter Review
Using the 2000122904 nightly build, i crash after changing the Show Tooltips option twice. Steps to reproduce: 1. Open prefs & change the Show Tooltips option under Appearance. 2. Open a new Navigator by pressing Ctrl+N or Tasks -> Navigator. 3. Close the Navigator window you just opened. 4. Open prefs & change the Show Tooltips option under Appearance again. Mozilla repeatedly crashes after performing the above steps. "*** FIX ME: '_elementIDs' in 'pref-appearance.xul' contains a reference to a non-existent element ID 'undefined'." is printed to the console after changing the Show Tooltips option each time.
Adding crash keyword
Keywords: crash
this is pink's, not mine...
Assignee: blakeross → pinkerton
Component: Preferences → XP Toolkit/Widgets: Menus
QA Contact: sairuh → jrgm
The '*** FIX ME' error message turns out to be a red herring, and I've reopened bug 60005 to get it cleaned up. However, even with that correction in place, I still get the crash as described with the steps to reproduce in the initial bug report.
This is the stack trace that I get with a current build on win2k. I don't think this is Mike's, but I am a bear of little brain. js_hash_scope_slot_invalidator(JSHashEntry * 0x03e2d3d8, int 83, void * 0x00000000) line 217 + 3 bytes JS_HashTableEnumerateEntries(JSHashTable * 0x03c226a8, int (JSHashEntry *, int, void *)* 0x002f52b1 js_hash_scope_slot_invalidator(JSHashEntry *, int, void *), void * 0x00000000) line 364 + 15 bytes js_hash_scope_clear(JSContext * 0x03bcf3c0, JSScope * 0x03e33440) line 234 + 16 bytes js_Clear(JSContext * 0x03bcf3c0, JSObject * 0x03aebae0) line 3191 + 17 bytes JS_ClearScope(JSContext * 0x03bcf3c0, JSObject * 0x03aebae0) line 2567 + 19 bytes GlobalWindowImpl::SetNewDocument(GlobalWindowImpl * const 0x03cdcdc8, nsIDOMDocument * 0x00000000) line 371 + 49 bytes DocumentViewerImpl::~DocumentViewerImpl() line 419 DocumentViewerImpl::`scalar deleting destructor'(unsigned int 1) + 15 bytes DocumentViewerImpl::Release(DocumentViewerImpl * const 0x03bcec00) line 357 + 154 bytes nsCOMPtr<nsIContentViewer>::assign_assuming_AddRef(nsIContentViewer * 0x00000000) line 471 nsCOMPtr<nsIContentViewer>::assign_with_AddRef(nsISupports * 0x00000000) line 951 nsCOMPtr<nsIContentViewer>::operator=(nsIContentViewer * 0x00000000) line 583 nsDocShell::Destroy(nsDocShell * const 0x0502eeb4) line 1696 nsWebShell::Destroy(nsWebShell * const 0x0502eeb4) line 1446 nsXULWindow::Destroy(nsXULWindow * const 0x03b1023c) line 330 nsWebShellWindow::Destroy(nsWebShellWindow * const 0x03b1023c) line 1782 + 9 bytes nsChromeTreeOwner::Destroy(nsChromeTreeOwner * const 0x050167e4) line 223 GlobalWindowImpl::Close(GlobalWindowImpl * const 0x03cdcdcc) line 2090 GlobalWindowImpl::CloseWindow(nsISupports * 0x03cdcdc8) line 3582 nsJSContext::ScriptEvaluated(nsJSContext * const 0x03e88538, int 1) line 1316 + 18 bytes nsJSContext::CallEventHandler(nsJSContext * const 0x03e88538, void * 0x03daafe8, void * 0x04fa02a8, unsigned int 1, void * 0x0012c1cc, int * 0x0012c1c8, int 0) line 936 nsJSEventListener::HandleEvent(nsIDOMEvent * 0x03ea49c4) line 154 + 64 bytes nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03d873b8, nsIDOMEvent * 0x03ea49c4, nsIDOMEventTarget * 0x03f069e0, unsigned int 8, unsigned int 7) line 843 + 19 bytes nsEventListenerManager::HandleEvent(nsIPresContext * 0x04f9ead0, nsEvent * 0x0012ca6c, nsIDOMEvent * * 0x0012ca04, nsIDOMEventTarget * 0x03f069e0, unsigned int 7, nsEventStatus * 0x0012cab0) line 1725 + 39 bytes nsXULElement::HandleDOMEvent(nsXULElement * const 0x03f069d8, nsIPresContext * 0x04f9ead0, nsEvent * 0x0012ca6c, nsIDOMEvent * * 0x0012ca04, unsigned int 1, nsEventStatus * 0x0012cab0) line 3455 PresShell::HandleDOMEventWithTarget(PresShell * const 0x03e4b028, nsIContent * 0x03f069d8, nsEvent * 0x0012ca6c, nsEventStatus * 0x0012cab0) line 4923 + 39 bytes nsButtonBoxFrame::MouseClicked(nsIPresContext * 0x04f9ead0, nsGUIEvent * 0x0012cc38) line 138 nsButtonBoxFrame::HandleEvent(nsButtonBoxFrame * const 0x05092aa8, nsIPresContext * 0x04f9ead0, nsGUIEvent * 0x0012cc38, nsEventStatus * 0x0012cf1c) line 99 PresShell::HandleEventInternal(nsEvent * 0x0012cc38, nsIView * 0x00000000, unsigned int 1, nsEventStatus * 0x0012cf1c) line 4891 + 38 bytes PresShell::HandleEventWithTarget(PresShell * const 0x03e4b028, nsEvent * 0x0012cc38, nsIFrame * 0x05092aa8, nsIContent * 0x03f069d8, unsigned int 1, nsEventStatus * 0x0012cf1c) line 4857 + 22 bytes nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 0x03abd0f0, nsIPresContext * 0x04f9ead0, nsMouseEvent * 0x0012d028, nsEventStatus * 0x0012cf1c) line 1941 + 61 bytes nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x03abd0f8, nsIPresContext * 0x04f9ead0, nsEvent * 0x0012d028, nsIFrame * 0x05092aa8, nsEventStatus * 0x0012cf1c, nsIView * 0x04f52690) line 1075 + 28 bytes PresShell::HandleEventInternal(nsEvent * 0x0012d028, nsIView * 0x04f52690, unsigned int 1, nsEventStatus * 0x0012cf1c) line 4896 + 43 bytes PresShell::HandleEvent(PresShell * const 0x03e4b02c, nsIView * 0x04f52690, nsGUIEvent * 0x0012d028, nsEventStatus * 0x0012cf1c, int 1, int & 1) line 4811 + 25 bytes nsView::HandleEvent(nsView * const 0x04f52690, nsGUIEvent * 0x0012d028, unsigned int 28, nsEventStatus * 0x0012cf1c, int 1, int & 1) line 379 nsViewManager2::DispatchEvent(nsViewManager2 * const 0x04f521a0, nsGUIEvent * 0x0012d028, nsEventStatus * 0x0012cf1c) line 1439 HandleEvent(nsGUIEvent * 0x0012d028) line 68 nsWindow::DispatchEvent(nsWindow * const 0x04f5272c, nsGUIEvent * 0x0012d028, nsEventStatus & nsEventStatus_eIgnore) line 687 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012d028) line 708 nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4006 + 21 bytes ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4216 nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 30474784, long * 0x0012d3dc) line 3023 + 24 bytes nsWindow::WindowProc(HWND__ * 0x014906a8, unsigned int 514, unsigned int 0, long 30474784) line 943 + 27 bytes USER32! 77e13eb0() USER32! 77e1401a() USER32! 77e13f0f() nsAppShell::DispatchNativeEvent(nsAppShell * const 0x04f99768, int 1, void * 0x02b04af0 msg) line 193 + 9 bytes nsXULWindow::ShowModal(nsXULWindow * const 0x03b10238) line 242 nsWebShellWindow::ShowModal(nsWebShellWindow * const 0x03b10238) line 1118 nsChromeTreeOwner::ShowModal(nsChromeTreeOwner * const 0x050167e0) line 182 GlobalWindowImpl::OpenInternal(GlobalWindowImpl * const 0x029bcd10, JSContext * 0x028a33d8, long * 0x03e07198, unsigned int 5, int 1, nsIDOMWindowInternal * * 0x0012db44) line 3143 GlobalWindowImpl::OpenDialog(GlobalWindowImpl * const 0x029bcd14, JSContext * 0x028a33d8, long * 0x03e07198, unsigned int 5, nsIDOMWindowInternal * * 0x0012db44) line 2076 WindowInternalOpenDialog(JSContext * 0x028a33d8, JSObject * 0x00cae988, unsigned int 5, long * 0x03e07198, long * 0x0012dc28) line 4424 + 42 bytes js_Invoke(JSContext * 0x028a33d8, unsigned int 5, unsigned int 0) line 784 + 23 bytes js_Interpret(JSContext * 0x028a33d8, long * 0x0012e974) line 2607 + 15 bytes js_Invoke(JSContext * 0x028a33d8, unsigned int 1, unsigned int 2) line 801 + 13 bytes js_InternalInvoke(JSContext * 0x028a33d8, JSObject * 0x03aeb478, long 61781192, unsigned int 0, unsigned int 1, long * 0x0012eb0c, long * 0x0012ea9c) line 873 + 20 bytes JS_CallFunctionValue(JSContext * 0x028a33d8, JSObject * 0x03aeb478, long 61781192, unsigned int 1, long * 0x0012eb0c, long * 0x0012ea9c) line 3264 + 31 bytes nsJSContext::CallEventHandler(nsJSContext * const 0x029e30a8, void * 0x03aeb478, void * 0x03aeb4c8, unsigned int 1, void * 0x0012eb0c, int * 0x0012eb08, int 0) line 928 + 33 bytes nsJSEventListener::HandleEvent(nsIDOMEvent * 0x03c30e14) line 154 + 64 bytes nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x02a4da38, nsIDOMEvent * 0x03c30e14, nsIDOMEventTarget * 0x02a4d798, unsigned int 8, unsigned int 7) line 843 + 19 bytes nsEventListenerManager::HandleEvent(nsIPresContext * 0x0281e9d8, nsEvent * 0x0012f3c4, nsIDOMEvent * * 0x0012f344, nsIDOMEventTarget * 0x02a4d798, unsigned int 7, nsEventStatus * 0x0012f40c) line 1725 + 39 bytes nsXULElement::HandleDOMEvent(nsXULElement * const 0x02a4d790, nsIPresContext * 0x0281e9d8, nsEvent * 0x0012f3c4, nsIDOMEvent * * 0x0012f344, unsigned int 1, nsEventStatus * 0x0012f40c) line 3455 PresShell::HandleDOMEventWithTarget(PresShell * const 0x02898718, nsIContent * 0x02a4d790, nsEvent * 0x0012f3c4, nsEventStatus * 0x0012f40c) line 4923 + 39 bytes nsMenuFrame::Execute() line 1373 nsMenuFrame::HandleEvent(nsMenuFrame * const 0x03cc65a8, nsIPresContext * 0x0281e9d8, nsGUIEvent * 0x0012f7f8, nsEventStatus * 0x0012f6ec) line 379 PresShell::HandleEventInternal(nsEvent * 0x0012f7f8, nsIView * 0x03b91d10, unsigned int 1, nsEventStatus * 0x0012f6ec) line 4891 + 38 bytes PresShell::HandleEvent(PresShell * const 0x0289871c, nsIView * 0x03b91d10, nsGUIEvent * 0x0012f7f8, nsEventStatus * 0x0012f6ec, int 0, int & 1) line 4811 + 25 bytes nsView::HandleEvent(nsView * const 0x03b91d10, nsGUIEvent * 0x0012f7f8, unsigned int 8, nsEventStatus * 0x0012f6ec, int 0, int & 1) line 379 nsView::HandleEvent(nsView * const 0x03c5ec68, nsGUIEvent * 0x0012f7f8, unsigned int 8, nsEventStatus * 0x0012f6ec, int 0, int & 1) line 352 nsView::HandleEvent(nsView * const 0x03ca8a68, nsGUIEvent * 0x0012f7f8, unsigned int 8, nsEventStatus * 0x0012f6ec, int 0, int & 1) line 352 nsView::HandleEvent(nsView * const 0x028d9f38, nsGUIEvent * 0x0012f7f8, unsigned int 28, nsEventStatus * 0x0012f6ec, int 1, int & 1) line 352 nsViewManager2::DispatchEvent(nsViewManager2 * const 0x02a155f0, nsGUIEvent * 0x0012f7f8, nsEventStatus * 0x0012f6ec) line 1439 HandleEvent(nsGUIEvent * 0x0012f7f8) line 68 nsWindow::DispatchEvent(nsWindow * const 0x03c52ffc, nsGUIEvent * 0x0012f7f8, nsEventStatus & nsEventStatus_eIgnore) line 687 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f7f8) line 708 nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4006 + 21 bytes ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4216 nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 12255307, long * 0x0012fbac) line 3023 + 24 bytes nsWindow::WindowProc(HWND__ * 0x00c2067e, unsigned int 514, unsigned int 0, long 12255307) line 943 + 27 bytes USER32! 77e13eb0() USER32! 77e1401a() USER32! 77e13f0f() nsAppShell::Run(nsAppShell * const 0x027381f8) line 104 + 9 bytes nsAppShellService::Run(nsAppShellService * const 0x02738a60) line 408 main1(int 1, char * * 0x004a7bd8, nsISupports * 0x00000000) line 1019 + 32 bytes main(int 1, char * * 0x004a7bd8) line 1287 + 37 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 77e87903()
And just to be clear: it crashes as soon as you hit 'OK' in the prefs panel after changing the pref a second time.
Yeah, probably not. I just blame things on Mike when I'm not sure (and this is his tooltip stuff...).
Interesting strategy, but I can't use it since Mike knows where to find me. At any rate, ignore the particular stack trace given above (I should have tried more than one). This crashes on any number of stack traces, usually trying to dereference a pointer that has a value of 0x00000001 (i.e., something is getting stomped). Curiously, if I change the code that Blake added to refer to a bogus preference, I still get the crash the same way (bad dereference). <checkbox id="showHideTooltips" value="SomethingorOther" pref="true" preftype="bool" prefstring="browser.chrome.toolbar_tips_Whatever" prefattribute="checked"/> I also get this warning when changing the pref in that panel: JavaScript strict warning: chrome://communicator/content/pref/nsPrefWindow.js line 212: reference to undefined property itemObject.pref
so it sounds to me like this has nothing to do with tooltips? since any pref you set the 2nd time will crash? is this correct? pushing to alecf. he can spank me if i'm wrong.
Assignee: pinkerton → alecf
> since any pref you set the 2nd time will crash? is this correct? Yes. Apparently so (though I don't see what's special about that checkbox in this panel).
See bug 64025. The stack there seems to point to trees and XBL, e.g., all roads lead to hyatt :-) Then again, you said the stacks differ.
So after clicking the same link over and over again, I realized I linked to this bug. Try bug 62272.
That other bug 62272 could be the same thing. > Then again, you said the stacks differ. Yes, I would crash on wildly varying stack traces. Basically, it looks like some memory is being stomped on. The stack traces just show the victim trying to dereference a bogus pointer. How the value '1' got written into someone else's pointer is unknown.
this really looks like hyatt
Assignee: alecf → hyatt
Any pref changed twice? Random stacks? Sounds like tree is a victim here, on stack just cuz it is used in prefs. ->prefs backend
Assignee: hyatt → dveditz
Component: XP Toolkit/Widgets: Menus → Preferences: Backend
QA Contact: jrgm → sairuh
Assignee: dveditz → ben
Component: Preferences: Backend → Preferences
Keywords: nsbeta1
OS: Windows 2000 → All
Hardware: PC → All
Summary: Crash when changing the Show Tooltips option the 2nd time → Crash when changing the Show Tooltips pref
interesting, i get a crash all the time when i toggle the tooltip pref --however, the steps i have a bit different from the original. seem i need to have both navigator and mail selected in the Appearance panel beforehand. recipe below, but pls lemme know if i'm really seeing a different bug. btw, occurs on mac, linux and winnt [opt comm bits 2001.02.28.xx]. 0. make sure you have Navigator and Mail selected [checked on] in the Appearance panel beforehand. if you need to do this, exit and restart the app after you do so. 1. open prefs dialog, and in the Appearance panel, turn off [or, turn on] the Show Tooltips checkbox. 2. click OK to save and exit prefs. results: crash. [side note: if it doesn't crash after step 2, then try quitting the app normally --talkback should then pop up.] the stack traces do vary for me as well, alas. they tend start in nsXULElement::GetAttribute, nsXULElement::SetDocument, or nsGenericContainerElement::ChildAt. some talkback incident links [win32 ones, as they seemed to have more info]: http://cyclone.mcom.com/reports/SingleIncidentInfo.cfm?dynamicBBID=27145227 http://cyclone.mcom.com/reports/SingleIncidentInfo.cfm?dynamicBBID=27145499 http://cyclone.mcom.com/reports/SingleIncidentInfo.cfm?dynamicBBID=27145516
ok, this is so totally not me.
Gotta fix this, marking p1, nsbeta1+, mozilla0.9, reassigning to pchen
Assignee: ben → pchen
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P1
Target Milestone: --- → mozilla0.9
I had disabled tooltips, and trying now to enable them again (to test another bug) i crashed three times in a row, all different backtraces. Attachment follows. Linux 20010313something
Attached file bt —
I also notice that each time i click Appearance, an error displays in console (but it usually displays whatever i click in prefs...) Warning prev sibling is not in our list!!!
minor fix, removing comment out of CDATA array. hey, I can't get this to crash with this patch...
before my patch, I could crash this easily on linux.
Fix is harmless enough, and if it fixes this crash, r=blake. But let's figure out why this is causing problems.
sr=shaver, and what blake said -- there's something rotten here.
Er, well it is dead code (remove it anyways), and I was going to say the same thing about finding the root cause ... but this change don't do it for me on win2k with an opt. build from last night. I crashed three times with that 'kill the comment' change.
This fix doesn't work for me, and I now think that it couldn't. I rescind my sr.
With what little debugging I've accomplished so far today on this, I can say that we've got a memory corruption issue. I see it on both win32 and mac.
There's something rotten in Denmark, sorry for the redherring.
On the mac, the problem that shows up is that the mPseudoTag member of a StyleContextImpl object is getting whacked to the value "1". That data member should either be null or an allocated object, certainly not 1. So, obviously, someone is stomping over a StyleContextImpl (as John Morrison has pointed out), the stacks are slightly different because it's likely to be a different StyleContextImpl everytime with a different code path. I've been trying to set a watchpoint on mPseudoTag for created StyleContextImpl's when the preference window comes up, hoping to catch the offending piece of code. Unfortunately, we allocate and deallocate a boatload of StyleContextImpl objects. Dave Hyatt then informs me that there is a bug where to check if a style context is cached, we first create a style context, then check to see if it's in the cache, and if it is, then destroy the style context just created. Sounds suboptimal to me. Reassigning to Pierre since a) he's a stud in general, b) he knows the style system better than I do, and c) I hear he's got some style system fixes and maybe this is one of them. Resetting nsbeta1+ to nsbeta1 and resetting target milestone since I am in no position to set those values for Pierre. Pierre, I suggest you set a conditional breakpoint in the destructor for StyleContextImpl on the line "NS_IF_RELEASE(mPseudoTag);" for when mPseudoTag==1. Oh yeah, the reproducible case for me is unchecking "Show Tooltips", opening a new browser window, close that new browser window, go to prefs, check the "Show Tooltips" pref, click "Ok" in the prefs dialog, boom.
Assignee: pchen → pierre
Keywords: nsbeta1+ → nsbeta1
Target Milestone: mozilla0.9 → ---
I think I have a fix for this - taking it over.
Assignee: pierre → attinasi
... like bug 73553 I think. Patch there probably fixes this too.
Status: NEW → ASSIGNED
Fixed with 73553
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
I'm still seeing this bug when i repeat the steps i listed when i reported this bug. I'm using the 2001042208 nightly on win2k.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm. Yep. In an opt. build on win2k, pulled this evening, I also crash following those steps (6 times, 3 different stack traces, but still looking like some memory is getting stomped; I'll attach the stack traces for what they are worth).
These stacks all seem to have something to do with the binding manager. Hyatt, any interest in looking at this?
moving onto current radar (0.9.1)
Status: REOPENED → ASSIGNED
Target Milestone: --- → mozilla0.9.1
OK, it looks to me like the XULPopupListenerImpl has a static pref-change callback on the ToolTip pref, adn that callback just happens to set a value to either 1 or 0, depending on the value of the pref. It sets this value into a data member of an instance of XULPopupListenerImpl, and that instance is passed into the callback by the pref service. Unfortunately, the pref service is never notified when XULPopupListenerImpl instances are deleted, so it ends up notifying with bogus instanec pointers, hence the 'random' 1 value poked into random memory. I have a patch for this that just unregisters the callback in the destructor of the XULPopupListenerImpl, but it seems to me that it might be better to have a single tooltip-enable state flag (static to XULPopupListenerImpl instead of instance data) and skip registering the callback on specific instances. Can one part of the app have tooltips on and another part have it off? If so, then the unregister is needed, however I noted that there are more than a dozen XULPopupListenerImpl instances created for a single window.
nsXULElement::Create is creating a popup listener for every element with a popup, tooltip, or context attribute (I think). Opening a single browser creates 57 instances for me (I put in a ctor/dtor counter). I'm no expert here, but making the tooltip pref static (i.e. global) would save us a lot of registering callbacks with the pref system at startup, and we won;t have to do all of the unregisters that are needed to fix this bug either. But then, maybe each XUL element needs to be able to support its very own tooltip enable state?
I attached a patch that only registers the pref change callback once and stores the tooltip enable state in a static data member instead of instance data. This dies several things: 1) it makes all PopupListeners respect the same global tooltip setting (which is all they ever did before, I think) 2) it speeds up new window opens because there is a lot less pref service creation and interaction - this is probably small and may be unmeasurable 3) fixes this bug by eliminating the stale this-ptr in the pref changed callback Coments? Reviews?
Keywords: patch
+ static PRBool mShowTooltips; // mirrors the "show tooltips" pref + static PRBool mPrefChangeRegistered; // set when the callback is registered Those want to be 'sShowTooltips' and 'sPrefChangeRegistered', respectively. Do we really need to go to the pref service for every popup listener (for which I presume Init will be called)? It would seem that we could fetch the pref the first time through, when we set the pref-changed listener, and then just rely on sShowTooltips during subsequent trips through Init. (It sucks that you don't just get the new value and the pref service in your change listener, but that's not your fault. I wonder if bnesse fixed that as part of his cleanup.) Other than that, I like it. I don't think we need per-tooltip settings.
Actually the whole callback thing has been deprecated out of existence (as has nsIPref.) Supporting nsIObserver and calling AddObserver is the "correct" method. Looking at the patch, it should be pretty straightforward task to make it use nsIPrefService and nsIPrefBranch. I'll try and put together a version of the patch based on the new prefs stuff. The Observe callback does return the PrefBranch, but does not return the new value of the preference. I suppose that would be a nice feature in some cases (i.e. int and bool), but could potentially be troublesome in cases where memory was allocated (i.e. strings or complex types).
Please rename static members to sShowTooltips and sPrefChangeRegistered.
OK, I need to change the name of the static members, I'll do that. But Mike's comment made me think that the Init method does NOT have to check the pref for each listener. The static sShowToolTips only needs to be set when the listener is registered, and in the callback. So, I can attach another patch to address these two issues, but it sounded like Brian Nesse might just take this all over and convert it to the new scheme... Brian, do you think you will get to that soon, or should I proceed with fixing the current scheme? I may be mistaken, but it seems like there are many places that might need a similar conversion, so maybe we should (r already do) have a bug to make the conversion?
sorry to come in late here. I agree that we should only have the one callback registered. My bad. I didn't think there would be so many, and my bad also for not unregistering when the listener went away. Marc, what comment did I make in Init() about checking the pref? Can't we assume once the callback is installed that the static variable is valid for all time, so we no longer have to check the pref in Init every time?
I'd init sShowTooltips, just to be safe. +PRBool XULPopupListenerImpl::sShowTooltips = PR_FALSE; i guess this is safe when there are no windows around, so we don't have to unregister, per se.
Mike, the old Init had to get the pref for each instance because they each had their own flag. Now, we only need to grab the pref value once (the first Init call) and if the callback is called - my latest patch does this. Also, statics are always initialized, but I can do it explicitely anyway just in case some lame-ass compiler forgets to implement something that has been mandated by C for like 20 years... And finally, calling the callback with no XULPopupListenerInstances in existance, although extremely unlikely, is perfectly safe, so there is no need to unregister (and that would require come kind of ref-counting to know when to unregister too, ugly). So, given that the latest patch (id=33914) and the addition of an initializer for the static data member, can I get some hot review action and get this baby chhecked in? Thanks.
At this point, fix it and check it in. I've been in meetings all day and haven't made any progress on this. We can do this post 0.9.1.
sr=hyatt. Sorry for the delay. Meeting hell. :)
--> Patch
Summary: Crash when changing the Show Tooltips pref → [PATCH]Crash when changing the Show Tooltips pref
Patch checked in: /cvsroot/mozilla/content/xul/content/src/nsXULPopupListener.cpp,v <-- nsXULPop upListener.cpp new revision: 1.84; previous revision: 1.83
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
vrfy fixed using 2001.05.17.0x comm bits on linux, mac and winnt. thx, marc!
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: