1383630 - ScriptedProxyHandler should report which first property a TypeError is thrown for

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[Alex Vincent [:WeirdAl]]

The latest example I've hit: [1]

I've been trying to craft a Membrane library a la Tom van Cutsem's Membrane concept, named es7-membrane. [2], [3]  The biggest pain point I have in developing this is that the JavaScript engines are very enthusiastic about telling me I've made an error... but utterly useless in telling me the name of the property (string or symbol) that triggered the error in the first place.

Could someone please append the stringification of the failing property key to the error message, so that I know where to look?

I can write up a testcase to trigger one of these failures pretty easily, if necessary.

[1] https://dxr.mozilla.org/mozilla-esr52/source/js/src/proxy/ScriptedProxyHandler.cpp#792
[2] https://tvcutsem.github.io/js-membranes
[3] https://github.com/ajvincent/es7-membrane/

Comment 1

[Jan de Mooij [:jandem]]

Yeah testcases would be great.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: bug1383630-test0.js

A simple testcase for the error I was complaining about.

I only remembered this one because I hit it a couple hours before filing... other testcases will take a little time for me to remember, but in theory they should be easy to create based on ScriptedProxyHandler.cpp.

Comment 3

[Alex Vincent [:WeirdAl]]

https://dxr.mozilla.org/mozilla-esr52/source/js/src/tests/ecma_6/Reflect/ownKeys.js#58 is another testcase.

Comment 4

[Alex Vincent [:WeirdAl]]

IsCompatiblePropertyDescriptor is particularly egregious:  good luck deciphering which of the many conditions in there causes *bp to become false.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: same testcase using a symbol-named property

I tried poking around to see if I could solve this myself, but every solution I come up with fails at converting targetNonconfigurableKeys[i] to a string when the key is a symbol... usually by crashing the tab that owns the scratchpad.

tcampbell suggested ValueToPrimitive as an approach, but that doesn't work either, masking the original error with "TypeError: can't convert symbol to string"

Comment 6

[André Bargull [:anba]]

Maybe copying this approach works here, too: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/jsapi.cpp#172-179

Comment 7

[Alex Vincent [:WeirdAl]]

Thank you, Andre... that actually works very nicely.

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: Modify several proxy errors to end with the property name. (work in progress)

Notes for feedback requested:

* I am not a JSAPI developer, and an infrequent C++ developer.  Please take extra caution on this.

* The error messages are slightly inconsistent after my changes:  the ones I modified have the property name at the end, without enclosing single quotes.  Other errors sometimes use single quotes, sometimes not, and rarely at the end.

* I based this on a prior example of JS_ReportErrorNumberLatin1 in GetProxyTrap(), but I'm wondering about Unicode in general:  should I have used a UTF-8 encoding instead?

* I want to break up IsCompatiblePropertyDescriptor's steps 3, 4 and 9 to determine a "details" string from the conditions tested for.  These details will be passed into ReportPropertyErrorWithDetails, which is currently ifdef'd out.  I would welcome advice.

* Where should I write constant strings for IsCompatiblePropertyDescriptor to pass as the details argument?  How should they be formatted?  Please provide a decent example that will pass review.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8890191 [details] [diff] [review]
Modify several proxy errors to end with the property name.  (work in progress)

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

Thanks for the patch! I'll do a more thorough review tomorrow, but a question below.

::: js/src/proxy/ScriptedProxyHandler.cpp
@@ +708,5 @@
>          if (!GetElement(cx, obj, obj, index, &next))
>              return false;
>  
>          // Step 6c.
> +        if (!ValueToId<CanGC>(cx, next, &id))

Is it okay to move this call *before* the string/symbol check below? It seems like something that would be observable when we have an object because we call obj.toString()

Comment 10

[Alex Vincent [:WeirdAl]]

(In reply to Jan de Mooij [:jandem] from comment #9)

> Is it okay to move this call *before* the string/symbol check below? It
> seems like something that would be observable when we have an object because
> we call obj.toString()

I did that as a pre-emptive measure to make sure id was available for ReportPropertyError.  As for whether it's safe, ok or not:  I have no idea.

Comment 11

[Alex Vincent [:WeirdAl]]

Another note I forgot to ask for in the feedback request:

* I've seen references to result.fail with an error number... how does that work, and should I be trying to use that instead?

Comment 12

[Alex Vincent [:WeirdAl]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Is it okay to move this call *before* the string/symbol check below? It
> seems like something that would be observable when we have an object because
> we call obj.toString()

Ahh, I see why you asked  GetElement sets next, while ValueToId relies on next to get id.  Chicken-and-egg problem there... oops.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8890191 [details] [diff] [review]
Modify several proxy errors to end with the property name.  (work in progress)

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

Looks great! Some suggestions below.

::: js/src/js.msg
@@ +401,5 @@
>  MSG_DEF(JSMSG_PROXY_ISEXTENSIBLE_RETURNED_FALSE,0,JSEXN_TYPEERR,"proxy isExtensible handler must return the same extensibility as target")
>  MSG_DEF(JSMSG_INCONSISTENT_SETPROTOTYPEOF_TRAP,0,JSEXN_TYPEERR,"proxy setPrototypeOf handler returned true, even though the target's prototype is immutable because the target is non-extensible")
>  MSG_DEF(JSMSG_CANT_CHANGE_EXTENSIBILITY, 0, JSEXN_TYPEERR, "can't change object's extensibility")
>  MSG_DEF(JSMSG_CANT_DEFINE_INVALID,     0, JSEXN_TYPEERR, "proxy can't define an incompatible property descriptor")
> +MSG_DEF(JSMSG_CANT_DEFINE_NEW,         1, JSEXN_TYPEERR, "proxy can't define a new property on a non-extensible object for property {0}")

What do you think about the following, shorter message:

"proxy can't define new property '{0}' on a non-extensible object"

Same below.

::: js/src/proxy/ScriptedProxyHandler.cpp
@@ +23,5 @@
> + */
> +static void
> +ReportPropertyError(JSContext* cx,
> +                    const jsid id,
> +                    const unsigned errorNumber)

Nit: this fits on one line (99 columns for code, 80 for comments).

@@ +31,5 @@
> +    RootedString str(cx, ValueToSource(cx, idv));
> +    if (str)
> +        propName.encodeLatin1(cx, str);
> +    JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
> +                               errorNumber, propName.ptr());

Hm isn't this ReportPropertyError function very similar to js::Throw here?

http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/js/src/jsobj.cpp#254

It would be great if we could just call that one :)

@@ +38,5 @@
> +static void
> +ReportPropertyErrorWithDetails(JSContext* cx,
> +                               const jsid id,
> +                               const unsigned errorNumber,
> +                               const char* detailsLiteral)

This function is not used?

@@ +708,5 @@
>          if (!GetElement(cx, obj, obj, index, &next))
>              return false;
>  
>          // Step 6c.
> +        if (!ValueToId<CanGC>(cx, next, &id))

To expand on my previous comment, we get here for each property returned by an ownPropertyKeys trap. Now the problem is this: what if ownPropertyKeys returns the following:

  var evilObject = {toString() { throw "FAIL"; }};
  return [evilObject];

If I understand correctly:

- Before your patch we would throw JSMSG_OWNKEYS_STR_SYM.
- With your patch, ValueToId will call evilObject.toString() and we throw "FAIL".

Maybe just undo the changes to this function? If you really care about the error message here, maybe you can call ValueToId inside the if-statement below only if it's a primitive.

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: bug1383630.diff

Further notes for review:

* If it is desirable to break up the regress-1383630.js file into smaller chunks, I have no objection to that:  when a test fails in running jstests.py, I wasn't able to get a stack trace.

* I was unable to start a run of all the jsapi-tests.  I suspect https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Running_Automated_JavaScript_Tests has bitrotted a little.

* I believe the way I wrote details strings as a series of static const char* declarations isn't in the Mozilla style, but I couldn't remember what that style is.  Adding them to js.msg seemed inappropriate (especially since touching that file causes files across the tree to rebuild).

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8891671 [details] [diff] [review]
bug1383630.diff

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

This looks great, just some minor suggestions below.

::: js/src/js.msg
@@ +414,5 @@
> +MSG_DEF(JSMSG_CANT_REPORT_INVALID,     2, JSEXN_TYPEERR, "proxy can't report an incompatible property descriptor ('{0}', {1})")
> +MSG_DEF(JSMSG_CANT_REPORT_NC_AS_NE,    1, JSEXN_TYPEERR, "proxy can't report a non-configurable own property '{0}' as non-existent")
> +MSG_DEF(JSMSG_CANT_REPORT_NEW,         1, JSEXN_TYPEERR, "proxy can't report a new property '{0}' on a non-extensible object")
> +MSG_DEF(JSMSG_CANT_REPORT_NE_AS_NC,    1, JSEXN_TYPEERR, "proxy can't report a non-existent property '{0}' as non-configurable")
> +MSG_DEF(JSMSG_CANT_SET_NW_NC,          1, JSEXN_TYPEERR, "proxy can't successfully set a non-writable, non-configurable property '{0}' ")

Nit: remove the space between '{0}' and "

::: js/src/proxy/ScriptedProxyHandler.cpp
@@ +18,5 @@
>  using JS::IsArrayAnswer;
>  using mozilla::ArrayLength;
>  
> +// trying to borrow from js.msg as much as possible...
> +static const char* DETAILS_NOT_EXTENSIBLE

Since these are used only once (except for an assertion below, but that doesn't seem too important), I'd just inline them directly below (*details = "Foo"). That also makes the code easier to read because we don't have to scroll up.

@@ +23,5 @@
> +    ("proxy can't report an extensible object as non-extensible");
> +static const char* DETAILS_CANT_REPORT_NC_AS_C
> +    ("proxy can't report an existing non-configurable property as configurable");
> +static const char* DETAILS_ENUM_DIFFERENT
> +    ("proxy can't report a different enumerable from target when target is not configurable");

Nit: this may sound a bit confusing. How about ..a different 'enumerable' from..?

@@ +38,5 @@
> +static const char* DETAILS_CANT_REPORT_C_AS_NC
> +    ("proxy can't define an existing configurable property as non-configurable");
> +
> +static void
> +ReportPropertyErrorWithDetails(JSContext* cx,

We should consider adding a |const char* details = nullptr| argument to js::Throw, so we don't have to duplicate the code below. Also the names should be consistent at least :)

@@ +44,5 @@
> +                               const unsigned errorNumber,
> +                               const char* detailsLiteral)
> +{
> +    JSAutoByteString propName,
> +                     details(cx, JS_NewStringCopyZ(cx, detailsLiteral));

JS_NewStringCopyZ is fallible (we have to check for OOM). I think we don't need to create a JS string though, it should be possible to just pass detailsLiteral directly to the JS_Report call below.

@@ +68,3 @@
>  {
> +    // precondition:  we won't set details if checks pass, so it must be null here.
> +    MOZ_ASSERT((*details) == nullptr);

Nit: can drop the parents around *details. I'd also rename to errorDetails.

@@ +104,1 @@
>              return true;

Nit: remove the {} here

@@ +517,5 @@
>      if (!Call(cx, trap, handler, targetVal, propKey, &trapResult))
>          return false;
>  
>      // Step 9.
>      if (!trapResult.isUndefined() && !trapResult.isObject()) {

Style nit: since the if-body is now a single line, we should drop the {}. Also a number of times below.

::: js/src/tests/js1_8_5/regress/regress-1383630.js
@@ +5,5 @@
> +
> +/* These tests are not checking whether an exception is thrown or not for
> + * proxies:  those tests should already exist in js/src/tests/ecma_6/Proxy .
> + * We expect TypeErrors to be thrown in these tests, with a stringification
> + * at the end of the error message showing whatever property name the error is

Nit: the "at the end" part is no longer true right?

Comment 16

[Alex Vincent [:WeirdAl]]

Attachment: bug1383630.diff with review nits addressed

I moved the whole string declaration, including the static keyword, inside the relevant blocks as directed.  (I worried that by leaving out the static keyword, the errorDetails pointer would be set to a local variable that would be immediately collected.)

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8892322 [details] [diff] [review]
bug1383630.diff with review nits addressed

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

Sorry for the delay, still looks good. if you can post an updated patch with the nits below addressed I'll send it to try and land it ASAP.

Also please set yourself as the patch author and add a description like:

  Bug 1383630 - Change ScriptedProxyHandler ..etc.. r=jandem

If you don't know how, I'd be happy to do it for you.

::: js/src/jsobj.cpp
@@ +261,5 @@
>      JSAutoByteString bytes(cx, idstr);
>      if (!bytes)
>          return false;
> +
> +    if (details)

Nit: add {} to both the then and else branches, since the then-branch spans multiple lines.

::: js/src/proxy/ScriptedProxyHandler.cpp
@@ +36,5 @@
>      if (!current.object()) {
>          // Step 2a-b,e.  As |O| is always undefined, steps 2c-d fall away.
> +        if (!extensible) {
> +            static const char* DETAILS_NOT_EXTENSIBLE
> +                ("proxy can't report an extensible object as non-extensible");

Nit: the syntax is a bit unusual for assigning to a const char*, I'd change this to:

static const char* DETAILS_NOT_EXTENSIBLE =
    "proxy can't report an extensible object as non-extensible";

Also below.

@@ +50,1 @@
>          return true;

Nit: keep the {} in this case: because the if-condition spans multiple lines, without the {} it's harder to see where the condition ends and the body starts.

@@ +1023,5 @@
> +        if (desc.isAccessorDescriptor() &&
> +            !desc.configurable() &&
> +            (desc.getterObject() == nullptr) &&
> +            !trapResult.isUndefined())
> +                return js::Throw(cx, id, JSMSG_MUST_REPORT_UNDEFINED);

Nit: add {} here too, and you can unindent (dedent?) the return statement now.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: final patch with review nits addressed

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8893550 [details] [diff] [review]
final patch with review nits addressed

For future reference, the checkin-needed bug keyword works much more nicely with our bug marking tools.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d302bb7b14
ScriptedProxyHandler should report which first property a TypeError is thrown for. r=jandem

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/34d302bb7b14