Closed Bug 519004 Opened 12 years ago Closed 12 years ago

make deep abort danger more clear and present


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Unassigned)



(2 files)

The proposed patch changes some common names, hence the big cc list.

Bug 517973 removed the deep abort stack, but there is still the requirement that after a deep abort, which can occur nested in recorder logic, that, after the deep abort is detected, control flow leave the recorder directly.  Right now this requirement is implicit and rather fragile.  I started to make a mistake this morning, hence this bug.

The attached patch makes the deep abort requirement more visible by adding a new enumeration JSAbortableRecordingStatus which is the return type of recorder functions that can contain, transitively, a deep abort.  This new enumeration contains the same enumerators as JSRecordingStatus (except named JSARS_X instead of JSRS_X) with one extra value: JSARS_ABORTED.  This return value means "recording already aborted; recorder already deleted".  JS[A]RS_STOP still means the more flexible "recording should be aborted after exiting the call stack".

The current CHECK_STATUS, ABORT_TRACE, ABORT_TRACE_ERROR, and ABORT_IF_XML macros return JSRS_X values, so versions of these macros were needed to return the JSARS_X variants.  Since this involved touching them anyhow, and after discussing it in the pit, I thought I should update the ABORT* names.  Thus, the macro names in the patch are:

ABORT_TRACE(msg)       ==> RETURN_STOP(msg),   RETURN_STOP_A(msg)

There is also a function to "inject" values from JSRecordingStatus into JSAbortableRecordingStatus that is often needed.  There is a comment in the patch on this above the JSRecordingStatus enum definition.

In the course of normal C++ typechecking, the JSAbortableRecordingStatus tends to be required to spread from the initial point where deep abort is detected (like right after calling ExecuteTree), to callers.  Hence, and as you can see in the patch, the deep abort dangers mentioned above should be more locally and syntactically apparent in the code.

The patch is quite large, but mostly syntactic, with a few small control flow changes.

Comments?  New names ok?
JSRS was ugly, JSARS is ugly^2. But, I don't have a better suggestion either. Using the type checker to prove where abort sources are is definitively an improvement. I like it.
Comment on attachment 403010 [details] [diff] [review]
recording status changes

It's a good patch, even if big.

Random nits:

* case and goto labels are half-indented (two spaces).

* existing nit, but you are touching it: RETURN_VALUE_IF_XML needs its body lines (bracketed by JS_{BEGIN,END}_MACRO) indented one basic offset, and the overlong arg list wrapped with consequent bracing.

* looks like this is not sync'ed up to tm tip, from the void guardShape(...) signature (I changed it today).

Quick drive-by, I'm too tired to do the full thing, but it looks good so far other than nits.

(In reply to comment #1)
> JSRS was ugly, JSARS is ugly^2.

Well, this is the patch to change it!  Hmm.  Hard to beat how short an acronym is.  Though longer, perhaps RECORD_STOP/ARECORD_STOP, etc?  (Dropping the "JS" prefix, since this enum is private to tracer impl details.)

(In reply to comment #2)
> * looks like this is not sync'ed up to tm tip, from the void guardShape(...)
> signature (I changed it today).

Yeah, I was trying to batch rebases... big patch and all :)
Attachment #403271 - Flags: review?
Comment on attachment 403271 [details] [diff] [review]
nits and name tweaks

(oops, hit <Enter> without comment)

This patch addresses comment 2, and comment 1, by renaming JSRS_* to RECORD_* and JSARS_* to ARECORD_*.  I also stripped the JS from JS[Abortable]RecordingStatus since its an internal enum and will [should] eventually be in a tracer-private header.
Attachment #403271 - Flags: review? → review?(dvander)
Attachment #403271 - Flags: review?(dvander) → review+
Pushing while I still don't have to rebase.  If better names are decided upon, I'll be happy to do the s/A/B/ patch.
Names are fine!

Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.