1492977 - Clean up OOM behaviour of AutoUnsafeCallWithABI calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Iain Ireland [:iain]]

Bug 1491350 and bug 1491337 revealed that our handling of OOM exceptions in VM functions had holes. Bug 1472574 is another example. There are a number of higher-level things we can do to help with these problems:

1. In every case where we currently use AutoUnsafeCallWithABI, AutoAssertNoPendingExceptions would also be correct. We should put an AAPNE inside of AUCw/ABI. (This is not inherent to AUCw/ABI; a future VM function might theoretically want AUCw/ABI without AAPNE. For now, though, we're better off with the check, and a comment to indicate that it is okay to opt out in the future if you're sure you know what you're doing.)

2. In most places, returning false implies that we're throwing an exception. This is not true for most (all?) of the functions that use AutoUnsafeCallWithABI. Many of these functions are labelled Pure; we should make that consistent. Ted's list of candidates: 
 EqualStringsHelper
 GetNativeDataProperty
 ValueToAtomOrSymbol
 GetNativeDataPropertyByValue
 SetNativeDataProperty
 ObjectHasGetterSetter
 HasNativeDataProperty
 HasNativeElement
 js::StringToNumberDontReportOOM
 ArgumentsObject::finishForIon
 NativeObject::growSlotsDontReportOOM
 NativeObject::addDenseElementDontReportOOM

3. AAPNE only asserts in the destructor. This can lead to confusion if cx->throwing is true before entering the scope of the AAPNE. I don't think that can ever happen. If that's right, we should put the same assertion in the constructor as well as the destructor to make it easier to 
tell what's going on. (This would have been useful in bug 1491350.)

4. Once this work is done, we should think about whether it's time to replace all these bools with an enum.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

To add to Ted's list, I added at least two more recently: 

* Int32ToStringHelper
* NumberToStringHelper

Comment 2

[Iain Ireland [:iain]]

I've implemented most of this. One fun problem: math_sqrt_handle, which contains an AutoUnsafeCallWithABI, is called from RSqrt::recover. RSqrt::recover can be called while we are bailing out of Ion because of an exception. So it isn't safe to assert that math_sqrt_handle should never return while an exception is pending.

Semi-related: we currently have two very similar classes -- AutoAssertNoPendingException and AutoAssertNoException -- that only differ in their behaviour when they are called with an already pending exception. The difference is that AANPE asserts when it goes out of scope if we are currently throwing an exception, but AANE only asserts when it goes out of scope if we are currently throwing an exception *and* we weren't already throwing when it was constructed. AANE was only called in two places (js::NativeGetPropertyNoGC and js::LookupNameNoGC), neither of which had any good reason to use AANE over AANPE. 

Initially, my plan was to unify AANE and AANPE by getting rid of AANE entirely. However, math_sqrt_handle is an example of a function that *does* have a good reason to prefer AANE over AANPE. Right now, I am leaning towards removing AANE, replacing its users with AANPE, and folding the existing AANE behaviour into AutoUnsafeCallWithABI (turned on by default, but with a constructor argument to ignore).

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1492977: Mark returns-false-for-retry functions as Pure r?tcampbell

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1492977: Rework RAII exception guards r?tcampbell

Depends on D7942

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81dd098adbd8
Mark returns-false-for-retry functions as Pure r=tcampbell

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/838b2692a934
Rework RAII exception guards r=tcampbell

Comment 7

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/81dd098adbd8

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/838b2692a934

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:iain, maybe it's time to close this bug?