660670 - js::Parser::analyzeFunctions does not report OOM errors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

Attachment: simple fix

On OOM, Parser::analyzeFunction does not report an error, but just returns NULL/false. Other callers around there handle their own error checking.

I think analyzeFunction doesn't because it has no cx parameter to use for error checking. A better way to handle this might be to pass a cx instead?

Comment 1

[Jim Blandy :jimb]

Mea culpa, I didn't get to this by the end of the day. I'll be gone Friday June 3, but back on Monday. This is at the top of my queue.

Comment 2

[Jim Blandy :jimb]

Comment on attachment 536119 [details] [diff] [review]
simple fix

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

I think it's only acceptable for functions to use a false/null return to mean "out of memory" when allocating memory is their sole responsibility. Functions like analyzeFunctions should follow the same convention all the other functions do: that a false/NULL return value means that the error has already been properly reported.

If I'm reading right, that just means moving the call to js_ReportOutOfMemory to the call to queue.init at the top of Parser::markFunArgs --- that's the only thing analyzeFunctions calls that could actually return false.

Comment 3

[Paul Biggar]

Moving the js_ReportOutOfMemory call to Parser::markFunArgs means adding JSContext parameters to a few functions. Is that a problem?

Comment 4

[Jim Blandy :jimb]

They're all member functions of Parser, which has a 'context' member.

Comment 5

[Paul Biggar]

Attachment: move down into function

Comment 6

[Jim Blandy :jimb]

Comment on attachment 537614 [details] [diff] [review]
move down into function

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

r=me, with the extraneous braces gone.

::: js/src/jsparse.cpp
@@ +1040,2 @@
>              goto out;
> +        }

No need for extra braces here.

@@ +2199,2 @@
>          return false;
> +    }

Looks great.

Comment 7

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/f1d0cfb6673f

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f1d0cfb6673f