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)
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: MatsPalmgren_bugz, Assigned: karlt)
References
()
Details
Attachments
(4 files, 3 obsolete files)
13.57 KB,
text/plain
|
Details | |
4.97 KB,
patch
|
roc
:
review+
joe
:
approval2.0+
|
Details | Diff | Splinter Review |
8.37 KB,
text/plain
|
Details | |
490 bytes,
text/html
|
Details |
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•14 years ago
|
||
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•14 years ago
|
||
Attachment #474588 -
Flags: review?(karlt)
Assignee | ||
Comment 3•14 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•14 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•14 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 | ||
Comment 6•14 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•14 years ago
|
||
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•14 years ago
|
Attachment #474587 -
Flags: review?(roc)
Assignee | ||
Comment 8•14 years ago
|
||
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•14 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•14 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•14 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•14 years ago
|
||
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•14 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•14 years ago
|
||
> ###!!! 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)
Attachment #474613 -
Flags: review?(roc) → review+
Attachment #474593 -
Flags: review?(roc) → review+
Comment 15•14 years ago
|
||
The "correct assertion test" patch is identical to bug 597915's patch, fwiw.
Comment 16•14 years ago
|
||
Assignee | ||
Comment 17•14 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 18•14 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 Please do check in Jesse's test as a crashtest!
Attachment #474593 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Updated•14 years ago
|
Attachment #474613 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 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
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.
Description
•