Last Comment Bug 518758 - Rendering artifacts in Google Wave
: Rendering artifacts in Google Wave
Status: RESOLVED FIXED
: verified1.9.1
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: 1.9.1 Branch
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Markus Stange [:mstange]
Mentors:
Depends on: 339548
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-24 20:24 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2009-11-25 17:23 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.6+
.6-fixed


Attachments
fix (2.54 KB, patch)
2009-09-24 20:28 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mstange: review+
Details | Diff | Splinter Review
fix v2 (2.58 KB, patch)
2009-09-24 23:43 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review
testcase for manual testing (1.30 KB, text/html)
2009-10-03 14:56 PDT, Markus Stange [:mstange]
no flags Details

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 20:24:44 PDT
In drawRect nsChildView.mm doesn't clip out areas that are covered by its child NSViews. On trunk we do subtract those areas from our dirty region, so we could potentially get into trouble in situations where we draw some --- but not all --- content outside our dirty region in the area occupied by a child NSView. Fortunately this isn't too big a deal on trunk because compositor-phase-1 eliminated most child views and I think Cocoa often (but not always) removes child view areas from our getRectsToDraw.

In 1.9.1 things are considerably worse. There, nsViewManager assumes that the platform paint handling will clip out the areas occupied by child widgets. nsChildView.mm does not do this. Things mostly work because Cocoa usually removes those areas from getRectsToDraw, but when count >= MAX_RECTS_IN_REGION we fluff the paint region out to the bounding-box and can easily end up with the clip region including an area occupied by a descendant NSView. Then nsViewManager kicks in and removes the child NSView's area from the paint region, so we don't paint (or only partly paint) that area, and that garbage reaches the screen.

This bug is affecting Google Wave quite badly, probably because it's so complex and it seems to use a number of overflow:auto elements. I've seen dirty regions with over 150 rects in them.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 20:28:07 PDT
Created attachment 402755 [details] [diff] [review]
fix

I don't think we can test this with existing infrastructure. This patch applies to the 1.9.1 branch.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 20:28:38 PDT
Google would really like to us to get this on 1.9.1...
Comment 3 Markus Stange [:mstange] 2009-09-24 20:36:35 PDT
Comment on attachment 402755 [details] [diff] [review]
fix

+  nsRegionRectSet* rgnRects = nsnull;
+  rgn->GetRects(&rgnRects);

Is it worth checking rgnRects for null here?
Comment 4 Markus Stange [:mstange] 2009-09-24 20:43:28 PDT
(In reply to comment #0)
> and can easily end up with
> the clip region including an area occupied by a descendant NSView. Then
> nsViewManager kicks in and removes the child NSView's area from the paint
> region, so we don't paint (or only partly paint) that area, and that garbage
> reaches the screen.

Note that this is only problematic in this special case with the large number of rects. Usually this doesn't matter because the child views paint their own stuff on top of the garbage, so it never reaches the screen. But if we're simplifying the region in the superview, and not doing so in the subviews (because they have a lower number of rects), then we fail to cover up the garbage.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 23:43:58 PDT
Created attachment 402780 [details] [diff] [review]
fix v2

With the null check
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-02 14:23:20 PDT
If we can't get automated tests, can we get a testcase, at least? Did Google have a deadline in mind for inclusion? It's a little late for 3.5.4, IMO, but we should try to get it for 3.5.5
Comment 7 Markus Stange [:mstange] 2009-10-03 14:56:31 PDT
Created attachment 404456 [details]
testcase for manual testing

Clicking the button should not cause any red to show up.
Comment 8 Markus Stange [:mstange] 2009-10-03 17:46:03 PDT
Let's make this bug's status reflect reality:

This bug is fixed on trunk and 1.9.2 by bug 339548, specifically by http://hg.mozilla.org/mozilla-central/rev/a1161f2b4595

The patch in this bug is basically a partial backport of that patch.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-10-04 15:10:38 PDT
I spent a few hours trying to create a testcase and failed before I noticed that Markus had already figured it out. How embarrassing.
Comment 10 Samuel Sidler (old account; do not CC) 2009-11-04 17:50:53 PST
Is this patch ready for 1.9.1? Please request approval, if so.
Comment 11 Daniel Veditz [:dveditz] 2009-11-06 11:29:27 PST
Comment on attachment 402780 [details] [diff] [review]
fix v2

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-08 17:41:52 PST
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db9c10d2cffd
Comment 13 Al Billings [:abillings] 2009-11-25 17:23:05 PST
Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.6pre) Gecko/20091125 Shiretoko/3.5.6pre using manual testcase.

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