Closed
Bug 80345
Opened 24 years ago
Closed 24 years ago
frequent crash in nsWindow::UpdateIdle; Trunk & M09 crash [@ 0x00000000 - nsAppShell::Run]
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: darin.moz, Assigned: darin.moz)
References
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
frequent crash in nsWindow::UpdateIdle looks like tmp_list->data points to garbage. this is because the nsWindow object is not AddRef'd before being put on the update_queue.
Assignee | ||
Comment 1•24 years ago
|
||
Updated•24 years ago
|
Severity: normal → critical
Assignee | ||
Updated•24 years ago
|
Keywords: mozilla0.9.1,
nsbeta1
Comment 6•24 years ago
|
||
Doesn't UpdateIdle need to Release each window, too -- before it calls g_slist_free on old_queue? /be
Comment 7•24 years ago
|
||
Looks reasonable to me, so sr=waterson if you'll respond to brendan's comments.
Assignee | ||
Comment 8•24 years ago
|
||
brendan: you're correct. i checked it out and with my fix, we leak windows. when i add a Release after window->Update(), i seem to get the reference count balanced, but we crash in layout almost immediately after clicking on a link.
Comment 9•24 years ago
|
||
Darin, how about attaching that crash stack and some of the scene-of-the-crime variables in registers and nearby memory? /be
Comment 10•24 years ago
|
||
Oh yeah, blizzard's out for the weekend, I'm virtual blizzard, but I'm sure he will want to be cc'd so he can catch up and maybe help us. /be
Comment 11•24 years ago
|
||
Brendan: check out bug 56086 (dup of this) for a stack and some "scene-of-the-crime" variables.
Comment 12•24 years ago
|
||
dr: I saw that, but I need to see where layout croaks with Darin's attempt to Release in Update Idle. I'm also curious whether his patch releases during the tmp_list walk, or in a separate pass, just before the g_slist_free call.] /be
Assignee | ||
Comment 13•24 years ago
|
||
sorry for not being very responsive... here's the patch i have most recently tested.
Assignee | ||
Comment 14•24 years ago
|
||
Assignee | ||
Comment 15•24 years ago
|
||
i confirmed that this patch gets the refcount properly balanced; however, it seems that if the final Release happens from within UpdateIdle, then we crash while destroying the window. i'll attach a stack trace just as soon as i have a linux build.
Assignee | ||
Comment 16•24 years ago
|
||
it seems to me that if the window cannot be destroyed from UpdateIdle (for whatever reason), then my attempt at fixing this bug has been misguided. we should probably be looking to simply ensure that the window is removed from the update_queue in its destructor. in fact, i bet this would be the correct patch. once i have a linux build, i'll go give that patch a try.
Assignee | ||
Comment 17•24 years ago
|
||
bug 53704 has the correct diagnosis of the problem. the nsWindow instance is being added to the update_queue multiple times, but only removed once. i'm not sure yet how the window could be added to the list multiple times, since it should be controlled by the mIsUpdating member variable. hmm...
Comment 18•24 years ago
|
||
dr says this is the bug I've been seeing (crash on hitting back/forward) in the past hour, ccing self.
Assignee | ||
Comment 19•24 years ago
|
||
Comment 21•24 years ago
|
||
added topcrash keyword and Trunk & M09 crash [@ 0x00000000 - nsAppShell::Run] to summary for tracking (from dupped bug 56086)
Keywords: topcrash
Summary: frequent crash in nsWindow::UpdateIdle → frequent crash in nsWindow::UpdateIdle; Trunk & M09 crash [@ 0x00000000 - nsAppShell::Run]
Comment 22•24 years ago
|
||
Since I don't see this crash I wonder if it's related to the version of glib/gtk. Maybe there's a bug in older version of g_slist_remove() or something.
Assignee | ||
Comment 23•24 years ago
|
||
yes exactly! in fact, i'm only seeing this under gtk/glib-1.2.8. with 1.2.9 i can't reproduce it.
Assignee | ||
Comment 24•24 years ago
|
||
turns out there are no differences b/w gslist.c in 1.2.8 and 1.2.9, so it is unlikely that it is this code that is causing the problem. i'm also seeing crashes from g_idle_dispatch, making a callback to some garbage address. Here's the differences between gmain.c in 1.2.8 and 1.2.9: --- ../glib-1.2.8/gmain.c Thu May 18 22:58:48 2000 +++ gmain.c Mon Feb 26 22:00:21 2001 @@ -1134,6 +1134,8 @@ else newrec = g_chunk_new (GPollRec, poll_chunk); + /* This file descriptor may be checked before we ever poll */ + fd->revents = 0; newrec->fd = fd; newrec->priority = priority; @@ -1360,7 +1362,7 @@ GTimeVal *dispatch_time, gpointer user_data) { - GSourceFunc func = source_data; + GSourceFunc func = (GSourceFunc) source_data; return func (user_data); } @@ -1373,7 +1375,7 @@ { g_return_val_if_fail (function != NULL, 0); - return g_source_add (priority, FALSE, &idle_funcs, function, data, notify); + return g_source_add (priority, FALSE, &idle_funcs, (gpointer) function, data, notify); } guint
Comment 25•24 years ago
|
||
Darin, Blizzard: We had this discussion in 56086 -- I had the same thought, but it turned out to be incorrect. The crash *is* reproducible using gtk/glib 1.2.9. Here's how to reproduce the crash: 1: mozilla -g (necessary for the obvious reason of getting debug info, but also to slow your computer down enough for the timing to work out just right). 2: Load all the items in Debug->Verification (I find loading all the items from Mozilla to Mirabilis is good enough of a history list). 3: Hit back and forward a lot. You don't need to hit them fast. In fact, when jrgm showed me how to repro this, he seemed to patiently wait for the pages to partially load. 4: Boom. Darin: want to accept this bug?...
Assignee | ||
Comment 26•24 years ago
|
||
dan: thanks for the info. makes sense since there's hardly any difference b/w glib 1.2.8 and 1.2.9, and also b/c people are reporting this with glib-1.2.10!
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 27•24 years ago
|
||
Assignee | ||
Comment 28•24 years ago
|
||
turns out the problem was that calling Update on a nsWindow from the UpdateIdle handle could cause other windows to be added to the update_queue. this is bad when a window is added to the queue that is already on the queue, but not yet processed. my patch fixes this by first iterating over the update_queue and clearing the mIsUpdating flags. then it makes a second pass to actually call window->Update(), so that if windows get added to the update_queue, their mIsUpdating flag will not be incorrectly set. (hope my explanation makes sense).
Comment 29•24 years ago
|
||
Looks good to me. I'll r= it after I put it through my tests :) BTW, one thing you might want to do (and I'd like to see this in the patch, actually) is add a comment to nsIWidget.idl, mentioning this bug number, saying that calling |update| can cause other widgets to update as well. Thanks!
Assignee | ||
Comment 30•24 years ago
|
||
Assignee | ||
Comment 33•24 years ago
|
||
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ 0x00000000 - nsAppShell::Run]
You need to log in
before you can comment on or make changes to this bug.
Description
•