Restore oomTest's check that an is exception thrown on failure

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

3 years ago
Bug 1219905 removed oomTest's check that an exception is always thrown on failure because there are some places where this doesn't happen.  This is unfortunate because it means we no longer check that ReportOutOfMemory is called correctly.

Instead we can make this an option to oomTest where the default behaviour is to include the check and turn this off in the cases where it doesn't apply.  We will also have to disable it when --fuzzing-safe is passed.
Assignee

Comment 1

3 years ago
As above.  Add an argument to oomTest() to indicate whether it should expects an exception to be thrown if execution fails, defaulting to true.  Pass false in the one case where this does not happen.
Attachment #8707881 - Flags: review?(terrence)
Assignee

Comment 2

3 years ago
Add missing calls to ReportOutOfMemory to make the tests pass again.
Attachment #8707882 - Flags: review?(terrence)
Attachment #8707882 - Flags: review?(terrence) → review+
Comment on attachment 8707881 [details] [diff] [review]
bug1239369-make-oomTest-check-for-exception

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

::: js/src/builtin/TestingFunctions.cpp
@@ +3285,5 @@
>  "  repeatedly executing it and simulating allocation failure at successive\n"
> +"  allocations until the function completes without seeing a failure.\n"
> +"  By default this tests that an exception is raised if execution fails, but\n"
> +"  this can be disabled by passing false as the optional second parameter."
> +),

Probably also worth mentioning that the check is disabled in --fuzzing-safe: it's usually included in the fuzzer's STR and that's when we're going to be most confused when toggling this arg does nothing.
Attachment #8707881 - Flags: review?(terrence) → review+

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a83f3effa278
https://hg.mozilla.org/mozilla-central/rev/df444117c7be
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1353356
Depends on: 1370905
Assignee

Updated

2 years ago
No longer depends on: 1370905

Comment 6

8 months ago
Steve: It looks like this bug caused a regression in bug 1491326.  (This is the only bug in the regression range).  Jon is out on PTO till Oct.  Can you investigate in the mean time?
Depends on: 1491326
Flags: needinfo?(sdetar)
I am going to let Jon investigate this when he gets back from PTO in a couple of days.
Flags: needinfo?(sdetar)
You need to log in before you can comment on or make changes to this bug.