1004088 - Clean up ErrorCopier

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

ErrorCopier takes a Maybe<AutoCompartment>. I'm told this is because it used to be fallible to enter a compartment. Since that's no longer true, it's no longer necessary.

It also requires callers to write very strange looking code:

mozilla::Maybe<Autocompartment> ac;
ac.construct(cx, obj);

We can clean this up. We have the power.

Comment 1

[Eric Faust [:efaust]]

OK, so the main barrier to completion here is that we are hack-and-slashing our way out of the AutoCompartment in the middle of the ErrorCopier destructor.

>        if (cx->getPendingException(&exc) && exc.isObject() && exc.toObject().is<ErrorObject>()) {
>            cx->clearPendingException();
>            ac.destroy();
>            Rooted<ErrorObject*> errObj(cx, &exc.toObject().as<ErrorObject>());
>            JSObject *copyobj = js_CopyErrorObject(cx, errObj, scope);
>            if (copyobj)
>                cx->setPendingException(ObjectValue(*copyobj));
>        }

This is hard to remove, because of destructor semantics. Our options here seem to be:

1) leave well enough alone, the maybe<> solution allows this (somewhat distasteful) behavior cleanly.
2) add a method to AutoCompartment. AutoCompartment::leave(), or something. Which leaves the origin compartment, and sets a flag for the descructor so we don't do it again. This isn't horrifying, but we need to agree that we're going to do it.
3) Something cleverer than I've got at the moment.

To be honest, ::leave() isn't the scariest thing I've ever heard of.

Comment 2

[Eric Faust [:efaust]]

Bobby, thoughts on doing that?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Eric Faust [:efaust] from comment #1)
> OK, so the main barrier to completion here is that we are hack-and-slashing
> our way out of the AutoCompartment in the middle of the ErrorCopier
> destructor.
> 
> >        if (cx->getPendingException(&exc) && exc.isObject() && exc.toObject().is<ErrorObject>()) {
> >            cx->clearPendingException();
> >            ac.destroy();
> >            Rooted<ErrorObject*> errObj(cx, &exc.toObject().as<ErrorObject>());
> >            JSObject *copyobj = js_CopyErrorObject(cx, errObj, scope);
> >            if (copyobj)
> >                cx->setPendingException(ObjectValue(*copyobj));
> >        }
> 
> This is hard to remove, because of destructor semantics. Our options here
> seem to be:
> 
> 1) leave well enough alone, the maybe<> solution allows this (somewhat
> distasteful) behavior cleanly.

This is a fine default.

> 2) add a method to AutoCompartment. AutoCompartment::leave(), or something.
> Which leaves the origin compartment, and sets a flag for the descructor so
> we don't do it again. This isn't horrifying, but we need to agree that we're
> going to do it.

Definitely not. If you want to do something like that, use a Maybe<>. That's what it's for.

> 3) Something cleverer than I've got at the moment.

Why not just construct the ErrorCopier before the JSAutoCompartment, so that the destructor will run after ~
JSAutoCompartment, and therefore run in the proper scope? The JSAutoCompartment will have wrapped the exception, so then ErrorCopier can just copy it if necessary.

Actually though, even better would be JSAutoCompartmentWithErrorsCopied, which bundles all that behavior together and lets the caller avoid worrying about ordering.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

I cannot find the original pattern describe in comment 0, I'll assume that this has been fixed.