| Summary: | [chromium] Crash in tiled compositor memcpy | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adrienne Walker <enne> | ||||||||
| Component: | New Bugs | Assignee: | Adrienne Walker <enne> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, darin, enne, eric, jamesr, kbr, vangelis, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Other | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Adrienne Walker
2011-01-13 10:48:55 PST
Created attachment 78829 [details]
Patch
This relates to http://code.google.com/p/chromium/issues/detail?id=69458. The crash is in the read of the memcpy, but after rereading the code, it's not clear to me where this might be failing. I suspect that maybe it's the intersect call that's resulting in an empty rect (which resets the position to 0,0), causing a negative coordinate somewhere, which is causing a failed memory read. Comment on attachment 78829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78829&action=review OK, let's see whether this fixes it. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Need to get rid of this OOPS. Committed r75733: <http://trac.webkit.org/changeset/75733> This fix did not address the bug. Reopening. Created attachment 80381 [details]
Patch
(In reply to comment #6) > Created an attachment (id=80381) [details] > Patch I opened up a number of crash dumps to try to see what was going on, but they were internally inconsistent in a way that made me not trust their values and so were not that helpful. So, this is an attempt to get more information about what's going wrong and the CRASH values can be reverted later. Looks fine. Hopefully we'll get some useful info from it. Comment on attachment 80381 [details]
Patch
rs=me
Comment on attachment 80381 [details] Patch Clearing flags on attachment: 80381 Committed r76914: <http://trac.webkit.org/changeset/76914> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/76914 might have broken Leopard Intel Release (Tests) I didn't mean for this to get closed, since the last patch was just diagnostic. (Thanks for the quick review and cq+, kbr.) Created attachment 80473 [details]
Patch
(In reply to comment #14) > Created an attachment (id=80473) [details] > Patch I got a report from Mike Reed that one of these CRASH calls got triggered, but it looked like there was somehow an empty intersection between the tile and the invalid rect. However, from code inspection, this would not cause any trouble because the tile wouldn't be dirty. And, even if somehow the dirty rect was not contained by the tile rect, there's another intersection later in the file that should prevent this from being a problem when uploading the texture. Offhand, is it reasonable to upload a series of patches to the same bug, or do I need to open different bugs for each of these patches? Comment on attachment 80473 [details]
Patch
Looks fine.
(In reply to comment #15) > (In reply to comment #14) > > Created an attachment (id=80473) [details] [details] > > Patch > > I got a report from Mike Reed that one of these CRASH calls got triggered, but it looked like there was somehow an empty intersection between the tile and the invalid rect. However, from code inspection, this would not cause any trouble because the tile wouldn't be dirty. And, even if somehow the dirty rect was not contained by the tile rect, there's another intersection later in the file that should prevent this from being a problem when uploading the texture. > > Offhand, is it reasonable to upload a series of patches to the same bug, or do I need to open different bugs for each of these patches? I think it's reasonable to apply multiple patches from this bug. I've seen it done before on other bugs, and will make it easier to find them all later. Committed r76984: <http://trac.webkit.org/changeset/76984> Despite the fact that others have done it, it’s not good practice to use multiple patches in the same bug report. There are other ways to tie the bug reports and patches together, and the automation tools get confused. For example, in part due to the multiple patches at the moment it is unclear if this bug should be open or closed; it’s showing up in the “reviewed but possibly not landed” query. (In reply to comment #19) > Despite the fact that others have done it, it’s not good practice to use multiple patches in the same bug report. There are other ways to tie the bug reports and patches together, and the automation tools get confused. > > For example, in part due to the multiple patches at the moment it is unclear if this bug should be open or closed; it’s showing up in the “reviewed but possibly not landed” query. I'll keep in mind next time that it's not good practice. Thanks for the information. I don't know why this didn't get re-marked as closed when I landed the second patch. I'll just mark this as closed now. |