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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mats, Assigned: karlt)

Tracking

Trunk
mozilla2.0b8
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 474587 [details]
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
(Reporter)

Comment 2

8 years ago
Created attachment 474588 [details] [diff] [review]
Like so? (diff -w)
Attachment #474588 - Flags: review?(karlt)
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
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-
(Assignee)

Comment 5

8 years ago
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
(Assignee)

Updated

8 years ago
Blocks: 580440
(Assignee)

Comment 6

8 years ago
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)
(Assignee)

Comment 7

8 years ago
Created attachment 474592 [details] [diff] [review]
don't rely on cairo_clip_extents returning empty when the clip region is empty

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)
(Assignee)

Updated

8 years ago
Attachment #474587 - Flags: review?(roc)
(Assignee)

Comment 8

8 years ago
Created attachment 474593 [details] [diff] [review]
don't rely on cairo_clip_extents returning empty when the clip region is empty v2

Move the screen check with the visual check.
Attachment #474592 - Attachment is obsolete: true
Attachment #474593 - Flags: review?(roc)
Attachment #474592 - Flags: review?(roc)
(Reporter)

Comment 9

8 years ago
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)
(Assignee)

Comment 10

8 years ago
(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.
(Reporter)

Comment 11

8 years ago
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.
(Reporter)

Comment 12

8 years ago
Created attachment 474600 [details]
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.
(Assignee)

Comment 13

8 years ago
(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
(Assignee)

Comment 14

8 years ago
Created attachment 474613 [details] [diff] [review]
correct assertion test to accept negative device offsets

> ###!!! 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)

Comment 15

8 years ago
The "correct assertion test" patch is identical to bug 597915's patch, fwiw.

Comment 16

8 years ago
Created attachment 485546 [details]
testcase #2
(Assignee)

Comment 17

8 years ago
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+
(Assignee)

Updated

8 years ago
Attachment #474613 - Attachment is obsolete: true
(Assignee)

Comment 19

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.