1306200 - Promise code should log original exception when it sanitizes exceptions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

That's what DOM code does in similar situations: logs the chrome-side exception, then returns a sanitized thing to content.  SpiderMonkey promise code, however, just drops the chrome-side exception on the floor.

This makes it pretty hard to tell where the chrome-side exception was actually thrown.  In bug 1303714 it really shouldn't have been necessary to dive into a C++ debugger to figure that out.

We're probably going to need a runtime callback for this, or something, since SpiderMonkey can't do its own exception reporting.

Comment 1

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move the "report this error now" machinery from Debugger.cpp to ErrorReporting

This allows other consumers to share this machinery.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. When a promise is rejected with an object that cannot be securely unwrapped, report the underlying object as an error to its global before replacing the rejection value with a security error placeholder

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8859313 [details] [diff] [review]
part 1.  Move the "report this error now" machinery from Debugger.cpp to ErrorReporting

Review of attachment 8859313 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ErrorReporting.cpp
@@ +121,5 @@
>      if (!cx->helperThread())
>          err->throwError(cx);
>  }
> +
> +namespace {

Add blank lines around the class for breathing space.

Anonymous namespace use is fairly uncommon in SpiderMonkey -- don't be too surprised if someone removes it at some point on you.

@@ +126,5 @@
> +class MOZ_STACK_CLASS ReportExceptionClosure : public ScriptEnvironmentPreparer::Closure
> +{
> +public:
> +    explicit ReportExceptionClosure(HandleValue& exn)
> +        : exn_(exn)

Notwithstanding this was copied from within SpiderMonkey, indentation style should be

class MOZ_STACK_CLASS REC : public SEP::C
{
  public:
    explicit REC(HV& exn)
      : exn_(exn)
    {
    }

with two-space indents of access modifiers and the colon in the member-init-list.  (You could also put ": public SEP::C" on its own line, indented two spaces, which is possibly more readable for a very-long declaration line, but that's definitely debatable whether this one is or not.)

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8859314 [details] [diff] [review]
part 2.  When a promise is rejected with an object that cannot be securely unwrapped, report the underlying object as an error to its global before replacing the rejection value with a security error placeholder

Review of attachment 8859314 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.cpp
@@ +816,5 @@
> +            RootedObject realReason(cx, UncheckedUnwrap(&reason.toObject()));
> +            RootedValue realReasonVal(cx, ObjectValue(*realReason));
> +            RootedObject realGlobal(cx, &realReason->global());
> +            ReportErrorToGlobal(cx, realGlobal, realReasonVal);
> +            

Trailing WS.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14bbafbc6a6
part 1.  Move the "report this error now" machinery from Debugger.cpp to ErrorReporting.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae4abdb3d86
part 2.  When a promise is rejected with an object that cannot be securely unwrapped, report the underlying object as an error to its global before replacing the rejection value with a security error placeholder.  r=waldo

Comment 7

[Carsten Book [:Tomcat]]

backed out for linux debug (not x64) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=92586785&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/190af5fd4801
Backed out changeset fae4abdb3d86 
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ff36c046fa
Backed out changeset c14bbafbc6a6 for linux debug sm-arm bustage

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f549be8f0bf4
part 1.  Move the "report this error now" machinery from Debugger.cpp to ErrorReporting.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/9142443a4ca2
part 2.  When a promise is rejected with an object that cannot be securely unwrapped, report the underlying object as an error to its global before replacing the rejection value with a security error placeholder.  r=waldo

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f549be8f0bf4
https://hg.mozilla.org/mozilla-central/rev/9142443a4ca2