Closed Bug 1239369 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files)

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.
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)
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+
https://hg.mozilla.org/mozilla-central/rev/a83f3effa278
https://hg.mozilla.org/mozilla-central/rev/df444117c7be
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
No longer depends on: 1370905
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.