Closed Bug 1492977 Opened 2 years ago Closed 1 year ago

Clean up OOM behaviour of AutoUnsafeCallWithABI calls

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: iain, Assigned: iain)

References

Details

Attachments

(2 files)

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.
To add to Ted's list, I added at least two more recently: 

* Int32ToStringHelper
* NumberToStringHelper
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).
Depends on D7942
Assignee: nobody → iireland
Priority: -- → P3
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81dd098adbd8
Mark returns-false-for-retry functions as Pure r=tcampbell
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/838b2692a934
Rework RAII exception guards r=tcampbell

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

Flags: needinfo?(iireland)
Flags: needinfo?(iireland)
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.