Bug 191563 – Add OOM detection to StringPrototype's substituteBackreferences().

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

<rdar://problem/45720428>

[Mark Lam]

Created attachment 354593 [details]
proposed patch.

[Michael Saboff]

Comment on attachment 354593 [details]
proposed patch.

r=me

[EWS Watchlist]

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]

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]

Created attachment 354618 [details]
proposed patch.

[Saam Barati]

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]

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 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]

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 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]

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 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]

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]

Created attachment 354632 [details]
proposed patch.

[Mark Lam]

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

[Mark Lam]

Comment on attachment 354632 [details]
proposed patch.

OK, the patch is ready for a review now.

[Saam Barati]

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 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]

(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]

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