Closed Bug 1283712 Opened 8 years ago Closed 8 years ago

Add a mechanism to add notes for an error message.

Categories

(Core :: JavaScript Engine, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(13 files, 16 obsolete files)

Part 4: Print error note in js::PrintError.
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
Part 1: Add JSErrorBase, JSErrorNotes, JSErrorNotes::Note, and JSErrorReport.{notes,freeNotes}. r=jwalden
(deleted), patch
arai
: review+
Details | Diff | Splinter Review
Part 2: Add JSEXN_NOTE. r=jwalden
(deleted), patch
arai
: review+
Details | Diff | Splinter Review
Part 3: Add Parser::errorWithNotes and Parser::errorWithNotesAt. r=jwalden
(deleted), patch
arai
: review+
Details | Diff | Splinter Review
Part 5: Support notes in Debugger.
(deleted), patch
jimb
: review+
Details | Diff | Splinter Review
Part 6: Add xpc::ErrorBase, xpc::ErrorNote, and xpc::ErrorReport.mNotes.
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review
Part 7: Add nsIScriptErrorNote and nsIScriptError.notes.
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review
Part 8: Add WorkerErrorBase, WorkerErrorNote, and WorkerErrorReport.
(deleted), patch
bholley
: review+
Details | Diff | Splinter Review
Part 9: Add getErrorNotes testing function to extract error notes from exception.
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
Part 10: Support notes in getLastWarning shell-only testing function.
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
Part 11.1: Show notes in devtools console.
(deleted), patch
nchevobbe
: review+
Details | Diff | Splinter Review
Part 11.2: Update stub.
(deleted), patch
nchevobbe
: review+
Details | Diff | Splinter Review
Part 11.3: Add mocha test.
(deleted), patch
nchevobbe
: review+
Details | Diff | Splinter Review
in bug 420857 and bug 104442, it would be better reporting extra position in the file with structural data, like {message, filename, line number, column number}, and format them while printing to terminal or web console. what I expect is something like following: js> let a = 1; let a = 2; typein:1:15 SyntaxError: redeclaration of let a: typein:1:15 let a = 1; let a = 2; typein:1:15 ...............^ typein:1:4 note: a is previously declared here: typein:1:4 let a = 1; let a = 2; typein:1:4 ....^
Depends on: 1283710
things that needs to be defined: 1. API design how to associate a note to an error * create note and pass it to error reporting API ? * report error and add note to the pending exception ? * should note be supported for warning? (warning is not converted to exception) 2. data structure where should we store note information after the error is converted to exception?
about API design, maybe we could add separated API for those 2 parts, (both are done in current error reporting API): * creating JSErrorReport from given formatter/error number and parameters * report JSErrorReport so, we can add another API to add note to JSErrorReport and call it between them.
another thing: 3. should we support multiple notes? (current supposed use case needs only 1 note)
Assignee: nobody → arai.unmht
As both supposed use cases are in frontend, bug 1302282 will be related, especially for API design.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1302282
about data structure, we hold JSErrorReport even after it's converted to exception, so we could just hold note as a linked list from JSErrorReport. with linked list, multiple notes can be supported easily. about internal API, I'm prototyping something like following, for frontend: SwappableCompileError error; if (!initError(&error, ParseError, false, null(), JSMSG_CURLY_AFTER_BODY)) return false; if (!addErrorNoteWithOffset(&error, openedPos, JSMSG_CURLY_OPENED)) return false; report(&error); (SwappableCompileError is for swapping the reference when it's off-mainthread) public API might also be similar style: JSErrorReport report; if (!JS_InitErrorNumberASCII(cx, &report, nullptr, nullptr, JSEXN_REFERENCEERR, "globl_")) return false; if (!JS_AddErrorNoteNumberASCII(cx, &report, nullptr, nullptr, JSEXN_REFERENCE_SUGGEST, "global")) return false; if (!JS_AddErrorNoteNumberASCII(cx, &report, nullptr, nullptr, JSEXN_REFERENCE_SUGGEST, "global_")) return false; JS_ReportError(cx, &report); honestly, I'm not sure if note is really useful for current public API, without having ability to set filename/lineno/column for each, since everything can be embedded into single error message. The only reason would be, we can add multiple notes without having different formatters for each. If there's any place that wants to report note for different place, outside of frontend, it would be nice to add API to set filename/lineno/column for each error and note.
outside JSAPI, we convert JSErrorReport to nsScriptError. So we should support note in nsScriptError too, to display note in web console and browser console.
between JSErrorReport and nsScriptError, there is xpc::ErrorReport. it also needs the support for note.
fitzgen, can I have your opinion on how to show note in web console and browser console? what I want to do with note is: * show the following things as note for error message * message * filename * line number * column number * link to the file, like error (open source or debugger) * associate note with error so, something like this: SyntaxError: redeclaration of let a a.js:1:15 note: a is previously declared here a.js:1:4 Is there already similar thing supported there? or maybe I should just log 2 message for error and note at the same time?
Flags: needinfo?(nfitzgerald)
by "associate", I mean the note is always shown as long as the error is shown. so, it may affect filtering.
First off -- this sounds really great! I dream that one day we'll have error messages as nice as Elm / Rust :) I don't think we have any existing support for tacking on extra notes -- the closest thing I know of is the error ID that we use to make links to MDN pages. I suspect that the path of least resistance will be logging twice. One potential gotcha is that the second log should probably be the same level (warn, error, etc) as the first, so that filtering by log level doesn't remove one but not the other. Lin and Brian might have opinions as well.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(lclark)
Flags: needinfo?(bgrinstead)
> I suspect that the path of least resistance will be logging twice. One potential gotcha is that the second log should probably be the same level (warn, error, etc) as the first, so that filtering by log level doesn't remove one but not the other. With this approach, you could potentially have one filtered when the other is not with the text filter. If the extra note could simply be appended to the exception text in the message this would be easy enough. It gets harder if the note needs to have a location link (a.js:1:4 in the example) that opens in the debugger. That would require changes in the structure of the messages.
Flags: needinfo?(lclark)
thanks. for current supposed use case, it's more important to link to the location, than associating note to error. but associating them would also help developers. will check error ID and "Learn More" link implementation.
Depends on: 1313921
error handling in Worker is troublesome. it uses ErrorEvent to report, and it's hard to add note to it unless we have ErrorObject... I'll go with mainthread-only support first.
(In reply to Tooru Fujisawa [:arai] from comment #13) > error handling in Worker is troublesome. > it uses ErrorEvent to report, and it's hard to add note to it unless we have > ErrorObject... this was incorrect. I just need to pass all information from JSErrorReport in WorkerPrivate::ReportError to nsIScriptError in LogErrorToConsole. thanks to bz :)
okay, now worker is also supported. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce053a18ea27 it's using different structure for error and note, in all phase (JSAPI, XPConnect, XPCOM, worker). in most phase (JSAPI, XPConnect, worker), error and note has same base class, that contains common properties and some helper methods. XPCOM case uses totally different structure and interface, since note is not console message. devtools part supports new console only. it shows notes in single error message box.
another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e03cc5f5d20e currently filtering on Web Console doesn't see notes. now fixing it.
prepared several patches, here's the whole patch series: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7a26d073044 will ask review incrementally. [Part 1] Separated the the following common properties and related methods between note and error to JSErrorBase: * message (message_, ownsMessage_) * filename (filename) * line buffer (linebuf_, linebufLength_, tokenOffset_, ownsLinebuf_) * line number (lineno) * column number (column) * error number (errorNumber) Now JSErrorReport is a sub class of JSErrorBase, and JSErrorNote is also a subclass of JSErrorBase. Notes are holded as linked list, from JSErrorReport.firstNote, and linked to next note with JSErrorNote.nextNote. All notes are freed in JSErrorReport destructor. I was about to use a vector to hold notes, but linked list was simpler for js::CopyErrorReport implementation. js::CopyErrorReport mallocs each error/note as a single chunk. JS_CHARS_SIZE macro was unused, so removed.
Attachment #8807260 - Flags: review?(jwalden+bmo)
Attached patch Part 2: Add JSEXN_NOTE. (obsolete) (deleted) — — Splinter Review
JSEXN_NOTE is used just for js.msg definition. JSErrorNote doesn't contain exnType property since it's always same.
Attachment #8807261 - Flags: review?(jwalden+bmo)
Introduced the following style to report error with notes. SwappableCompileError error; if (!initErrorWithOffset(&error, ParseError, false, errorPos, JSMSG_SOME_ERROR, ...)) return false; if (!addErrorNoteWithOffset(&error, notePos, JSMSG_SOME_NOTE, ...)) return false; if (!addErrorNoteWithOffset(&error, notePos, JSMSG_ANOTHER_NOTE, ...)) return false; // ... report(&error); So, instead of just calling report function, it constructs the error and notes on the stack value, and pass it to report function. SwappableCompileError was added to handle errors in off-main-thread parsing, that is passed to ExclusiveContext::addPendingCompileError. Without this patch, we're using stack allocated 2 variables: https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/js/src/frontend/TokenStream.cpp#602-606 > CompileError tempErr; > CompileError* tempErrPtr = &tempErr; > if (!cx->isJSContext() && !cx->addPendingCompileError(&tempErrPtr)) > return false; SwappableCompileError abstracts them, and now they can be passed to functions, and also allocated on parser side. Also, added ExpandErrorArgumentsVA for JSErrorNote, to create error note message from formatter.
Attachment #8807262 - Flags: review?(jwalden+bmo)
Attached patch Part 4: Print error note in js::PrintError. (obsolete) (deleted) — — Splinter Review
js::PrintError now prints notes, after printing error. each note has the same format as error. js> let a, a; typein:1:7 SyntaxError: redeclaration of let a: typein:1:7 let a, a; typein:1:7 .......^ typein:1:4 note: a is previously declared at line 1: typein:1:4 note: let a, a; typein:1:4 note: ....^
Attachment #8807263 - Flags: review?(jwalden+bmo)
currently devtool console doesn't show location information for console input. it makes hard to say "here" or something in the note (that's why I choose "... at line 1" in note) bug 1315242 will fix the situation. of course it might be better avoiding "here" tho.
Are there any outstanding questions about integrating with the console?
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #22) > Are there any outstanding questions about integrating with the console? thanks, I think now I can fix this, some parts rebased on bug 1315242 patch.
Comment on attachment 8807262 [details] [diff] [review] Part 3: Make it possible to report error with note in frontend. clearing request for now. will update it to follow bug 1296814's way.
Attachment #8807262 - Flags: review?(jwalden+bmo)
Comment on attachment 8807260 [details] [diff] [review] Part 1: Add JSErrorBase, JSErrorNote, and JSErrorReport.{firstNote,addNote,freeNotes}. Review of attachment 8807260 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good, but the memory leak thing could be finicky I want to see the next version of this. ::: js/src/jsexn.cpp @@ +145,5 @@ > IMPLEMENT_ERROR_CLASS(RuntimeError, &ErrorObject::nonGlobalErrorClassSpec_) > }; > > +template <typename T> > +T* Tack a static on this. @@ +235,5 @@ > + > + while (note->nextNote) { > + JSErrorNote* nextNoteCopy = CopyErrorBase(cx, note->nextNote); > + if (!nextNoteCopy) > + return nullptr; This doesn't really correctly handle OOM, does it? If either note-copying function returns null, we leak the error report copy and all notes thus allocated. Seems like you should have some JS::UniquePtr for these things so that on failure, they'll be properly cleaned up. @@ +245,5 @@ > + } > + > + noteCopy->nextNote = nullptr; > + } else { > + copy->firstNote = nullptr; This is a little finicky, but you can just assert it's null here, because the JSErrorReport was calloc'd.
Attachment #8807260 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8807261 [details] [diff] [review] Part 2: Add JSEXN_NOTE. Review of attachment 8807261 [details] [diff] [review]: ----------------------------------------------------------------- rs=me, tho I suppose reading later patches might solidify this understanding.
Attachment #8807261 - Flags: review?(jwalden+bmo) → review+
updated to follow new naming/parameter convention
Attachment #8807262 - Attachment is obsolete: true
Attachment #8812376 - Flags: review?(jwalden+bmo)
Comment on attachment 8812376 [details] [diff] [review] Part 3: Make it possible to report error with note in frontend. discussed on IRC. I'll change the data structure for notes, and also change the frontend APIs.
Attachment #8812376 - Flags: review?(jwalden+bmo)
Attachment #8807263 - Flags: review?(jwalden+bmo)
Comment on attachment 8812491 [details] [diff] [review] Part 1: Add JSErrorBase, JSErrorNotes, JSErrorNotes::Note, and JSErrorReport.{notes,freeNotes}. Review of attachment 8812491 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsexn.cpp @@ +160,5 @@ > + return 0; > +} > + > +bool > +CopyExtraData(JSContext* cx, JSErrorReport* copy, uint8_t* cursor, JSErrorReport* report) forgot to add "&" to cursor. it should be "uint8_t*& cursor", to update cursor on caller.
Comment on attachment 8812491 [details] [diff] [review] Part 1: Add JSErrorBase, JSErrorNotes, JSErrorNotes::Note, and JSErrorReport.{notes,freeNotes}. apparently I broke several more things. clearing r? for now. will fix them shortly.
Attachment #8812491 - Flags: review?(jwalden+bmo)
Attachment #8812492 - Flags: review?(jwalden+bmo)
Attachment #8812493 - Flags: review?(jwalden+bmo)
Fixed nullptr and OOM handling. Re-posting the comment with some changes related to free/delete. Removed linebuf from note, now JSErrorBase has the following fields: * message (message_, ownsMessage_) * filename (filename) * line number (lineno) * column number (column) * error number (errorNumber) Now JSErrorReport has |JSErrorNotes* notes| field, that points JSErrorNotes allocated by js_new if the error has notes, and JSErrorReport destructor calls js_delete for it. |notes| field is a pointer because we want to allocate it before creating JSErrorReport, in frontend, and maybe in public API (not provided tho) JSErrorNotes has vector of JSErrorNotes::Note pointer (that is previously JSErrorNote, I chose JSErrorNotes::Note instead of JSErrorNote to avoid typo), that is a subclass of JSErrorBase, and it now has no extra member (because we no more need |nextNote| link) Each note is allocated either with js_new (in JSErrorNotes::add*) or pod_calloc+constructor call (in CopyErrorNote). JSErrorNotes destructor calls js_delete for each, since js_delete and "destructor+js_free" have same effect. Then, since now JSErrorReport may point JSErrorNotes that is allocated separately, exn_finalize now calls |fop->delete_| instead of |fop->free_|, to call destructor. (previously all members were allocated at once and destructor had no effect) JSErrorNotes has iterator, so each note can be iterated with: JSErrorNotes* notes = ...; // ... for (auto note : *notes) { ... } Currently JSErrorNotes provides addNote* methods only for js::ExclusiveContext, to call it from frontend, so it's not for public API. Maybe we could add other methods or functions when we want to provide public API later.
Attachment #8812491 - Attachment is obsolete: true
Attachment #8812616 - Flags: review?(jwalden+bmo)
(almost no change) Added Parser::errorWithNotes and Parser::errorWithNotesAt, that receives |UniquePtr<JSErrorNotes>| as 1st parameter. Also added |UniquePtr<JSErrorNotes>| parameter to underlying error reporting methods, to pass it down to TokenStream::reportCompileErrorNumberVA. Here's example code from bug 104442 patch. auto notes = MakeUnique<JSErrorNotes>(); uint32_t lineno, column; char lineNumber[16]; tokenStream.srcCoords.lineNumAndColumnIndex(prevPos, &lineno, &column); SprintfLiteral(lineNumber, "%u", lineno); if (!notes->addNoteLatin1(pc->sc()->context, getFilename(), lineno, column, GetErrorMessage, nullptr, JSMSG_REDECLARED_PREV, bytes.ptr(), lineNumber)) { return; } errorWithNotesAt(Move(notes), pos.begin, JSMSG_REDECLARED_VAR, DeclarationKindString(prevKind), bytes.ptr());
Attachment #8812492 - Attachment is obsolete: true
Attachment #8812617 - Flags: review?(jwalden+bmo)
Attached patch Part 4: Print error note in js::PrintError. (obsolete) (deleted) — — Splinter Review
Modified to follow changes above. Separated PrintErrorLine part since JSErrorNotes::Note doesn't have it. Also used range based loop for notes.
Attachment #8812493 - Attachment is obsolete: true
Attachment #8812618 - Flags: review?(jwalden+bmo)
Comment on attachment 8812616 [details] [diff] [review] Part 1: Add JSErrorBase, JSErrorNotes, JSErrorNotes::Note, and JSErrorReport.{notes,freeNotes}. Review of attachment 8812616 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +6164,5 @@ > + for (auto note : notes_) > + js_delete(note); > +} > + > +static JSErrorNotes::Note* Can't this return UniquePtr<JSErrorNotes::Note>? That would simplify error handling. @@ +6206,5 @@ > + if (!notes_.append(note)) { > + js_delete(note); > + return false; > + } > + return true; And then with the above function returning UP, this would just be return notes_.append(Move(note)); And same for the others. @@ +6257,5 @@ > +{ > + return notes_.length(); > +} > + > +JSErrorNotes* Return UniquePtr here -- should push the use-as-raw decision as far into callers, as a general rule, as possible. @@ +6265,5 @@ > + if (!copiedNotes) > + return nullptr; > + > + for (auto note : *this) { > + JSErrorNotes::Note* copy = CopyErrorNote(cx, note); Maybe name this clone or something, so it's not shadowing this function's name. @@ +6270,5 @@ > + if (!copy) > + return nullptr; > + > + if (!copiedNotes->notes_.append(copy)) { > + js_delete(copy); And then this would similarly be if (!copiedNotes->notes_.append(Move(copy))) return nullptr; ::: js/src/jsapi.h @@ +5332,5 @@ > freeMessage(); > } > > + // Source file name, URL, etc., or null. > + const char* filename; Add blank lines between each field and the preceding comment -- crowding together like this is hard to read. And a little vertical "waste" more than pays for itself in readability. Also, please don't bother aligning the field names. It'll just bitrot in short order, and it's easier to read a field/name when they're adjacent than when there's a blank space between that requires the eye to sync across. @@ +5460,5 @@ > + unsigned flags; > + // One of the JSExnType constants. > + int16_t exnType; > + // See the comment in ReadOnlyCompileOptions. > + bool isMuted : 1; Don't try vertically aligning these fields either. Also blank lines in between. ::: js/src/jsexn.cpp @@ +160,5 @@ > + return 0; > +} > + > +bool > +CopyExtraData(JSContext* cx, uint8_t*& cursor, JSErrorReport* copy, JSErrorReport* report) uint8_t** cursor, please, so the mutation is clearer at the caller. @@ +229,5 @@ > if (!cursor) > return nullptr; > > + T* copy = (T*)cursor; > + new (copy) T(); I believe this works more concisely: T* copy = new (cursor) T(); @@ +322,5 @@ > exn_finalize(FreeOp* fop, JSObject* obj) > { > MOZ_ASSERT(fop->maybeOffMainThread()); > if (JSErrorReport* report = obj->as<ErrorObject>().getErrorReport()) > + fop->delete_(report); If we're definitely deleting this stuff now, and have to do so, we perhaps should make some of the other pointers in these structures into UniquePtr. Separate patches/bugs, and maybe it needs to wait on having a proper string class to embed yet, anyway. :-\
Attachment #8812616 - Flags: review?(jwalden+bmo) → review+
Attachment #8812617 - Flags: review?(jwalden+bmo) → review+
updated to use UniquePtr
Attachment #8812618 - Attachment is obsolete: true
Attachment #8812618 - Flags: review?(jwalden+bmo)
Attachment #8826844 - Flags: review?(jwalden+bmo)
addressed review comments. Changed to use UniquePtr for JSErrorNotes::notes_ elements and JSErrorReport::notes, and some function return types.
Attachment #8812616 - Attachment is obsolete: true
Attachment #8827129 - Flags: review+
Attachment #8807261 - Attachment is obsolete: true
Attachment #8827130 - Flags: review+
rebased onto bug 1296814
Attachment #8812617 - Attachment is obsolete: true
Attachment #8827132 - Flags: review+
Added errorNotes getter to debugger object, that returns JSErrorReport::notes as an array, to use in devtools.
Attachment #8827134 - Flags: review?(jimb)
like JSErrorReport, added xpc::ErrorBase and moved shared properties from xpc::ErrorReport to xpc::ErrorBase, and added xpc::ErrorNote that extends xpc::ErrorBase.
Attachment #8827135 - Flags: review?(bobbyholley)
Also added nsIScriptErrorNote interface and nsScriptErrorNote implementation, and also nsIScriptError::{addNote,notes} to store/get notes. this is a bit different than others. there was already nsScriptErrorBase that implements nsIScriptError, and nsScriptErrorNote doesn't extend it. in XPCOM, error and note are totally different thing.
Attachment #8827137 - Flags: review?(bobbyholley)
posting remaining patches without review flag for now.
Attached patch Part 11.1: Show notes in devtools console. (obsolete) (deleted) — — Splinter Review
Comment on attachment 8827135 [details] [diff] [review] Part 6: Add xpc::ErrorBase, xpc::ErrorNote, and xpc::ErrorReport.mNotes. Review of attachment 8827135 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/nsXPConnect.cpp @@ +260,5 @@ > + error.Append(NS_LossyConvertUTF16toASCII(mFileName)); > + error.AppendLiteral(", line "); > + error.AppendInt(mLineNumber, 10); > + error.AppendLiteral(": "); > + error.Append(NS_LossyConvertUTF16toASCII(mErrorMsg)); Can you break this stringification of the file, line, and message into a helper on ErrorBase and share it between this method and the one below? @@ +344,5 @@ > + nsAString& aString) > +{ > + aString.Truncate(); > + if (aNote->message()) > + aString.Append(NS_ConvertUTF8toUTF16(aNote->message().c_str())); This string is guaranteed to be UTF8, right?
Attachment #8827135 - Flags: review?(bobbyholley) → review+
Comment on attachment 8827137 [details] [diff] [review] Part 7: Add nsIScriptErrorNote and nsIScriptError.{notes,addNotes}. Review of attachment 8827137 [details] [diff] [review]: ----------------------------------------------------------------- This patch has a bunch of machinery for creating and mutating notes over XPIDL. Is there actually any good reason to do that? I'd prefer to keep the XPIDL interface as slim as possible and make everything read-only.
Attachment #8827137 - Flags: review?(bobbyholley) → review-
Thank you for reviewing :) (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #49) > @@ +344,5 @@ > > + nsAString& aString) > > +{ > > + aString.Truncate(); > > + if (aNote->message()) > > + aString.Append(NS_ConvertUTF8toUTF16(aNote->message().c_str())); > > This string is guaranteed to be UTF8, right? yes, message() returns JS::ConstUTF8CharsZ. (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #50) > Comment on attachment 8827137 [details] [diff] [review] > Part 7: Add nsIScriptErrorNote and nsIScriptError.{notes,addNotes}. > > Review of attachment 8827137 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch has a bunch of machinery for creating and mutating notes over > XPIDL. Is there actually any good reason to do that? I'd prefer to keep the > XPIDL interface as slim as possible and make everything read-only. it's because error note isn't specific to JavaScript error, but other errors (like HTML/CSS parser) also can have it, and I thought this layer would be the point that other parts will generate those errors. I'll make it read-only for now. maybe we could add mutating feature when we need it.
actually, there's already one place that needs to mutate notes outside from XPConnect. it's in Part 8 (attachment 8827140 [details] [diff] [review]), it's converting error in worker to nsIScriptError. we could include xpcprivate.h and directly instantiate nsScriptErrorBase and nsScriptErrorNote there tho, is it reasonable? (xpcprivate.h is included from several files under dom/, but I'm not sure about this case)
Flags: needinfo?(bobbyholley)
Hm, if this stuff is being used by workers, it probably doesn't belong in XPConnect. I think we should probably move all the nsIScriptError stuff to dom/bindings. Boris, does that seem reasonable to you?
Flags: needinfo?(bobbyholley) → needinfo?(bzbarsky)
We could probably do that, yes. Note that we'd need to move nsScriptError too, in general.
Flags: needinfo?(bzbarsky)
which module should nsScriptError be stored if it's moved to dom/bindings ? looks like there's no all-in-one module under dom/ https://dxr.mozilla.org/mozilla-central/search?q=path%3Adom%2F+NSMODULE_DEFN&redirect=false currently XPConnect items are stored in nsLayoutModule https://dxr.mozilla.org/mozilla-central/rev/b3774461acc6bee2216c5f57e167f9e5795fb09d/layout/build/nsLayoutModule.cpp#912
for now I'll go with dedicated module for nsScriptError, placed under dom/bindings.
Please leave dom/bindings things in nsLayoutModule.
Comment on attachment 8826844 [details] [diff] [review] Part 4: Print error note in js::PrintError. Review of attachment 8826844 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscntxt.cpp @@ +445,5 @@ > + > + if (report->lineno) { > + UniquePtr<char> tmp(JS_smprintf("%s%u:%u ", prefix ? prefix.get() : "", report->lineno, > + report->column)); > + prefix.swap(tmp); Mild preference for prefix = Move(tmp); and elsewhere.
Attachment #8826844 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8827134 [details] [diff] [review] Part 5: Support notes in Debugger. Review of attachment 8827134 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. A few small comments. ::: js/src/jscntxt.cpp @@ +950,5 @@ > + if (!DefineProperty(cx, noteObj, cx->names().message, messageVal)) > + return nullptr; > + > + RootedValue filenameVal(cx); > + if (note->filename) { If the filename is indeed sometimes omitted, we get whatever RootedValue initializes things to by default. Is that `undefined`? ::: js/src/jscntxt.h @@ +717,5 @@ > ((void)ReportValueErrorFlags(cx, JSREPORT_ERROR, errorNumber, \ > spindex, v, fallback, arg1, arg2)) > > +JSObject* > +CreateErrorNotesArray(JSContext* cx, JSErrorReport* report); Does this want a MOZ_MUST_USE?
Attachment #8827134 - Flags: review?(jimb) → review+
now nsScriptError/nsIScriptError are moved to dom/bindings, and no need to make it mutable via XPIDL. changed the code to directly mutate it by nsScriptErrorBase/nsScriptErrorNote methods.
Attachment #8827137 - Attachment is obsolete: true
Attachment #8832566 - Flags: review?(bobbyholley)
Added WorkerErrorReport class to store all error related information, and changed error related methods in Worker to use that class instead of receiving as separated parameters. also added WorkerErrorNote to store notes.
Attachment #8827140 - Attachment is obsolete: true
Attachment #8832568 - Flags: review?(bobbyholley)
Comment on attachment 8832566 [details] [diff] [review] Part 7: Add nsIScriptErrorNote and nsIScriptError.notes. Review of attachment 8832566 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the review delay. Thanks for the careful and excellent work on this!
Attachment #8832566 - Flags: review?(bobbyholley) → review+
Comment on attachment 8832568 [details] [diff] [review] Part 8: Add WorkerErrorBase, WorkerErrorNote, and WorkerErrorReport. Review of attachment 8832568 [details] [diff] [review]: ----------------------------------------------------------------- I'm tempted to kick this over to bkelly, but it seems pretty mechanical.
Attachment #8832568 - Flags: review?(bobbyholley) → review+
Added getErrorNotes testing function, that receives an error object and returns error notes array, created by CreateErrorNotesArray, that is same thing as debugger uses.
Attachment #8836290 - Flags: review?(jwalden+bmo)
Also supported notes in getLastWarning. warning object returned by it now has "notes" property, with error notes array created by CreateErrorNotesArray
Attachment #8827141 - Attachment is obsolete: true
Attachment #8827142 - Attachment is obsolete: true
Attachment #8836291 - Flags: review?(jwalden+bmo)
Added "notes" property to EvaluationResult, PageError, and Message. notes is created from debugger object (Part 5) or nsIScriptErrorNote (Part 7) notes are displayed in same message as error. the content of note is also the target of search filter. I'll post the patch for stub update later
Attachment #8827143 - Attachment is obsolete: true
Attachment #8836293 - Flags: review?(chevobbe.nicolas)
Comment on attachment 8836293 [details] [diff] [review] Part 11.1: Show notes in devtools console. Review of attachment 8836293 [details] [diff] [review]: ----------------------------------------------------------------- This looks good thanks ! But no r+ since it lacks tests and, as you said, stub generation. I'll definitely r+ when we'll have those. ::: devtools/client/webconsole/new-console-output/components/message.js @@ +160,5 @@ > } > > + let notesNodes = []; > + if (notes) { > + for (let note of notes) { nit: For consistency's sake, could we use `notes.map` instead of a for..of loop ?
Thank you for reviewing :) (In reply to Nicolas Chevobbe [:nchevobbe] from comment #67) > Comment on attachment 8836293 [details] [diff] [review] > Part 11.1: Show notes in devtools console. > > Review of attachment 8836293 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good thanks ! But no r+ since it lacks tests and, as you said, > stub generation. I'll definitely r+ when we'll have those. actually, there's no way to test this before bug 420857 or bug 104442, since there's no place that generates error note. I'm about to fix bug 104442 shortly after this, there mocha test is added.
> I'm about to fix bug 104442 shortly after this, there mocha test is added. Wow, fixing a 16 year old bug will be rad ! Do you mean you write tests related to error notes in the patch for bug 104442 ? I still think the patch I reviewed should have tests to make sure the components do show the notes. Since those mocha tests use stubs, you can get those who already exist, and add the `notes` props in the message to create the components.
Attached patch Part 11.2: Update stub. (deleted) — — Splinter Review
Updated stubs.
Attachment #8836922 - Flags: review?(chevobbe.nicolas)
Attached patch Part 11.3: Add mocha test. (deleted) — — Splinter Review
Added mocha test that uses existing data + dummy note(s).
Attachment #8836925 - Flags: review?(chevobbe.nicolas)
Comment on attachment 8836925 [details] [diff] [review] Part 11.3: Add mocha test. Review of attachment 8836925 [details] [diff] [review]: ----------------------------------------------------------------- Great !
Attachment #8836925 - Flags: review?(chevobbe.nicolas) → review+
Attachment #8836922 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8836293 [details] [diff] [review] Part 11.1: Show notes in devtools console. Review of attachment 8836293 [details] [diff] [review]: ----------------------------------------------------------------- r+ if try is green :)
Attachment #8836293 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8836290 [details] [diff] [review] Part 9: Add getErrorNotes testing function to extract error notes from exception. Review of attachment 8836290 [details] [diff] [review]: ----------------------------------------------------------------- Not super-happy about having a function added without at least one test included in the selfsame patch, but okay. I guess there are more-indirect uses in the other patches, or so it appears. ::: js/src/builtin/TestingFunctions.cpp @@ +4809,5 @@ > #endif > > + JS_FN_HELP("getErrorNotes", GetErrorNotes, 1, 0, > +"getErrorNotes(error)", > +" Returns an array of error note."), array of error notes or maybe error note strings which would explain what exactly a note is, if not necessarily the form of it.
Attachment #8836290 - Flags: review?(jwalden+bmo) → review+
Attachment #8836291 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcd8c6fe6bb6ddbb38bbdf72f6365a0faeb56f2 Bug 1283712 - Part 1: Add JSErrorBase, JSErrorNotes, JSErrorNotes::Note, and JSErrorReport.{notes,freeNotes}. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/c7a11d17916df6fad0884230d1010621f0cd1311 Bug 1283712 - Part 2: Add JSEXN_NOTE. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/f827db18261779031504eb8a5fe9f6e1fcc683d2 Bug 1283712 - Part 3: Add Parser::errorWithNotes and Parser::errorWithNotesAt. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/687e2816ac03e0d4919c50366c0b20a8c26cca05 Bug 1283712 - Part 4: Print error note in js::PrintError. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/00672a065d8c13e2399c55f0d86226f83afb9393 Bug 1283712 - Part 5: Support notes in Debugger. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/003d483f64e0bf69ff6e7d1e9b02a9b74dfa4cc4 Bug 1283712 - Part 6: Add xpc::ErrorBase, xpc::ErrorNote, and xpc::ErrorReport.mNotes. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/de4875e04b7747d22377009d016feec433e666d1 Bug 1283712 - Part 7: Add nsIScriptErrorNote and nsIScriptError.notes. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/ca906f20c23da421a614df6b9f16ee7711ed84cb Bug 1283712 - Part 8: Add WorkerErrorBase, WorkerErrorNote, and WorkerErrorReport. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/1135a29fbc37a90cea364599c973e0918206a3e5 Bug 1283712 - Part 9: Add getErrorNotes testing function to extract error notes from exception. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/231c25dff4ce02cca0ada64fa31be62a30e864fe Bug 1283712 - Part 10: Support notes in getLastWarning shell-only testing function. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce13b03d9562b51debd438eeb46c33dd3b4c448 Bug 1283712 - Part 11.1: Show notes in devtools console. r=nchevobbe https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6d22e44e92b60efd7ee9238011323b72d1edfb Bug 1283712 - Part 11.2: Update stub. r=nchevobbe https://hg.mozilla.org/integration/mozilla-inbound/rev/aeca66c82a3cc92f18b60900260857c93e8ac2cd Bug 1283712 - Part 11.3: Add mocha test. r=nchevobbe
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: