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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paul Biggar, Assigned: Paul Biggar)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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?
Attachment #536119 - Flags: review?(jimb)

Comment 1

6 years ago
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

6 years ago
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.
(Assignee)

Comment 3

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

Comment 4

6 years ago
They're all member functions of Parser, which has a 'context' member.
(Assignee)

Comment 5

6 years ago
Created attachment 537614 [details] [diff] [review]
move down into function
Assignee: general → pbiggar
Attachment #536119 - Attachment is obsolete: true
Attachment #536119 - Flags: review?(jimb)
Attachment #537614 - Flags: review?(jimb)

Comment 6

6 years ago
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.
Attachment #537614 - Flags: review?(jimb) → review+
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/tracemonkey/rev/f1d0cfb6673f
Whiteboard: [fixed-in-tracemonkey]
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f1d0cfb6673f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.