1259822 - Improve the error message produced when a user attempts to access a property of [something that evaluated to] undefined.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Morgan Phillips [:mrrrgn]]

Take 

js> f().fail()
typein:7:1 TypeError: f(...) is undefined
Stack:
  @typein:7:1

or 

js> var a = undefined
js> a.fail
typein:4:1 TypeError: a is undefined
Stack:
  @typein:4:1

These error messages could do better about explaining what actually went wrong in these situations. I suggest something along the lines of:

TypeError: f(...) is undefined, can't access property 'fail' of undefined

or maybe

TypeError: f(...) evaluates to undefined, undefined has no properties.

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Morgan Phillips [:mrrrgn] from comment #0)
> These error messages could do better about explaining what actually went
> wrong in these situations. I suggest something along the lines of:
> 
> TypeError: f(...) is undefined, can't access property 'fail' of undefined
> 
> or maybe
> 
> TypeError: f(...) evaluates to undefined, undefined has no properties.

My 2 cents, I prefer the first one because I really like seeing the property name that it was trying to access in the message

Comment 2

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> (In reply to Morgan Phillips [:mrrrgn] from comment #0)
> > These error messages could do better about explaining what actually went
> > wrong in these situations. I suggest something along the lines of:
> > 
> > TypeError: f(...) is undefined, can't access property 'fail' of undefined
> > 
> > or maybe
> > 
> > TypeError: f(...) evaluates to undefined, undefined has no properties.
> 
> My 2 cents, I prefer the first one because I really like seeing the property
> name that it was trying to access in the message

I'm down with that. +1

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

So, the message seems to be located here https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/js.msg#43 , which is then used in https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/JSContext.cpp#887-894 .
This ReportIsNotDefined appears to be called in https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/NativeObject.cpp#2288,2294 and https://hg.mozilla.org/mozilla-central/log/tip/js/src/vm/NativeObject.cpp

I don't know C++ well so I'm not sure what it would take to make the change we want.

Arai, I see you did review in this code lately. How hard would you think it is to change the error message to be like `{0} is undefined, can't access property {1}` ?

Comment 4

[Tooru Fujisawa [:arai]]

The message is JSMSG_UNEXPECTED_TYPE
https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/js.msg#80
which is used by ReportIsNullOrUndefined
https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/JSContext.cpp#904

which lacks the higher context what the resulting object is used for,
so we need to provide property id from callers like this:
https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/Interpreter.cpp#4575

but the ReportIsNullOrUndefined is used also in different contexts, here's the reverse call graph:
  ReportIsNullOrUndefined
    ToObjectSlow
      ToObject (many callers)
      CodeGenerator::visitValueToObject (JIT)
      ThrowObjectCoercible
        BaselineCompiler::emit_JSOP_CHECKOBJCOERCIBLE (JIT)
        CodeGenerator::visitCheckObjCoercible (JIT)
      ToObjectFromStack (many callers)

so, it would require adding a new function alternative to ToObject/ToObjectSlow which tries to convert the given value to an object, and returns the object or an error details without throwing the actual error,
and then also adding a new function which handles the error case and report an appropriate error message, based on the higher context including property id.

it might be complicated since the related case happens also in JIT code, which means we should pass the property id from JIT code.

I'll take a look.

Comment 5

[Tooru Fujisawa [:arai]]

so, for most property/element operations, we can get property id or element value, so we can report them.
but some expression (like x[y]++) is converted to using JSOP_CHECKOBJCOERCIBLE which throws the error, and it doesn't have property/element info,
so in that case we cannot report helpful error message.

anyway, I'll try to prepare patch for property access case.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

> anyway, I'll try to prepare patch for property access case.

that's already a very good improvement ! Thanks for looking into this arai

Comment 7

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd7d6b430d8eaff818fc0de594ea3a1304fdd2f

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Show property key in the error message when target object value is null or undefined.

* Renamed ReportIsNullOrUndefined to ReportIsNullOrUndefinedForPropertyAccess for clarity,
   and changed it to take `bool reportScanStack`, instead of `int spindex`,
   in order to handle `reportScanStack==false` case there too,
   given that there are more callers
 * Added ReportIsNullOrUndefinedForPropertyAccess variant which takes property key,
   and report it in the error message.
   the existing one is used when it's element access and the element key is
   not primitive (thus converting it to string has side-effect)
 * Added AutoByteId as a wrapper for JSAutoByteString+jsid
 * Added ToObjectFromStackForPropertyAccess which takes property key,
   and calls ReportIsNullOrUndefinedForPropertyAccess with the property key
   if possible
   * ToObjectFromStackForPropertyAccess and ToObjectSlowForPropertyAccess
     have 3 variants (HandleId, HandlePropertyName, HandleValue) in order to
     lazily convert the property/element key to jsid only when error happens
 * Changed ToObjectFromStack callers to use ToObjectFromStackForPropertyAccess
   if property key or element key is available

the change for tests are in the next patch, which will be folded with this patch after review.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8995729 [details] [diff] [review]
Show property key in the error message when target object value is null or undefined.

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

This looks great. Thanks!

I'm sorry for the slow review.

::: js/src/vm/JSContext.cpp
@@ +936,5 @@
>      }
>  }
>  
> +// A thin wrapper for JSAutoByteString for jsid.
> +class MOZ_RAII AutoByteId {

If this can be a function rather than a class, that seems just as good and less code. Like this (?)

    char*
    EncodeIdAsLatin1(JSContext* cx, HandleId id, JSAutoByteString& str)
    {
        ...
        return str.encodeLatin1(cx, idstr);
    }

@@ +992,5 @@
> +                                   bytes.get(), js_undefined_str, keyBytes.ptr());
> +    } else {
> +        MOZ_ASSERT(v.isNull());
> +        JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_PROPERTY_FAIL_EXPR,
> +                                   bytes.get(), js_null_str, keyBytes.ptr());

Style nit: I think the ternary expression in the original would be clearer here: `(v.isNull() ? js_null_str : js_undefined_str)`. With this patch, the reader has to narrow their eyes and visually diff the two branches.

The assertion by itself isn't a good enough reason to duplicate code; I do like assertions, but we already asserted v.isNullOrUndefined() at the top and `v` is a non-mutable handle.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: tests (will be folded).

Thank you for reviewing :)

Apparently I forgot to post the test patch.
here it is.

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c6e521429cfaff0585ec6eaf734e9fcf873f8a
Bug 1259822 - Show property key in the error message when target object value is null or undefined. r=jorendorff

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/f0c6e521429c

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

This is great arai, thanks a lot !
2 things I noticed: 
- it seems that we lost the [Learn more] link we had in the console next to TypeErrors
- could we have the same thing for ReferenceError (e.g. doing `x.prop` without prior `var x;`) ?

Comment 14

[Tooru Fujisawa [:arai]]

thanks :)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> - could we have the same thing for ReferenceError (e.g. doing `x.prop`
> without prior `var x;`) ?

given that the opcode for reading "x" doesn't have info about ".prop", it's a bit complicated.
if this happens frequently, it would worth trying, but I think it happens only once per each expression,
compared to this bug that happens whenever the value becomes null/undefined, depending on the execution.

Comment 15

[Julien Wajsberg [:julienw]]

This is really great, thanks !

Now I miss a last piece, that's when we try to use an undefined variable as a function. The current error is:

  XXX is not a function

It would be nice that the error message also says it's undefined or null. For example:

  XXX is undefined, can't call it as a function.

Comment 16

[Tooru Fujisawa [:arai]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> This is really great, thanks !
> 
> Now I miss a last piece, that's when we try to use an undefined variable as
> a function. The current error is:
> 
>   XXX is not a function
> 
> It would be nice that the error message also says it's undefined or null.
> For example:
> 
>   XXX is undefined, can't call it as a function.

sounds like related to bug 418573.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Maybe it is too specific for the general release notes. I will leave the release manager decided.

Release Note Request (optional, but appreciated)
[Why is this notable]: New helpful feature in the devtools
[Affects Firefox for Android]: No
[Suggested wording]: Improved error messages in the developer console when dealing with undefined variables
[Links (documentation, blog post, etc)]:

Comment 19

[Pascal Chevrel:pascalc]

Adding dev-doc-needed needed keyword as it fits https://developer.mozilla.org/fr/docs/Mozilla/Firefox/Releases/63 better than our end-users release notes.

Comment 21

[Chris Mills [:cmills]]

I see arai created a reference page for the error — thanks! I've added a note to the rel notes, see:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#JavaScript

Comment 22

[Jason Orendorff [:jorendorff]]

Attachment: Backed out changeset f0c6e521429c () for web compat issues (bug 1498257)

Comment 23

[Jason Orendorff [:jorendorff]]

That patch should apply cleanly to mozilla-beta after first backing out b58b7cd29a0b.

Comment 24

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

Backed out from mozilla-release (= Gecko 63) and mozilla-beta (= Gecko 64 in beta repo) as requested by Jason:

https://hg.mozilla.org/releases/mozilla-beta/rev/0e99081b5322d213fdba77a12ebbf6293f9c2a7f
https://hg.mozilla.org/releases/mozilla-beta/rev/db3a5881e0d22fa59c2107d49c0fefa675fe6bd3 (FIREFOX_63b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/ea4b5c999c575532506dfe6c702f59c5a268983b

Setting needinfo to set status for firefox65 to fixed once it gets added next week.

Comment 25

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

Backed out the backout (=relanded) due to bug913749.js failing:

Bug 1488417: https://hg.mozilla.org/releases/mozilla-beta/rev/0413c9c3d902bf4653569bd4c0e7e6ba1e239ba8
Bug 1259822: https://hg.mozilla.org/releases/mozilla-beta/rev/35c61888a49d69506cdd330b81885838ccf45f8c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205877473&repo=mozilla-beta
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=usercancel%2Crunnable%2Ctestfailed%2Cbusted%2Cexception%2Cretry&group_state=expanded&selectedJob=205877473&revision=f76d65097eb36e929926ebf9ebe86052bbd2f67e

Means it fails this change:
https://hg.mozilla.org/releases/mozilla-beta/rev/0e99081b5322d213fdba77a12ebbf6293f9c2a7f#l8.1

Comment 26

[Kohei Yoshino [:kohei]]

(updating the flag as per Bug 1498257)

Comment 27

[Julien Cristau [:jcristau] (back April 22)]

Backed out once again for 64:
https://hg.mozilla.org/releases/mozilla-release/rev/290d09651022f524b7bcc5a5514dc616f93fe89a

Comment 28

[Ryan VanderMeulen [:RyanVM]]

This was backed out from all releases for various web compat regressions. Reopening per IRC discussion with jorendorff.

Comment 29

[Tom S [:evilpie]]

Seems like we need to exactly match Chrome's error message "Cannot read property 'foobar' of undefined" to be able to reland this.

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Maybe we could keep Error.message as it currently is, and fill another, DevTools exposed property with a nicer message?
This will allow us to iterate on those message without breaking web compatibility.
I'm not sure if this could have an impact in some other place though.

Comment 31

[Tom S [:evilpie]]

Assuming that https://github.com/facebookincubator/idx is the only problem here, we might actually have a bit more flexibility in what it allows: https://github.com/facebookincubator/idx/blob/master/packages/idx/src/idx.js#L76-L81

Comment 32

[Tooru Fujisawa [:arai]]

fwiw, I've located yet another pattern of error message detection, which dynamically creates the pattern:
https://github.com/ignivamanishbhudhraja/React-Native-Boilerplate/blob/83bc40ca1e549ae4dd12ec23db1b5fddd2d84a56/src/utilities/Idx.js#L62-L112

Comment 33

[Tom S [:evilpie]]

(In reply to Tooru Fujisawa [:arai] from comment #32)
> fwiw, I've located yet another pattern of error message detection, which
> dynamically creates the pattern:
> https://github.com/ignivamanishbhudhraja/React-Native-Boilerplate/blob/
> 83bc40ca1e549ae4dd12ec23db1b5fddd2d84a56/src/utilities/Idx.js#L62-L112

Is this actually being used? This repository has almost no stars or activity.

Comment 34

[Tooru Fujisawa [:arai]]

(In reply to Tom Schuster [:evilpie] from comment #33)
> (In reply to Tooru Fujisawa [:arai] from comment #32)
> > fwiw, I've located yet another pattern of error message detection, which
> > dynamically creates the pattern:
> > https://github.com/ignivamanishbhudhraja/React-Native-Boilerplate/blob/
> > 83bc40ca1e549ae4dd12ec23db1b5fddd2d84a56/src/utilities/Idx.js#L62-L112
> 
> Is this actually being used? This repository has almost no stars or activity.

sorry, I forgot to mention that I wasn't sure it's the canonical repository for the file.

then, it seems to be the older version of idx, from 1 year ago:
https://github.com/facebookincubator/idx/blame/51349691549bb2202c66bc320025eb1e8c2513f8/packages/idx/src/idx.js

Comment 35

[:Harald Kirschner :digitarald]

Mike, Adam, not shipping this means regressing error messages for developers (like https://twitter.com/SamHulick/status/1204598663525421056/photo/1), so I wanted to re-open this discussion to at least iterate this forward.

We might get this unstuck by limiting exposure:
a) Maybe just ship this to DevEdition and let it bake there until the web catches up. This way developers can be exposed and hopefully can fix their error detection.
b) Only throw better error messages when DevTools is open

What are your thoughts?

Comment 36

[Mike Taylor [:miketaylr]]

a) Maybe just ship this to DevEdition and let it bake there until the web catches up. This way developers can be exposed and hopefully can fix their error detection.

I think this approach is good -- ship in DevEdition (maybe beta?) + Nightly and not break release.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Mike Taylor [:miketaylr] from comment #36)

I think this approach is good -- ship in DevEdition (maybe beta?) + Nightly and not break release.

If we do this, how could we make sure to not regress a release? If we would also exclude beta it would be more safe. See eg bug 1512401, which I filed only a couple days before a release.

Comment 38

[Mike Taylor [:miketaylr]]

effectively,

if NOT_RELEASE:
  do new thing
else:
  do old thing

It won't ride to release, until some future point where we have confidence we can ship it (if ever).

Comment 39

[:Harald Kirschner :digitarald]

I proposed Nightly and DevEdition (exclude Beta) to reach developers. This is the audience that we need need to make aware of the error-detection breakage in their projects. Beta should stay intact and close to behavior of release.

If we can land this behind a pref, we can also mitigate any impact we see and roll back off-train. We have other devtools prefs recently that were only enabled on Nightly/DevEdition https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#4911 : #if defined(MOZ_DEV_EDITION) || defined(NIGHTLY_BUILD).

Upside will hopefully be that developers have a place to discover, test and fix error parsing breakage. The trade-off would be that Nightly and DevEdition users will receive increased breakage for daily browsing and this will add overhead in web compat triage to sift out the breakage caused by the improved error messages.

Comment 40

[Tom S [:evilpie]]

Have we though about switching to error message from

win.browser.runtime is undefined, can't access property "getManifest" of it

to

can't access property "getManifest", win.browser.runtime is undefined

I think this might have a higher probability of keeping some of those regular expressions working.

Comment 41

[:Harald Kirschner :digitarald]

I think this might have a higher probability of keeping some of those regular expressions working.

How confident would we be that this breaks scripts less?

Alternatively, we could report the better error just to the Console and the old one to content?

Comment 42

[Tom S [:evilpie]]

(In reply to :Harald Kirschner :digitarald from comment #41)

I think this might have a higher probability of keeping some of those regular expressions working.

How confident would we be that this breaks scripts less?

I looked into some of the previous failures reported here, and it would not help much. For example the problem described in bug 1490772 Comment 5 would still occur.

Alternatively, we could report the better error just to the Console and the old one to content?

I don't know, we would have to add a lot of extra machinery to make this possible.

I think we should try doing this for the Dev Edition and Nightly like you proposed.

Comment 43

[:Harald Kirschner :digitarald]

:arai, would you be interested in re-landing this behind a pref that can be enabled for Nightly/DE?

Comment 44

[Tooru Fujisawa [:arai]]

will look into this weekend.

Comment 45

[Tooru Fujisawa [:arai]]

Attachment: Bug 1259822 - Part 1: Add pref to enable fix for accessing property of null or undefined. r?jorendorff

Comment 46

[Tooru Fujisawa [:arai]]

Attachment: Bug 1259822 - Part 2: Show property key in the error message when target object value is null or undefined. r=jorendorff

Depends on D58103

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03939cd2d0ab
Part 1: Add pref to enable fix for accessing property of null or undefined. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/e8d220dbe029
Part 2: Show property key in the error message when target object value is null or undefined. r=jorendorff

Comment 48

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/03939cd2d0ab
https://hg.mozilla.org/mozilla-central/rev/e8d220dbe029