Bug 87811

Summary: correct the sequence of diagnostic crash check
Product: WebKit Reporter: Lu Guanqun <guanqun.lu>
Component: New BugsAssignee: Lu Guanqun <guanqun.lu>
Status: RESOLVED WONTFIX    
Severity: Normal CC: cc-bugs, enne, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch jamesr: review-

Lu Guanqun
Reported 2012-05-29 19:16:56 PDT
correct the sequence of diagnostic crash check
Attachments
Patch (1.46 KB, patch)
2012-05-29 19:18 PDT, Lu Guanqun 		jamesr: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Lu Guanqun
Comment 1 2012-05-29 19:18:10 PDT
Created attachment 144678 [details] Patch
Lu Guanqun
Comment 2 2012-05-29 19:26:29 PDT
We should do the null check before we de-reference the pointer. Otherwise, we don't get the benefits of crash check.
James Robinson
Comment 3 2012-05-29 21:00:04 PDT
Comment on attachment 144678 [details] Patch I think you slightly misunderstand the meaning of the tileAt() check - we're trying to verify that _after_ adding the tile it exists. Moving the CHECK() for addedTile seems reasonable.
Lu Guanqun
Comment 4 2012-05-29 21:58:40 PDT
(In reply to comment #3) > (From update of attachment 144678 [details]) > I think you slightly misunderstand the meaning of the tileAt() check - we're trying to verify that _after_ adding the tile it exists. Moving the CHECK() for addedTile seems reasonable. It does check the tile _after_ it's been added, since it's after the line "m_tiler->addTile(tile.release(), i, j);". The line "addedTile->dirtyRect = m_tiler->tileRect(addedTile);" doesn't add the tile, does it? Do I miss something here?
James Robinson
Comment 5 2012-05-29 22:11:40 PDT
Comment on attachment 144678 [details] Patch Ah sorry, you're right. Still not sure it's worth churning the line numbers, we're going to remove these lines in a few days anyway.
Lu Guanqun
Comment 6 2012-05-29 22:18:54 PDT
(In reply to comment #5) > (From update of attachment 144678 [details]) > Ah sorry, you're right. Still not sure it's worth churning the line numbers, we're going to remove these lines in a few days anyway. OK. It's good to know that these lines would be removed. So I will mark this bug as won't fix. Thanks for the review anyway.
Note You need to log in before you can comment on or make changes to this bug.