Bug 85393 - Convert FractionalLayoutUnit overflow assertions to stderr warnings
Summary: Convert FractionalLayoutUnit overflow assertions to stderr warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency tree / graph
 
Reported: 2012-05-02 11:22 PDT by Levi Weintraub
Modified: 2012-05-02 19:00 PDT (History)
3 users (show)

See Also:

Attachments
Patch (3.98 KB, patch)
2012-05-02 11:29 PDT, Levi Weintraub 		no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2012-05-02 16:06 PDT, Levi Weintraub 		no flags Details | Formatted Diff | Diff
Patch (4.02 KB, patch)
2012-05-02 17:04 PDT, Levi Weintraub 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 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.
Comment 1 Levi Weintraub 2012-05-02 11:29:19 PDT 
Created attachment 139845 [details]
Patch
Comment 2 Eric Seidel (no email) 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.
Comment 3 Eric Seidel (no email) 2012-05-02 15:53:01 PDT 
I thought you planned to do the stdout version?
Comment 4 Levi Weintraub 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!
Comment 5 Levi Weintraub 2012-05-02 16:06:14 PDT 
Created attachment 139903 [details]
Patch
Comment 6 Eric Seidel (no email) 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.
Comment 7 Levi Weintraub 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.
Comment 8 Build Bot 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
Comment 9 Levi Weintraub 2012-05-02 16:56:22 PDT 
Going back to stderr after talking with eseidel on irc.
Comment 10 Levi Weintraub 2012-05-02 17:04:04 PDT 
Created attachment 139919 [details]
Patch
Comment 11 Eric Seidel (no email) 2012-05-02 17:18:13 PDT 
Comment on attachment 139919 [details]
Patch

Thanks.
Comment 12 WebKit Review Bot 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>
Comment 13 WebKit Review Bot 2012-05-02 19:00:57 PDT 
All reviewed patches have been landed.  Closing bug.