Closed Bug 1306200 Opened 9 years ago Closed 8 years ago

Promise code should log original exception when it sanitizes exceptions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(2 files)

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.
This allows other consumers to share this machinery.
Attachment #8859313 - Flags: review?(jwalden+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(till)
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 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+
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
Flags: needinfo?(bzbarsky)
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
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)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: