Closed Bug 595727 Opened 14 years ago Closed 14 years ago

ASSERTION: Where did the clip region go?: '!needs_clip || rect_count != 0', file gfx/thebes/gfxXlibNativeRenderer.cpp, line 262

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: MatsPalmgren_bugz, Assigned: karlt)

References

()

Details

Attachments

(4 files, 3 obsolete files)

STEPS TO REPRODUCE
1. mochitest --browser-chrome

ACTUAL RESULT
###!!! ASSERTION: Where did the clip region go?: '!needs_clip || rect_count != 0', file gfx/thebes/gfxXlibNativeRenderer.cpp, line 262

while running the test browser/components/places/tests/browser/browser_library_middleclick.js

PLATFORMS AND BUILDS TESTED
Firefox mozilla-central on Linux64
Attached file stack
GetWidgetTransparency() returns nsITheme::eUnknownTransparency so
'rendererFlags' is zero (passed as 'flags'), which results in
'max_rectangles' being zero:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibNativeRenderer.cpp#227
Attached patch Like so? (diff -w) (obsolete) — Splinter Review
Attachment #474588 - Flags: review?(karlt)
Thanks for filing this, Mats.

I was also seeing this assertion while using DOM inspector, though I didn't work out exactly what action was leading to this.

The failed assertion means that this assumption is wrong:
http://hg.mozilla.org/mozilla-central/annotate/4f6d1a4cd8ee/gfx/thebes/gfxXlibNativeRenderer.cpp#l260

Theoretically this could be causing drawing in the wrong places, though I
didn't notice anything.
Comment on attachment 474588 [details] [diff] [review]
Like so? (diff -w)

Even when max_rectangles is 0, it is still necessary to check that drawing without a clip will not draw to clipped regions in the context, which _get_rectangular_clip will do.
Attachment #474588 - Flags: review?(karlt) → review-
It seems that cairo_clip_extents is not necessarily empty if the clip is
empty.  If two clip regions are applied in succession, then the clip extents
reported are the intersection of the extents of each of the regions (not
necessarily the extents of the intersection of the regions).

http://hg.mozilla.org/mozilla-central/annotate/4f6d1a4cd8ee/gfx/cairo/cairo/src/cairo-clip.c#l338
Assignee: nobody → karlt
Blocks: 580440
Comment on attachment 474587 [details]
stack

I moved the visual check to after the possible early return with nothing to
do, because that was easy to change.

It would be more effort to switch back to checking the clip rectangle
before the surface type because getting the surface extents depends on the surface type.  Hopefully cairo_clip_extents should be catching the common empty clip cases.  If it's not, then there are other cases that would benefit from a more thorough clip extents.
Attachment #474587 - Flags: review?(roc)
This comment was meant to go on this patch:

I moved the visual check to after the possible early return with nothing to
do, because that was easy to change.

It would be more effort to switch back to checking the clip rectangle
before the surface type because getting the surface extents depends on the surface type.  Hopefully cairo_clip_extents should be catching the common empty clip cases.  If it's not, then there are other cases that would benefit from a more thorough clip extents.
Attachment #474588 - Attachment is obsolete: true
Attachment #474592 - Flags: review?(roc)
Attachment #474587 - Flags: review?(roc)
Move the screen check with the visual check.
Attachment #474592 - Attachment is obsolete: true
Attachment #474593 - Flags: review?(roc)
Attachment #474592 - Flags: review?(roc)
Thanks, that fixes the assertion for me.
Would it be possible to fix the (existing) compile warning at the same time?
gfxXlibNativeRenderer.cpp:210: warning: 'needs_clip' may be used uninitialized in this function
gfxXlibNativeRenderer.cpp:212: warning: 'rect_count' may be used uninitialized in this function
(gcc 4.4.3 Linux)
(In reply to comment #9)
> Would it be possible to fix the (existing) compile warning at the same time?
> gfxXlibNativeRenderer.cpp:210: warning: 'needs_clip' may be used uninitialized
> in this function
> gfxXlibNativeRenderer.cpp:212: warning: 'rect_count' may be used uninitialized
> in this function
> (gcc 4.4.3 Linux)

As much as it would be nice to prevent the compiler spitting out these warnings, there are disadvantages in initializing the values for out parameters.

If out parameters are always initialized, then valgrind (or more thorough static analysis) won't notice when functions (e.g. _get_rectangular_clip) fail to set out parameters when returning successfully.

These warnings are only in optimized builds, I assume - I don't see them with 4.4.3 debug builds here.  Personally, I'd prefer not to let the compiler influence the code here.

I acknowledge my view would be open to debate.  Otherwise, I guess, the compiler wouldn't be spitting out these warnings.  I once had a patch with similar unnecessary initialization, and on review dbaron suggested removing it.  His reasoning made sense to me so I've switched my preference to follow his.
Fair enough, but it also makes developers pay less attention to warnings
over time and likely also waste developer's time looking into warnings
that others have already investigated.  I wonder if that's not a bigger evil.
BTW, I see these warnings in my Fx debug build.
Attached file Assertion #2
Here's another one that is very noisy:
###!!! ASSERTION: Expected integer device offsets: 'PRUint32(device_offset_x) == device_offset_x && PRUint32(device_offset_y) == device_offs
et_y', file gfx/thebes/gfxXlibNativeRenderer.cpp, line 191

It looks like it's just a bogus assert expression (not allowing negative
offsets).  It's the same file so I thought you might want to fix it here.
I can file a new bug if you prefer.
(In reply to comment #11)
> BTW, I see these warnings in my Fx debug build.

Interesting.  Up to 4.3 these warnings weren't issued except with -O*.

http://gcc.gnu.org/onlinedocs/gcc-4.3.0/gcc/Warning-Options.html
"If you do not specify -O, you will not get these warnings."

The docs changed in gcc 4.4 to be more vague:

http://gcc.gnu.org/onlinedocs/gcc-4.4.0/gcc/Warning-Options.html#Warning-Options
"Because these warnings depend on optimization, the exact variables or elements for which there are warnings will depend on the precise optimization options and version of GCC used."

With "g++ (Gentoo 4.4.3-r2 p1.2) 4.4.3", I don't see warnings until I turn on -O.
With -O[1-3] I only get the warning on rect_count.
With -Os I get warnings for both needs_clip and rect_count.
With -Os -no-inline, I get no warnings.

Even with -Wno-init-self, "int rect_count = rect_count" doesn't suppress the warning.

Related discussion:
http://kerneltrap.org/node/6591
> ###!!! ASSERTION: Expected integer device offsets: 'PRUint32(device_offset_x)
> == device_offset_x && PRUint32(device_offset_y) == device_offs
> et_y', file gfx/thebes/gfxXlibNativeRenderer.cpp, line 191
> 
> It looks like it's just a bogus assert expression (not allowing negative
> offsets).

Yes, I agree.  Thanks again.
Attachment #474613 - Flags: review?(roc)
The "correct assertion test" patch is identical to bug 597915's patch, fwiw.
Attached file testcase #2
Comment on attachment 474593 [details] [diff] [review]
don't rely on cairo_clip_extents returning empty when the clip region is empty v2

Requesting approval2.0.  The assumption tested in this assertion was invalid, so the patch adjusts the code so as not to rely on the assumption.

The code is exercised by mochitests (as indicated here) but our test harness does not yet treat assertion failures as failed tests.  We can check in Jesse's testcase as a crashtest as assertion failures are detected there.

I haven't seen a case where the misassumption causes incorrect drawing, but that is theoretically possible without this patch.
Attachment #474593 - Flags: approval2.0?
Comment on attachment 474593 [details] [diff] [review]
don't rely on cairo_clip_extents returning empty when the clip region is empty v2

Please do check in Jesse's test as a crashtest!
Attachment #474593 - Flags: approval2.0? → approval2.0+
Attachment #474613 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/fe299c0eb024
http://hg.mozilla.org/mozilla-central/rev/a4acedb0dc7e

Modified the crashtest a bit so as not to depend on the document width.
(It was passing with the reftest window.)
http://hg.mozilla.org/mozilla-central/rev/44cc8965647e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: