Bug 191563

Summary:

Add OOM detection to StringPrototype's substituteBackreferences().

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cmarcelo, dbates, ews-watchlist, fpizlo, keith_miller, msaboff, rmorisset, rniwa, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed patch.	msaboff: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews116 for mac-sierra	none
proposed patch.	saam: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews102 for mac-sierra	none
Archive of layout-test-results from ews107 for mac-sierra-wk2	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
proposed patch.	saam: review+

Mark Lam

Reported 2018-11-12 14:29:37 PST

<rdar://problem/45720428>

Attachments
proposed patch. (10.74 KB, patch) 2018-11-12 14:41 PST, Mark Lam	msaboff: review+ ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews116 for mac-sierra (3.70 MB, application/zip) 2018-11-12 17:36 PST, EWS Watchlist	no flags	Details
proposed patch. (10.53 KB, patch) 2018-11-12 18:25 PST, Mark Lam	saam: review+ ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-sierra (465.85 KB, application/zip) 2018-11-12 19:22 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (1.95 MB, application/zip) 2018-11-12 19:28 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.14 MB, application/zip) 2018-11-12 19:43 PST, EWS Watchlist	no flags	Details
proposed patch. (12.20 KB, patch) 2018-11-12 19:59 PST, Mark Lam	saam: review+	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Mark Lam

Comment 1 2018-11-12 14:41:04 PST

Created attachment 354593 [details] proposed patch.

Michael Saboff

Comment 2 2018-11-12 14:55:09 PST

Comment on attachment 354593 [details] proposed patch. r=me

EWS Watchlist

Comment 3 2018-11-12 17:36:30 PST

Comment on attachment 354593 [details] proposed patch. Attachment 354593 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9964318 New failing tests: webgl/2.0.0/conformance/glsl/misc/shader-struct-scope.html imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.worker.html webgl/2.0.0/conformance/glsl/misc/shaders-with-uniform-structs.html

EWS Watchlist

Comment 4 2018-11-12 17:36:31 PST

Created attachment 354613 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6

Mark Lam

Comment 5 2018-11-12 18:25:15 PST

Created attachment 354618 [details] proposed patch.

Saam Barati

Comment 6 2018-11-12 18:42:01 PST

Comment on attachment 354618 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=354618&action=review > Source/WTF/ChangeLog:3 > + Add OOM detection to StrongPrototype's substituteBackreferences(). You mean StringPrototype here and everywhere else

Mark Lam

Comment 7 2018-11-12 18:45:50 PST

Thanks for the review. (In reply to Saam Barati from comment #6) > Comment on attachment 354618 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354618&action=review > > > Source/WTF/ChangeLog:3 > > + Add OOM detection to StrongPrototype's substituteBackreferences(). > > You mean StringPrototype here and everywhere else Fixed locally.

EWS Watchlist

Comment 8 2018-11-12 19:22:56 PST

Comment on attachment 354618 [details] proposed patch. Attachment 354618 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9966814 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 9 2018-11-12 19:22:57 PST

Created attachment 354627 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 10 2018-11-12 19:28:30 PST

Comment on attachment 354618 [details] proposed patch. Attachment 354618 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9966822 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 11 2018-11-12 19:28:32 PST

Created attachment 354628 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 12 2018-11-12 19:43:19 PST

Comment on attachment 354618 [details] proposed patch. Attachment 354618 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9966802 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 13 2018-11-12 19:43:21 PST

Created attachment 354631 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Mark Lam

Comment 14 2018-11-12 19:59:48 PST

Created attachment 354632 [details] proposed patch.

Mark Lam

Comment 15 2018-11-12 20:01:12 PST

Holding off on r? until I get more testing done in case StringBuilder has more surprises for me.

Mark Lam

Comment 16 2018-11-12 22:44:38 PST

Comment on attachment 354632 [details] proposed patch. OK, the patch is ready for a review now.

Saam Barati

Comment 17 2018-11-13 09:31:17 PST

Comment on attachment 354632 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=354632&action=review > Source/WTF/ChangeLog:10 > + the hasOverflowed() check if m_string is not null. You should say why > Source/WTF/ChangeLog:12 > + StringBuffer can only overflow if m_buffer is non-null. shrinkToFit() only I don’t understand what this paragraph is trying to show

Mark Lam

Comment 18 2018-11-13 13:08:23 PST

Comment on attachment 354632 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=354632&action=review Thanks for there review. >> Source/WTF/ChangeLog:10 >> + the hasOverflowed() check if m_string is not null. > > You should say why I did try to say why. It's in the paragraphs below. I'll re-write these paragraphs here to be clearer.

Mark Lam

Comment 19 2018-11-13 13:12:06 PST

(In reply to Mark Lam from comment #18) > >> Source/WTF/ChangeLog:10 > >> + the hasOverflowed() check if m_string is not null. > > > > You should say why > > I did try to say why. It's in the paragraphs below. I'll re-write these > paragraphs here to be clearer. Looks like I also had some typos which would further confuse things. Anyway, I'm fixing it.

Mark Lam

Comment 20 2018-11-13 13:34:15 PST

Landed in r238143: <http://trac.webkit.org/r238143>.

Note You need to log in before you can comment on or make changes to this bug.