Clean up OOM behaviour of AutoUnsafeCallWithABI calls

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 months ago
21 days ago

People

(Reporter: iain, Assigned: iain)

Tracking

(Depends on 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

8 months ago
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
Assignee

Comment 2

8 months ago
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).
Assignee

Comment 4

8 months ago
Depends on D7942
Assignee: nobody → iireland
Priority: -- → P3
Keywords: leave-open

Comment 5

7 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81dd098adbd8
Mark returns-false-for-retry functions as Pure r=tcampbell

Comment 6

7 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/838b2692a934
Rework RAII exception guards r=tcampbell
Depends on: 1498303
Depends on: 1499010

Updated

7 months ago
Depends on: 1502853

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)
Assignee

Updated

23 days ago
Flags: needinfo?(iireland)
Keywords: leave-open
Assignee

Updated

21 days ago
Status: NEW → RESOLVED
Last Resolved: 21 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.