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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(2 files)
4.97 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•8 years ago
|
||
Mass wontfix for bugs affecting firefox 52.
![]() |
Assignee | |
Comment 2•8 years ago
|
||
This allows other consumers to share this machinery.
Attachment #8859313 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 3•8 years ago
|
||
Attachment #8859314 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Updated•8 years ago
|
Flags: needinfo?(till)
Comment 4•8 years 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•8 years 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+
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•8 years 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)
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
![]() |
Assignee | |
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f549be8f0bf4
https://hg.mozilla.org/mozilla-central/rev/9142443a4ca2
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.
Description
•