Last Comment Bug 660670 - js::Parser::analyzeFunctions does not report OOM errors
: js::Parser::analyzeFunctions does not report OOM errors
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Paul Biggar
:
Mentors:
Depends on:
Blocks: 624094
  Show dependency treegraph
 
Reported: 2011-05-30 10:06 PDT by Paul Biggar
Modified: 2011-06-13 11:00 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple fix (972 bytes, patch)
2011-05-30 10:06 PDT, Paul Biggar
no flags Details | Diff | Splinter Review
move down into function (1.51 KB, patch)
2011-06-06 13:08 PDT, Paul Biggar
jimb: review+
Details | Diff | Splinter Review

Description Paul Biggar 2011-05-30 10:06:55 PDT
Created attachment 536119 [details] [diff] [review]
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 2011-06-02 19:04:44 PDT
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 2011-06-06 07:21:01 PDT
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 2011-06-06 10:04:08 PDT
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 2011-06-06 12:54:13 PDT
They're all member functions of Parser, which has a 'context' member.
Comment 5 Paul Biggar 2011-06-06 13:08:40 PDT
Created attachment 537614 [details] [diff] [review]
move down into function
Comment 6 Jim Blandy :jimb 2011-06-06 13:50:06 PDT
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 8 Chris Leary [:cdleary] (not checking bugmail) 2011-06-13 11:00:10 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f1d0cfb6673f

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