Closed Bug 1259822 Opened 9 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla74
Tracking Flags:
Tracking Status
relnote-firefox --- -
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox74 --- fixed

People

(Reporter: mrrrgn, Assigned: arai, NeedInfo)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete, feature, site-compat)

Attachments

(2 files, 3 obsolete files)

Bug 1259822 - Part 1: Add pref to enable fix for accessing property of null or undefined. r?jorendorff
(deleted), text/x-phabricator-request
Details
Bug 1259822 - Part 2: Show property key in the error message when target object value is null or undefined. r=jorendorff
(deleted), text/x-phabricator-request
Details
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.
Blocks: 1257918
(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
(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
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}` ?
Flags: needinfo?(arai.unmht)
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.
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.
Flags: needinfo?(arai.unmht)
> anyway, I'll try to prepare patch for property access case. that's already a very good improvement ! Thanks for looking into this arai
* 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.
Attachment #8995729 - Flags: review?(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.
Attachment #8995729 - Flags: review?(jorendorff) → review+
Attached patch tests (will be folded). (obsolete) (deleted) — — Splinter Review
Thank you for reviewing :) Apparently I forgot to post the test patch. here it is.
Attachment #9001793 - Flags: review?(jorendorff)
Attachment #9001793 - Flags: review?(jorendorff) → review+
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
https://hg.mozilla.org/mozilla-central/rev/f0c6e521429c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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;`) ?
Depends on: 1486140
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.
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.
(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.
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)]:
relnote-firefox: --- → ?
Keywords: feature
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.
relnote-firefox: ? → -
Keywords: dev-doc-needed
Depends on: 1490772
Depends on: 1498257
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
Keywords: dev-doc-needed → dev-doc-complete
Assignee: arai.unmht → jorendorff
That patch should apply cleanly to mozilla-beta after first backing out b58b7cd29a0b.
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.
status-firefox63: fixed → wontfix
status-firefox64: --- → wontfix
Flags: needinfo?(aryx.bugmail)
(updating the flag as per Bug 1498257)
status-firefox64: fixed → disabled
This was backed out from all releases for various web compat regressions. Reopening per IRC discussion with jorendorff.
Status: RESOLVED → REOPENED
status-firefox65: fixed → wontfix
status-firefox66: --- → affected
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
Seems like we need to exactly match Chrome's error message "Cannot read property 'foobar' of undefined" to be able to reland this.
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.
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
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
(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.
(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
Keywords: site-compat

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?

Flags: needinfo?(miket)
Flags: needinfo?(astevenson)

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.

Flags: needinfo?(miket)

(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.

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).

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.

Blocks: jserror

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.

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?

(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.

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

Flags: needinfo?(arai.unmht)

will look into this weekend.

Assignee: jorendorff → arai.unmht
Status: REOPENED → ASSIGNED
Flags: needinfo?(arai.unmht)
Attachment #8995729 - Attachment is obsolete: true
Attachment #9001793 - Attachment is obsolete: true
Attachment #9017614 - Attachment is obsolete: true
Pushed by arai_a@mac.com: 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

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 5 years ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: