1261904 - Refactor Debugger.Object.getErrorMessageName

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Morgan Phillips [:mrrrgn]]

This API doesn't make proper use of the Debugger object. In short, this method should not take an argument.

Comment 1

[Jason Orendorff [:jorendorff]]

Sorry I didn't do a better job reviewing that patch!

Comment 2

[Jim Blandy :jimb]

(In reply to Morgan Phillips [:mrrrgn] from comment #0)
> This API doesn't make proper use of the Debugger object. In short, this
> method should not take an argument.

It shouldn't take an argument, and it should possibly be just an accessor property of the Debugger.Object (see DebuggerObject_properties).

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: getErrorMsgName.diff

Comment 4

[Morgan Phillips [:mrrrgn]]

(In reply to Jim Blandy :jimb from comment #2)
> (In reply to Morgan Phillips [:mrrrgn] from comment #0)
> > This API doesn't make proper use of the Debugger object. In short, this
> > method should not take an argument.
> 
> It shouldn't take an argument, and it should possibly be just an accessor
> property of the Debugger.Object (see DebuggerObject_properties).

Yeah, 100% agree about it being a property.

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: getErrorMsgName.diff

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: getErrorMsgName.diff

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8738319 [details] [diff] [review]
getErrorMsgName.diff

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

Tests are excellent. One minor issue to address.

::: js/src/vm/Debugger.cpp
@@ +8039,5 @@
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get errorMessageName", args, referent);
> +
> +    JSObject* obj = referent;
> +    if (IsCrossCompartmentWrapper(obj))
> +        obj = CheckedUnwrap(obj);

It looks to me like CheckedUnwrap can return nullptr if the unwrap isn't permitted. If this happens, the is<ErrorObject> check will segfault. We need to report an error on cx and return false; see how Debugger::unwrapDebuggeeArgument checks the return value from its call to CheckedUnwrap for an example to emulate.

Comment 8

[Morgan Phillips [:mrrrgn]]

(In reply to Jim Blandy :jimb from comment #7)
> Comment on attachment 8738319 [details] [diff] [review]
> getErrorMsgName.diff
> 
> Review of attachment 8738319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tests are excellent. One minor issue to address.
> 
> ::: js/src/vm/Debugger.cpp
> @@ +8039,5 @@
> > +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get errorMessageName", args, referent);
> > +
> > +    JSObject* obj = referent;
> > +    if (IsCrossCompartmentWrapper(obj))
> > +        obj = CheckedUnwrap(obj);
> 
> It looks to me like CheckedUnwrap can return nullptr if the unwrap isn't
> permitted. If this happens, the is<ErrorObject> check will segfault. We need
> to report an error on cx and return false; see how
> Debugger::unwrapDebuggeeArgument checks the return value from its call to
> CheckedUnwrap for an example to emulate.

Oy vey, fixing it up. Thanks !

https://49.media.tumblr.com/38430f1e34abf51dc21bd62bdc7f2063/tumblr_myy2yekXjj1rrx588o1_500.gif

Comment 9

[Morgan Phillips [:mrrrgn]]

Attachment: getErrorMsgName.diff

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: getErrorMsgName.diff

Comment 11

[Jim Blandy :jimb]

Comment on attachment 8738391 [details] [diff] [review]
getErrorMsgName.diff

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

Looks great, thanks!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b76e05f7ecf

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b76e05f7ecf