Last Comment Bug 378273 - Crash [@ nsViewManager::AddCoveringWidgetsToOpaqueRegion]
: Crash [@ nsViewManager::AddCoveringWidgetsToOpaqueRegion]
Status: VERIFIED FIXED
[sg:critical?]
: crash, fixed1.8.0.12, fixed1.8.1.4, regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (vacation)
:
Mentors:
Depends on:
Blocks: stirtable 363253 376155
  Show dependency treegraph
 
Reported: 2007-04-21 07:06 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
6 users (show)
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
mats: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch rev. 1 (20.58 KB, patch)
2007-04-23 09:23 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 1 (diff -w) (17.91 KB, patch)
2007-04-23 09:25 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 2 (50.50 KB, patch)
2007-04-27 17:08 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 2 (diff -w) (43.97 KB, patch)
2007-04-27 17:10 PDT, Mats Palmgren (vacation)
roc: review+
roc: superreview+
Details | Diff | Splinter Review
nsViewManager 1.8 branch patch (2.38 KB, patch)
2007-04-30 10:26 PDT, Mats Palmgren (vacation)
dbaron: review+
dveditz: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-21 07:06:49 PDT
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.
Comment 1 Mats Palmgren (vacation) 2007-04-21 16:07:45 PDT
Also crashes on Linux if I reload a few times: TB31416605Z
Comment 2 Mats Palmgren (vacation) 2007-04-23 09:23:27 PDT
Created attachment 262505 [details] [diff] [review]
Patch rev. 1
Comment 3 Mats Palmgren (vacation) 2007-04-23 09:25:09 PDT
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.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-04-25 17:56:01 PDT
+    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.
Comment 5 Mats Palmgren (vacation) 2007-04-27 17:08:28 PDT
Created attachment 263066 [details] [diff] [review]
Patch rev. 2
Comment 6 Mats Palmgren (vacation) 2007-04-27 17:10:21 PDT
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.
Comment 7 Mats Palmgren (vacation) 2007-04-27 17:21:54 PDT
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)
Comment 8 Mats Palmgren (vacation) 2007-04-27 18:28:43 PDT
Nevermind, I can repro bug 309298 and it looks like a different problem...
Comment 9 Mats Palmgren (vacation) 2007-04-29 18:29:27 PDT
Checked in to trunk at 2007-04-29 17:46 PDT.

-> FIXED
Comment 10 Mats Palmgren (vacation) 2007-04-29 18:48:16 PDT
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 :-(
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-30 06:27:42 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070430 Minefield/3.0a5pre
Comment 13 Mats Palmgren (vacation) 2007-04-30 10:26:59 PDT
Created attachment 263268 [details] [diff] [review]
nsViewManager 1.8 branch patch
Comment 14 Mats Palmgren (vacation) 2007-04-30 10:33:18 PDT
The nsViewManager part of the patch is low risk, maybe we should consider
taking that for 1.8.1.4/1.8.0.12?

Andrew, since you were able to reproduce bug 376155 - could you try the
attached branch patch and see if that alone fixes it?
Comment 15 Andrew Schultz 2007-04-30 11:55:32 PDT
Yes, the branch patch fixes bug 376155 for me.
Comment 16 Olli Pettay [:smaug] 2007-04-30 12:05:14 PDT
I know this is very late for 1.8.1.4, but to fix bug 376155, we, IMO, should take the branch patch. (If roc or someone first reviews it.)
Comment 17 Daniel Veditz [:dveditz] 2007-04-30 18:24:57 PDT
Comment on attachment 263268 [details] [diff] [review]
nsViewManager 1.8 branch patch

sr=dveditz fwiw, but we need a module owner review.
Comment 18 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2007-04-30 18:31:16 PDT
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 19 Daniel Veditz [:dveditz] 2007-04-30 18:35:12 PDT
Comment on attachment 263268 [details] [diff] [review]
nsViewManager 1.8 branch patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Comment 20 Daniel Veditz [:dveditz] 2007-04-30 18:36:37 PDT
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 21 Mats Palmgren (vacation) 2007-04-30 19:01:36 PDT
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.
Comment 22 Daniel Veditz [:dveditz] 2007-04-30 19:43:23 PDT
The view manager patch landed, the rest we'll have to look at in 1.8.1.5
Comment 23 Daniel Veditz [:dveditz] 2007-05-01 14:41:10 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.