Closed
Bug 1255133
Opened 8 years ago
Closed 8 years ago
Surface the most common JavaScript errors.
Categories
(DevTools :: Console, defect, P3)
DevTools
Console
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mrrrgn, Assigned: mrrrgn)
References
(Blocks 1 open bug)
Details
(Whiteboard: [btpp-backlog])
Attachments
(2 files, 4 obsolete files)
21.71 KB,
patch
|
Details | Diff | Splinter Review | |
4.09 KB,
patch
|
miker
:
review+
benjamin
:
feedback-
|
Details | Diff | Splinter Review |
In bug 1245877 we've been adding infrastructure for linking JavaScript errors with external documentation. Because there are 400+ errors, to maximize impact, it will be important to triage which errors get articles. Given that, let's see if we can rank JavaScript errors by their frequency of appearance in the web console. This should be fun to know !
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → winter2718
Updated•8 years ago
|
Component: Developer Tools → Developer Tools: Console
Priority: -- → P3
Whiteboard: [btpp-backlog]
Assignee | ||
Comment 1•8 years ago
|
||
This needs an integration test. I'm asking for feedback first to ensure that my approach isn't atrocious. If I get the tests finished soon I'll just change the flag to an r.
Attachment #8731488 -
Flags: feedback?(bgrinstead)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8731488 [details] [diff] [review] errmsgs.diff Actually, I'm gonna put a tiny bit more work into this before flagging for review. "We can do bettah!" http://www.slate.com/content/dam/slate/blogs/xx_factor/2016/01/26/bernie_sanders_just_can_t_get_it_right_with_women_s_groups/505435298-democratic-presidential-candidate-senator-bernie.jpg.CROP.promo-xlarge2.jpg
Attachment #8731488 -
Flags: feedback?(bgrinstead)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8731488 -
Attachment is obsolete: true
Attachment #8731617 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 4•8 years ago
|
||
I named the last one ".log" instead of ".diff"
Attachment #8731617 -
Attachment is obsolete: true
Attachment #8731617 -
Flags: review?(bgrinstead)
Attachment #8731619 -
Flags: review?(bgrinstead)
Comment 5•8 years ago
|
||
Comment on attachment 8731619 [details] [diff] [review] errormessages.diff Review of attachment 8731619 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/webconsole/console-output.js @@ +1,2 @@ > /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ Unintentional whitespace change? @@ +1369,5 @@ > }; > + > + let messages = [msg]; > + > + if (errorDocLink) Nit: please use braces around if ::: devtools/client/webconsole/test/browser_webconsole_errordoc_urls.js @@ +5,5 @@ > + > +// When strings containing URLs are entered into the webconsole, check > +// its output and ensure that the output can be clicked to open those URLs. > + > +"use strict"; For testing, here's what I'm envisioning. Keep this test as an integration test to make sure the frontend is adding a link with correct behavior. Then add some extra assertions to test the protocol more directly (maybe in in shared/webconsole/test/test_jsterm.html or a new test in that folder) that fully test error inputs and the expected exceptionDocURL that the server returns. ::: devtools/server/actors/webconsole.js @@ +872,5 @@ > > let evalInfo = this.evalWithDebugger(input, evalOptions); > let evalResult = evalInfo.result; > let helperResult = evalInfo.helperResult; > + let dbgWindow = this.dbg.makeGlobalObjectReference(this.evalWindow); This is only going to run with eval results - i.e. if I enter "(42).toString(0);" into the console. But it won't get page errors (like if a page tries to run the same thing in a script. Is this intentional? If you are getting the required error data from the nsIScriptError then similar logic could be wired up in the ConsoleServiceListener when the error is observed by the console actor @@ +890,5 @@ > error.unsafeDereference(); > errorMessage = unsafeDereference && unsafeDereference.toString > ? unsafeDereference.toString() > : String(error); > + errorDocURL = ErrorDocs.GetURL(dbgWindow.getErrorMessageName(unsafeDereference)); Right now accessing properties on the unsafeDereference object will cause an exception in the worker debugging global scope so we need to be careful about that and wrap it in an if. Even still, the unsafe dereference is supposed to go away as in: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/nxbM4hGjQ2Y. Is unsafeDereference needed here anyway or could you use 'error'? (which I think is the Debugger.Object)
Attachment #8731619 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #5) > Comment on attachment 8731619 [details] [diff] [review] > errormessages.diff > > Review of attachment 8731619 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/webconsole/console-output.js > @@ +1,2 @@ > > /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > > Unintentional whitespace change? > > @@ +1369,5 @@ > > }; > > + > > + let messages = [msg]; > > + > > + if (errorDocLink) > > Nit: please use braces around if > > ::: devtools/client/webconsole/test/browser_webconsole_errordoc_urls.js > @@ +5,5 @@ > > + > > +// When strings containing URLs are entered into the webconsole, check > > +// its output and ensure that the output can be clicked to open those URLs. > > + > > +"use strict"; > > For testing, here's what I'm envisioning. Keep this test as an integration > test to make sure the frontend is adding a link with correct behavior. Then > add some extra assertions to test the protocol more directly (maybe in in > shared/webconsole/test/test_jsterm.html or a new test in that folder) that > fully test error inputs and the expected exceptionDocURL that the server > returns. > > ::: devtools/server/actors/webconsole.js > @@ +872,5 @@ > > > > let evalInfo = this.evalWithDebugger(input, evalOptions); > > let evalResult = evalInfo.result; > > let helperResult = evalInfo.helperResult; > > + let dbgWindow = this.dbg.makeGlobalObjectReference(this.evalWindow); > > This is only going to run with eval results - i.e. if I enter > "(42).toString(0);" into the console. But it won't get page errors (like if > a page tries to run the same thing in a script. Is this intentional? > > If you are getting the required error data from the nsIScriptError then > similar logic could be wired up in the ConsoleServiceListener when the error > is observed by the console actor This is better, pure derp on my part. > @@ +890,5 @@ > > error.unsafeDereference(); > > errorMessage = unsafeDereference && unsafeDereference.toString > > ? unsafeDereference.toString() > > : String(error); > > + errorDocURL = ErrorDocs.GetURL(dbgWindow.getErrorMessageName(unsafeDereference)); > > Right now accessing properties on the unsafeDereference object will cause an > exception in the worker debugging global scope so we need to be careful > about that and wrap it in an if. Even still, the unsafe dereference is > supposed to go away as in: > https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/ > nxbM4hGjQ2Y. Is unsafeDereference needed here anyway or could you use > 'error'? (which I think is the Debugger.Object) Ahh great, I'll go with that. Thanks !
Assignee | ||
Comment 7•8 years ago
|
||
I've decided to split this patch up into two pieces. The first piece adds a field to nsIScriptError and gets us "[Learn More]" links for script errors. Every error we currently support is tested. The second patch will add support to the js terminal.
Attachment #8731619 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8734214 -
Attachment is obsolete: true
Attachment #8734215 -
Flags: review?(bgrinstead)
Comment 9•8 years ago
|
||
Are these patches in the wrong bug? Should be bug 1179876 maybe?
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #9) > Are these patches in the wrong bug? Should be bug 1179876 maybe? Yeah, moving the patch over >.<
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8734215 [details] [diff] [review] errordocs-1of2.diff Moving this patch to another bug.
Attachment #8734215 -
Flags: review?(bgrinstead)
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/873bdc9379fd
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 14•8 years ago
|
||
I goof'd by attaching this bug number to a patch that wasn't totally related. Re-opening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•8 years ago
|
||
One important note: afaiu this will only record errors after the webconsole is opened. That is, if some page has errors and the webconsole is opened they will be counted. If the page has errors and the webconsole is never opened they will not be counted. I believe this makes sense considering our reason for collecting these stats in the first place (to triage error documentation for developers).
Attachment #8739333 -
Flags: review?(bgrinstead)
Comment 16•8 years ago
|
||
Comment on attachment 8739333 [details] [diff] [review] telemetry.diff Review of attachment 8739333 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding the review to Mike
Attachment #8739333 -
Flags: review?(bgrinstead) → review?(mratcliffe)
Comment on attachment 8739333 [details] [diff] [review] telemetry.diff Review of attachment 8739333 [details] [diff] [review]: ----------------------------------------------------------------- Seems simple enough... r+. ::: toolkit/components/telemetry/Histograms.json @@ +7767,5 @@ > "n_buckets": 20, > "description": "The amount of time spent in a specific performance tool view, keyed by view name (waterfall, js-calltree, js-flamegraph, etc)." > }, > + "DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED": { > + "alert_emails": ["mphillips@mozilla.com"], Are you sure you want to receive all those error emails?
Attachment #8739333 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #17) > Comment on attachment 8739333 [details] [diff] [review] > telemetry.diff > > Review of attachment 8739333 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems simple enough... r+. > > ::: toolkit/components/telemetry/Histograms.json > @@ +7767,5 @@ > > "n_buckets": 20, > > "description": "The amount of time spent in a specific performance tool view, keyed by view name (waterfall, js-calltree, js-flamegraph, etc)." > > }, > > + "DEVTOOLS_JAVASCRIPT_ERROR_DISPLAYED": { > > + "alert_emails": ["mphillips@mozilla.com"], > > Are you sure you want to receive all those error emails? I was planning to just make a filter for them and see how it goes. It's my first telemetry probe so I'm not sure what to expect. If it's overwhelming I'll throw up another patch.
Needs a review from :bsmedberg per the rules on the telemetry probe page[1]. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6b92a06c6abe
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 22•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19) > Needs a review from :bsmedberg per the rules on the telemetry probe page[1]. > > [1]: > https://developer.mozilla.org/en-US/docs/Mozilla/Performance/ > Adding_a_new_Telemetry_probe Did this ever happen? I don't see any data review and this sounds potentially sensitive. Does this have any bounding mechanism? This sounds like a lot of distinct error labels could be submitted here, not just a few known ones.
Flags: needinfo?(winter2718)
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #22) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19) > > Needs a review from :bsmedberg per the rules on the telemetry probe page[1]. > > > > [1]: > > https://developer.mozilla.org/en-US/docs/Mozilla/Performance/ > > Adding_a_new_Telemetry_probe > > Did this ever happen? I don't see any data review and this sounds > potentially sensitive. > > Does this have any bounding mechanism? This sounds like a lot of distinct > error labels could be submitted here, not just a few known ones. Apologies, it did not. That said, an error like "new Error('some sensitive stuff')" will not be recorded here. Only errors from js/src/js.msg are recorded. Further, even the variable names, etc... which might show up in an error message are unknown to us in the probe. Only the type of error.
Flags: needinfo?(winter2718)
Comment 24•8 years ago
|
||
Sorry for the late follow-up... can you flag Benjamin for at least a post-landing review to see if any improvements are needed?
Flags: needinfo?(winter2718)
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #24) > Sorry for the late follow-up... can you flag Benjamin for at least a > post-landing review to see if any improvements are needed? On it, thanks!
Flags: needinfo?(winter2718)
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8739333 [details] [diff] [review] telemetry.diff Review of attachment 8739333 [details] [diff] [review]: ----------------------------------------------------------------- Flagged for privacy review based on comment 22 - also note: "an error like "new Error('some sensitive stuff')" will not be recorded here. Only errors from js/src/js.msg are recorded."
Attachment #8739333 -
Flags: feedback?(benjamin)
Comment 27•8 years ago
|
||
Comment on attachment 8739333 [details] [diff] [review] telemetry.diff It seems weird that we require a keyed histogram for this instead of an enumeration, but I'm willing to accept that there's some reason the keyed histogram is preferable (it's definitely less efficient to process). But the histogram description is insufficient. You need to write down 1) when the histogram is recorded 2) what the histogram records In this case I'm guessing that a good description would be "A true value is recorded each time a JS engine error is recorded in the web console. No value is recorded when the web console is not visible to the user. A false value is never recorded." 3) The key format and possible values. You can either list all the possible values, or (more likely in this case) reference a list/enumeration of possible values elsewhere in the source tree.
Attachment #8739333 -
Flags: feedback?(benjamin) → feedback-
Comment 28•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #27) > It seems weird that we require a keyed histogram for this instead of an > enumeration Morgan, can we move this to an enumerated (non-keyed) histogram in a follow-up bug?
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•