Closed Bug 1255133 Opened 8 years ago Closed 8 years ago

Surface the most common JavaScript errors.

Categories

(DevTools :: Console, defect, P3)

defect

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)

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: nobody → winter2718
Component: Developer Tools → Developer Tools: Console
Priority: -- → P3
Whiteboard: [btpp-backlog]
Attached patch errmsgs.diff (obsolete) — Splinter Review
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)
Attached file errormessages.log (obsolete) —
Attachment #8731488 - Attachment is obsolete: true
Attachment #8731617 - Flags: review?(bgrinstead)
Attached patch errormessages.diff (obsolete) — Splinter Review
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 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)
(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 !
Attached patch errordocs-1of2.diff (obsolete) — Splinter Review
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
Attachment #8734214 - Attachment is obsolete: true
Attachment #8734215 - Flags: review?(bgrinstead)
Are these patches in the wrong bug? Should be bug 1179876 maybe?
(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 >.<
Comment on attachment 8734215 [details] [diff] [review]
errordocs-1of2.diff

Moving this patch to another bug.
Attachment #8734215 - Flags: review?(bgrinstead)
https://hg.mozilla.org/mozilla-central/rev/873bdc9379fd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blocks: 1261877
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 → ---
Attached patch telemetry.diffSplinter Review
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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/6b92a06c6abe
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 1264095
(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)
(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)
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)
(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)
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 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-
(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?
Depends on: 1341996
Product: Firefox → DevTools
See Also: → 1622135
You need to log in before you can comment on or make changes to this bug.