Bug 194962 - Add an exception check and some assertions in StringPrototype.cpp.
Summary: Add an exception check and some assertions in StringPrototype.cpp.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-22 15:54 PST by Mark Lam
Modified: 2019-02-23 10:16 PST (History)
13 users (show)

See Also:

Attachments
proposed patch. (13.34 KB, patch)
2019-02-22 16:01 PST, Mark Lam 		ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing w/ build fixes. (13.45 KB, patch)
2019-02-22 16:28 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
patch for landing. (13.45 KB, patch)
2019-02-22 16:56 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
patch for landing (uploaded to hopefully tickle the EWS bots into running). (13.45 KB, patch)
2019-02-22 17:18 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
patch for landing. (13.64 KB, patch)
2019-02-22 17:42 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
patch for landing: 6th time's the charm. (13.66 KB, patch)
2019-02-22 17:52 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) 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 Mark Lam 2019-02-22 15:54:25 PST 
<rdar://problem/48013416>
Comment 1 Mark Lam 2019-02-22 16:01:36 PST 
Created attachment 362775 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2019-02-22 16:06:53 PST 
Comment on attachment 362775 [details]
proposed patch.

r=me
Comment 3 Saam Barati 2019-02-22 16:07:53 PST 
Comment on attachment 362775 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=362775&action=review

> Source/WTF/wtf/CheckedArithmetic.h:545
> +inline bool observesOverflows() { return true; }

constexpr?

I'd also name this "ovservesOverflow"

> Source/WTF/wtf/CheckedArithmetic.h:548
> +inline bool observesOverflows<AssertNoOverflow>() { return !ASSERT_DISABLED; }

constexpr?
Comment 4 Mark Lam 2019-02-22 16:27:16 PST 
Thanks for the reviews.

(In reply to Saam Barati from comment #3)
> Comment on attachment 362775 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362775&action=review
> 
> > Source/WTF/wtf/CheckedArithmetic.h:545
> > +inline bool observesOverflows() { return true; }
> 
> constexpr?
> 
> I'd also name this "ovservesOverflow"
> 
> > Source/WTF/wtf/CheckedArithmetic.h:548
> > +inline bool observesOverflows<AssertNoOverflow>() { return !ASSERT_DISABLED; }
> 
> constexpr?

I'll apply these changes.
Comment 5 Mark Lam 2019-02-22 16:28:54 PST 
Created attachment 362784 [details]
patch for landing w/ build fixes.

Let's see if the EWS likes this better.
Comment 6 Mark Lam 2019-02-22 16:56:18 PST 
Created attachment 362790 [details]
patch for landing.

Let's try the EWS again with a different fix.
Comment 7 Mark Lam 2019-02-22 17:18:27 PST 
Created attachment 362800 [details]
patch for landing (uploaded to hopefully tickle the EWS bots into running).
Comment 8 Mark Lam 2019-02-22 17:42:38 PST 
Created attachment 362805 [details]
patch for landing.

The issue does not repro on my local builds.  So let's try another speculative build fix.
Comment 9 Mark Lam 2019-02-22 17:52:45 PST 
Created attachment 362807 [details]
patch for landing: 6th time's the charm.
Comment 10 Mark Lam 2019-02-23 10:16:19 PST 
Landed in r241991: <http://trac.webkit.org/r241991>.