Promise code should log original exception when it sanitizes exceptions

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox55 fixed)

Details

Attachments

(2 attachments)

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.
Flags: needinfo?(till)
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
Created attachment 8859313 [details] [diff] [review]
part 1.  Move the "report this error now" machinery from Debugger.cpp to ErrorReporting

This allows other consumers to share this machinery.
Attachment #8859313 - Flags: review?(jwalden+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created 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
Attachment #8859314 - Flags: review?(jwalden+bmo)
Flags: needinfo?(till)

Comment 4

10 months ago
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.)
Attachment #8859313 - Flags: review?(jwalden+bmo) → review+

Comment 5

10 months ago
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.
Attachment #8859314 - Flags: review?(jwalden+bmo) → review+

Comment 6

10 months ago
Pushed by bzbarsky@mozilla.com:
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

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

Comment 8

10 months ago
Backout by cbook@mozilla.com:
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

10 months ago
Pushed by bzbarsky@mozilla.com:
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
Flags: needinfo?(bzbarsky)

Comment 10

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f549be8f0bf4
https://hg.mozilla.org/mozilla-central/rev/9142443a4ca2
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.