50.50 KB, patch
|Details | Diff | Splinter Review|
43.97 KB, patch
|Details | Diff | Splinter Review|
2.38 KB, patch
|Details | Diff | Splinter Review|
Created attachment 262340 [details] unminised testcase See unminimised testcase, when I reach 801 in the statusbar, current trunk builds crash. It usually happens directly, or after a reload. Talkback ID: TB31404174E 0x80000000 nsViewManager::AddCoveringWidgetsToOpaqueRegion [mozilla/view/src/nsviewmanager.cpp, line 619] nsViewManager::Refresh [mozilla/view/src/nsviewmanager.cpp, line 538] This regressed for me between 2007-03-04 and 2007-03-05: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-03-04+04&maxdate=2007-03-05+09&cvsroot=%2Fcvsroot I guess a regression from bug 363253, somehow. I'm getting this crash all the time when I use the thing from bug 339128. I suspect this is a windows only crash, somehow. I tried to crash in a windows debug build with gdb, but there it wouldn't crash. However, I did see some cairo rules (I guess these might indicate something): _cairo_win32_surface_composite(BitBlt): De bewerking is voltooid. _cairo_win3_surface_acquire_dest_image: De ingang is ongeldig. _cairo_win32_surface_set_clip_region (reset): De ingang is ongeldig. The testcase is totally unminimised, so this bug should be closed. When I try to minimise it, it just tends to crash less and less likely.
Also crashes on Linux if I reload a few times: TB31416605Z
Created attachment 262506 [details] [diff] [review] Patch rev. 1 (diff -w) There are a few separate bugs here. First there is the WillPaint() call in nsViewManager which flushes notifications which leads up to the view being destroyed, then calling Refresh on that view. This affects all platforms. The gtk2 widget code have additional bugs. We need to hold strong refs on 'this' whenever we call DispatchEvent() and depend on being alive after that. Also, preventing deletion isn't enough, we also need to test 'mIsDestroyed' if we're using any native windows since nsWindow::Destroy makes them them invalid. This is necessary to avoid X Window errors like 'BadDrawable (invalid Pixmap or Window parameter)' which makes _XError call exit(). This patch fixes the above issues. I will review non-gtk2 widget code for similar errors of the second type, I'm pretty sure there are some.
+ nsRefPtr<nsWindow> kungFuDeathGrip = this; Can we move these out to the gtk callbacks, just before we call into the nsWindow? We should avoid deleting the nsWindow while still running methods in it. Otherwise looks fine.
Created attachment 263066 [details] [diff] [review] Patch rev. 2
Created attachment 263068 [details] [diff] [review] Patch rev. 2 (diff -w) Good point. I have changed nsWindow such that all its methods now assume the caller has a strong ref on 'this'. I added missing refs in callers (gtk callbacks and a few IME related calls) and removed a few AddRef's on 'this' inside nsWindow methods. I have added strong refs on local nsWindow pointers inside these methods. As an example: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.215&root=/cvsroot&mark=775,852#774 Although we can assume 'this' is strongly held, we can't assume the same for 'gFocusWindow' so I added a strong refs on such locals, to keep the caller's promise.
I noted that we have some long standing bugs involving nsWindow::OnDrag* for gtk/gtk2, bug 269568, bug 300675, bug 309298, I wonder if this patch might help. (feel free to CC me on any bugs that have clear STR for trunk or 1.8 and I might have a look at it)
Nevermind, I can repro bug 309298 and it looks like a different problem...
Checked in to trunk at 2007-04-29 17:46 PDT. -> FIXED
I can't crash branch builds (Linux) on the testcases, but looking at the view manager/pres shell code on branch it looks pretty much the same as trunk, so there is some risk this could happen on branch too. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/view/src/nsViewManager.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=1964,2035,2044#1961 http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=6554,6571#6553 I tried querying http://talkback-public.mozilla.org/ for nsViewManager crashes in branch builds but half an hour later I still haven't got the search result back :-(
It seems to me this could be the #92 crasher (nsViewManager::Refresh) at: http://talkback-public.mozilla.org/reports/firefox/FF2003/FF2003-topcrashers.html right? http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsViewManager::Refresh&vendor=MozillaOrg&product=Firefox2&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid (especially thanks for fixing this one, Mats)
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070430 Minefield/3.0a5pre
The nsViewManager part of the patch is low risk, maybe we should consider taking that for 220.127.116.11/18.104.22.168? Andrew, since you were able to reproduce bug 376155 - could you try the attached branch patch and see if that alone fixes it?
Yes, the branch patch fixes bug 376155 for me.
I know this is very late for 22.214.171.124, but to fix bug 376155, we, IMO, should take the branch patch. (If roc or someone first reviews it.)
Comment on attachment 263268 [details] [diff] [review] nsViewManager 1.8 branch patch sr=dveditz fwiw, but we need a module owner review.
Comment on attachment 263268 [details] [diff] [review] nsViewManager 1.8 branch patch This looks fine to me, and pretty safe. Would probably still be good if roc takes a look if he has a chance, but r=dbaron.
Comment on attachment 263268 [details] [diff] [review] nsViewManager 1.8 branch patch approved for 126.96.36.199 and 188.8.131.52, a=dveditz for release-drivers
The 1.8 branch patch doesn't fix the entire bug, right? If that's the case when this is checked in put the "fixed" keywords on bug 376155 rather than this one.
Comment on attachment 263268 [details] [diff] [review] nsViewManager 1.8 branch patch (In reply to comment #20) > The 1.8 branch patch doesn't fix the entire bug, right?' Correct. It should fix the vast majority of crashes though (also the "topcrash #92" that was mentioned). The remaining part is GTK2-only and leads in most cases to a call to exit() from XError. (With the reservation that I haven't investigated non-GTK2 widget code.) > put the "fixed" keywords on bug 376155 rather than this one. Will do. Checked in to MOZILLA_1_8_BRANCH at 2007-04-30 18:45 PDT. Checked in to MOZILLA_1_8_0_BRANCH at 2007-04-30 18:47 PDT.
The view manager patch landed, the rest we'll have to look at in 184.108.40.206
With attachment 263268 [details] [diff] [review] I no longer crash on the branches on windows, and calling exit() on GTK2 is safe enough to consider this fixed on the branches.