Closed Bug 378273 Opened 17 years ago Closed 17 years ago

Crash [@ nsViewManager::AddCoveringWidgetsToOpaqueRegion]

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

References

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached file 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.
Flags: blocking1.9?
Also crashes on Linux if I reload a few times: TB31416605Z
OS: Windows XP → All
Assignee: nobody → mats.palmgren
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attached patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
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.
Attachment #262506 - Flags: superreview?(roc)
Attachment #262506 - Flags: review?(roc)
+    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.
Attached patch Patch rev. 2Splinter Review
Attachment #262505 - Attachment is obsolete: true
Attachment #262506 - Attachment is obsolete: true
Attachment #262506 - Flags: superreview?(roc)
Attachment #262506 - Flags: review?(roc)
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.
Attachment #263068 - Flags: superreview?(roc)
Attachment #263068 - Flags: review?(roc)
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...
Attachment #263068 - Flags: superreview?(roc)
Attachment #263068 - Flags: superreview+
Attachment #263068 - Flags: review?(roc)
Attachment #263068 - Flags: review+
Checked in to trunk at 2007-04-29 17:46 PDT.

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: blocking1.9? → in-testsuite?
Resolution: --- → 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 :-(
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070430 Minefield/3.0a5pre
Status: RESOLVED → VERIFIED
Blocks: 376155
Attachment #263268 - Attachment description: nsViewManager branch patch → nsViewManager 1.8 branch patch
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?
Yes, the branch patch fixes bug 376155 for me.
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.)
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Attachment #263268 - Flags: superreview?(roc)
Attachment #263268 - Flags: review?(roc)
Comment on attachment 263268 [details] [diff] [review]
nsViewManager 1.8 branch patch

sr=dveditz fwiw, but we need a module owner review.
Attachment #263268 - Flags: superreview?(roc)
Attachment #263268 - Flags: superreview+
Attachment #263268 - Flags: approval1.8.1.4?
Attachment #263268 - Flags: approval1.8.0.12?
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.
Attachment #263268 - Flags: review+
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
Attachment #263268 - Flags: approval1.8.1.4?
Attachment #263268 - Flags: approval1.8.1.4+
Attachment #263268 - Flags: approval1.8.0.12?
Attachment #263268 - Flags: approval1.8.0.12+
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
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 1.8.1.5
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.12+
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.
Whiteboard: [sg:critical?]
Group: security
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Crash Signature: [@ nsViewManager::AddCoveringWidgetsToOpaqueRegion]
You need to log in before you can comment on or make changes to this bug.