1836082 - Add OOM Handling assertions to Sprinter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

"How about adding assertion to Sprinter::{string,stringEnd,release,stringAt,operator[]} and all other methods that accesses the resulting string, to check if Sprinter::hadOutOfMemory is called, depending on Sprinter::shouldReportOOM?
So that it's guaranteed that OOM check is always performed for non-error case."

from https://phabricator.services.mozilla.com/D179477#inline-994670

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #0)

"How about adding assertion to Sprinter::{string,stringEnd,release,stringAt,operator[]} and all other methods that accesses the resulting string, to check if Sprinter::hadOutOfMemory is called, depending on Sprinter::shouldReportOOM?
So that it's guaranteed that OOM check is always performed for non-error case."

I am not sure to understand the need for shouldReportOOM, which reports the fact that the JSCotnext would be used to report the allocation failure.

In all cases, accessors should be forbidden access if any OOM happened before.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1836082 - Verify that we never access strings content after a OOM had been reported.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:nbp, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f44bd42ebc2
Verify that we never access strings content after a OOM had been reported. r=mgaudet

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1f44bd42ebc2