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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1239369-make-oomTest-check-for-exception

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.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1239369-fix-missing-ReportOutOfMemory

Add missing calls to ReportOutOfMemory to make the tests pass again.

Comment 3

[Terrence Cole [:terrence]]

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.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a83f3effa278
https://hg.mozilla.org/integration/mozilla-inbound/rev/df444117c7be

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a83f3effa278
https://hg.mozilla.org/mozilla-central/rev/df444117c7be

Comment 6

[Jason Duell]

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?

Comment 7

[Steven DeTar [:sdetar]]

I am going to let Jon investigate this when he gets back from PTO in a couple of days.