1326454 - Make TokenStream report OOM immediately

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jeff Walden [:Waldo]]

TokenStream::getChar and similar, if they parse a line terminator, record this fact by updating line information using TS::updateLineInfoForEOL.  This sometimes attempts to perform an allocation.  If that allocation fails, failure isn't recorded by returning false -- instead, a flag is set, then at some distant future time it gets checked and the error reported.

Aside from just being completely inconsistent with the usual error-reporting  idiom, this also has the apparent demerit of sometimes double-reporting, if line-ending information is updated (or rather, fails to be updated).

We should just fix this to report OOM immediately if it happens.  Some of the offset finicking will be a little tricky to massage into place with the normal idiom, but I'm not convinced what we do now is really totally correct, anyway.

I have patches for all of this *except* for this final bit of massagery.  Will attach what I have in a second.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Introduce TokenStream::error that reports an error at the current offset

This is a little bit cleanupy, but it makes this consistent with Parser's error reporting functions.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Introduce TokenStream::warning that warns at the current offset

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Rename TokenStream::getBracedUnicode to TokenStream::matchBracedUnicode and make its signature fallible

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Make TokenStream::peekChar's signature fallible

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Make TokenStream::skipChars{,IgnoreEOL} accept an unsigned integral number of chars to skip

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Add assertions to TokenStream::skipChars{,IgnoreEOL} verifying EOF isn't yet hit and that newlines aren't skipped, if appropriate

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8822736 [details] [diff] [review]
Rename TokenStream::getBracedUnicode to TokenStream::matchBracedUnicode and make its signature fallible

Review of attachment 8822736 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/irregexp/RegExpParser.cpp
@@ +242,5 @@
>  }
>  
>  template <typename CharT>
>  RegExpTree*
> +RegExpParser<CharT>::ReportError(unsigned errorNumber, const char* param)

Can we have /* = nullptr */ after param?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f7dfbbbc25
Introduce TokenStream::error that reports an error at the current offset.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1b2ddc8d0a
Introduce TokenStream::warning that warns at the current offset.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/043860a49efd
Rename TokenStream::getBracedUnicode to TokenStream::matchBracedUnicode and make its signature fallible.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0c9d78a8ea
Make TokenStream::peekChar's signature fallible.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ec29bcb862
Make TokenStream::skipChars{,IgnoreEOL} accept an unsigned integral number of chars to skip.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c260a2d1893e
Add assertions to TokenStream::skipChars{,IgnoreEOL} verifying EOF isn't yet hit and that newlines aren't skipped, if appropriate.  r=arai

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Don't report an error when SourceCoords::add fails, because it fails only when an underlying Vector::append fails, and that operation will report OOM

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6f7dfbbbc25
https://hg.mozilla.org/mozilla-central/rev/ee1b2ddc8d0a
https://hg.mozilla.org/mozilla-central/rev/043860a49efd
https://hg.mozilla.org/mozilla-central/rev/df0c9d78a8ea
https://hg.mozilla.org/mozilla-central/rev/c7ec29bcb862
https://hg.mozilla.org/mozilla-central/rev/c260a2d1893e

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27dda9c4c3dc
Don't report an error when SourceCoords::add fails, because it fails only when an underlying Vector::append fails, and that vector handles OOM reporting itself.  r=arai

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27dda9c4c3dc

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Make TokenStream::getChar's signature fallible

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Make TokenStream::updateLineInfoForEOL fallible, and immediately handle errors when it fails instead of temporarily deferring such handling. NOT REVIEWED YET

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Make TokenStream::getChar's signature fallible (w/minor typo fix)

Comment 16

[Jeff Walden [:Waldo]]

It would be nice, IMO, for a failure of getChar/etc. to result in an *immediate* return-false.  But current practice is to goto error, which does a bunch of stuff to poison the TokenStream and returned token so no one can use them, and it's probably best for this bug's chances if I leave that in place.  Notwithstanding that really we shouldn't be relying on such a crutch, IMO.

Comment 17

[Tooru Fujisawa [:arai]]

I'll review these patches maybe this weekend.
if it's urgent, feel free to redirect.

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8847827 [details] [diff] [review]
Make TokenStream::getChar's signature fallible (w/minor typo fix)

Review of attachment 8847827 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.cpp
@@ +1851,5 @@
>                      goto error;
>                  }
>                  if (!tokenbuf.append(c))
>                      goto error;
> +            } while (true);

just out of curiosity, is there any reason to use do-while(true) instead of while(true) ?

Comment 19

[Jeff Walden [:Waldo]]

I generally use do-while because it poses no mental overhead of having to process a useless condition to recognize the loop is always entered.  If I see |while (...) {| I have to look at the |...| to determine what happens.  Even if that's |1| or |true|, it still takes time.  Same for the alternative of |for (;;) {|.  But with do-while, seeing |do {| always indicates the body is entered.

The ideal solution to this, of course, would be having dedicated loop-forever syntax like Rust has, but I doubt this is going to come to C++ any time soon.

Comment 20

[Tooru Fujisawa [:arai]]

I see, thanks :)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32ea58f62527
Make TokenStream::getChar's signature fallible.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f852c28e71
Make TokenStream::updateLineInfoForEOL fallible, and immediately handle errors when it fails instead of temporarily deferring such handling.  r=arai

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32ea58f62527
https://hg.mozilla.org/mozilla-central/rev/65f852c28e71

Comment 23

[Jason Orendorff [:jorendorff]]

Everything landed. -> FIXED.