Bug 52379 – [chromium] Crash in tiled compositor memcpy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Crash in tiled compositor memcpy

[Adrienne Walker]

Created attachment 78829 [details]
Patch

[Adrienne Walker]

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.

[Kenneth Russell]

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.

[Adrienne Walker]

Committed r75733: <http://trac.webkit.org/changeset/75733>

[Adrienne Walker]

This fix did not address the bug.  Reopening.

[Adrienne Walker]

Created attachment 80381 [details]
Patch

[Adrienne Walker]

(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.

[Vangelis Kokkevis]

Looks fine.  Hopefully we'll get some useful info from it.

[Kenneth Russell]

Comment on attachment 80381 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 80381 [details]
Patch

Clearing flags on attachment: 80381

Committed r76914: <http://trac.webkit.org/changeset/76914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76914 might have broken Leopard Intel Release (Tests)

[Adrienne Walker]

I didn't mean for this to get closed, since the last patch was just diagnostic.  (Thanks for the quick review and cq+, kbr.)

[Adrienne Walker]

Created attachment 80473 [details]
Patch

[Adrienne Walker]

(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?

[Kenneth Russell]

Comment on attachment 80473 [details]
Patch

Looks fine.

[Kenneth Russell]

(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.

[Adrienne Walker]

Committed r76984: <http://trac.webkit.org/changeset/76984>

[Darin Adler]

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.

[Adrienne Walker]

(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.