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+
3 years ago
Depends on: 1269710
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
I am going to let Jon investigate this when he gets back from PTO in a couple of days.
You need to log in before you can comment on or make changes to this bug.