Bug 85393

Summary: Convert FractionalLayoutUnit overflow assertions to stderr warnings
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eae, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Levi Weintraub
Reported 2012-05-02 11:22:26 PDT
It's quite useful to be aware when overflows are occurring, but it's not always a programming error. With this change, we'll write to stderr on debug builds when an overflow occurs, but not crash.
Attachments
Patch (3.98 KB, patch)
2012-05-02 11:29 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (4.16 KB, patch)
2012-05-02 16:06 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (4.02 KB, patch)
2012-05-02 17:04 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2012-05-02 11:29:19 PDT
Created attachment 139845 [details] Patch
Eric Seidel (no email)
Comment 2 2012-05-02 11:47:49 PDT
Comment on attachment 139845 [details] Patch Hmm... I suspect many callers will be surprised by overflow. I wonder if we need to have these taken an Enum instead, which is AllowOverflow and ASSERT when we overflow and that enum is false (default). Then we could white-list the test cases we care about? I feel like these stderr logs will mostly just get lost.
Eric Seidel (no email)
Comment 3 2012-05-02 15:53:01 PDT
I thought you planned to do the stdout version?
Levi Weintraub
Comment 4 2012-05-02 15:55:58 PDT
(In reply to comment #3) > I thought you planned to do the stdout version? Sorry, I did and landed it in our branch but forgot to upload it!
Levi Weintraub
Comment 5 2012-05-02 16:06:14 PDT
Created attachment 139903 [details] Patch
Eric Seidel (no email)
Comment 6 2012-05-02 16:09:45 PDT
Comment on attachment 139903 [details] Patch OK. This seems like the best we've come up with so far. The big question remaining is what to do with the tests which now only fail in Debug mode. If any of those are ever *supposed* to trigger overflow, they'll now never be able to pass in both Debug/Release which is bad. If there exist such tests, then it seems we'll need to move this back to stderr.
Levi Weintraub
Comment 7 2012-05-02 16:15:03 PDT
(In reply to comment #6) > (From update of attachment 139903 [details]) > OK. This seems like the best we've come up with so far. The big question remaining is what to do with the tests which now only fail in Debug mode. If any of those are ever *supposed* to trigger overflow, they'll now never be able to pass in both Debug/Release which is bad. If there exist such tests, then it seems we'll need to move this back to stderr. Agreed. The majority of tests that trigger this is the result of bug 68744, which is incorrect behavior.
Build Bot
Comment 8 2012-05-02 16:48:32 PDT
Comment on attachment 139903 [details] Patch Attachment 139903 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12613082
Levi Weintraub
Comment 9 2012-05-02 16:56:22 PDT
Going back to stderr after talking with eseidel on irc.
Levi Weintraub
Comment 10 2012-05-02 17:04:04 PDT
Created attachment 139919 [details] Patch
Eric Seidel (no email)
Comment 11 2012-05-02 17:18:13 PDT
Comment on attachment 139919 [details] Patch Thanks.
WebKit Review Bot
Comment 12 2012-05-02 19:00:46 PDT
Comment on attachment 139919 [details] Patch Clearing flags on attachment: 139919 Committed r115928: <http://trac.webkit.org/changeset/115928>
WebKit Review Bot
Comment 13 2012-05-02 19:00:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.