Closed Bug 812764 Opened 7 years ago Closed 6 years ago

Errors don't carry any meaningful information on them

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: vporof, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

The only thing available on instances of Error is __proto__, which, as expected, contains empty strings for the name and message properties. This makes the pause-on-exceptions unusable in some cases and watch expressions to display strings instead of error objects for now.

See https://bugzilla.mozilla.org/show_bug.cgi?id=727429#c13
Blocks: 812765
Assignee: nobody → past
Priority: -- → P3
Bumping the priority on this, since it's starting to get on my nerves.
Priority: P3 → P2
Duplicate of this bug: 911134
Assignee: past → ejpbruel
Attached patch error-properties.patch (obsolete) — Splinter Review
Sorry for stealing the bug, Eddy, but I was talking with a dev and he was really annoyed by this, so I looked into it, and I was able to fix it in the server layer really quick.
Assignee: ejpbruel → nfitzgerald
Status: NEW → ASSIGNED
Attachment #816783 - Flags: review?(past)
Sorry, didn't add the test file in the last patch.
Attachment #816783 - Attachment is obsolete: true
Attachment #816783 - Flags: review?(past)
Attachment #816784 - Flags: review?(past)
It might be good to note whether _forceMagicProperties has been run on this object and bail out after the first time doing it.
Blocks: 926722
Comment on attachment 816784 [details] [diff] [review]
error-properties.patch

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

Thank you Nick, this was really important!

I still think fixing the platform to avoid the need for this workaround would be preferable, so if Eddy agrees, I would suggest that we file a followup for the better fix. I don't think it's useful for the Debugger.Object reflection of an error object to not have these properties eagerly fetched. XPCOM's Components.Exception is already reflected properly as a Debugger.Object, so a JS Error should too.

(In reply to Brandon Benvie [:bbenvie] from comment #5)
> It might be good to note whether _forceMagicProperties has been run on this
> object and bail out after the first time doing it.

I agree with Brandon here. The message and stack properties can be quite large in some cases, and the extra memory overhead from copying them across the engine boundary could be a problem with mobile devices.

::: toolkit/devtools/server/actors/script.js
@@ +2697,5 @@
> +    const MAGIC_ERROR_PROPERTIES = [
> +      "message", "stack", "fileName", "lineNumber", "columnNumber"
> +    ];
> +
> +    if (this.obj.class.endsWith("Error")) {

This wouldn't catch custom errors with funny names, would it? I think a better approach is to check that this.obj.proto && this.obj.proto.class == "Error" or at least this.obj.proto.class.endsWith("Error").
Attachment #816784 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #6)
> Comment on attachment 816784 [details] [diff] [review]
> error-properties.patch
> 
> Review of attachment 816784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you Nick, this was really important!
> 
> I still think fixing the platform to avoid the need for this workaround
> would be preferable, so if Eddy agrees, I would suggest that we file a
> followup for the better fix. I don't think it's useful for the
> Debugger.Object reflection of an error object to not have these properties
> eagerly fetched. XPCOM's Components.Exception is already reflected properly
> as a Debugger.Object, so a JS Error should too.

Filed bug 927019

> 
> (In reply to Brandon Benvie [:bbenvie] from comment #5)
> > It might be good to note whether _forceMagicProperties has been run on this
> > object and bail out after the first time doing it.
> 
> I agree with Brandon here. The message and stack properties can be quite
> large in some cases, and the extra memory overhead from copying them across
> the engine boundary could be a problem with mobile devices.

Done.

> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +2697,5 @@
> > +    const MAGIC_ERROR_PROPERTIES = [
> > +      "message", "stack", "fileName", "lineNumber", "columnNumber"
> > +    ];
> > +
> > +    if (this.obj.class.endsWith("Error")) {
> 
> This wouldn't catch custom errors with funny names, would it? I think a
> better approach is to check that this.obj.proto && this.obj.proto.class ==
> "Error" or at least this.obj.proto.class.endsWith("Error").

Subclasses don't inherit the magic properties.
\o/
https://hg.mozilla.org/mozilla-central/rev/f615c239b9be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Duplicate of this bug: 929984
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.