1615518 - Improve error links for web API errors

Status: NEW

(DevTools :: Console, task, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: test.patch

STEPS TO REPRODUCE: Load the following testcase:

  <script>
  document.querySelector();
  </script>
  <script>
    document.querySelector(":::");
  </script>
  <script>
    Object.defineProperties();
  </script>

EXPECTED RESULTS: All the console messages have useful "Learn More" links.

ACTUAL RESULTS: Only the first and third one do, and the link is to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/More_arguments_needed which is of somewhat marginal utility here, in my opinion. In particular, it does not make it very easy to see how many arguments are needed.

ADDITIONAL INFORMATION: The attached patch changes things so that the links are to:

• https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector
• https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector
• https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperties

respectively.

There might be a less hacky way to do this by communicating more information from the original exception, perhaps, but this works well enough for the moment, I think, and doesn't require significant SpiderMonkey or DOM changes. I'd love some feedback on it.

I also wonder whether MDN could help out with the guessing game of which URL to use by exposing redirects from a more uniform URL syntax to the right places...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note: that patch does not handle errors from constructors (which start with "Foo constructor:"), and does not do a great job at argument conversion errors, because of how we format the messages for those. But if the general idea seems OK I can change codegen to format things in a more friendly way and add constructors.

Comment 2

[Boris Zbarsky [:bzbarsky]]

One other thing was suggested on twitter. If a message looks like "TypeError: Document.querySelector requires at least 1 argument, but only 0 were passed" we could link "Document.querySelector" to the docs for that and link the "requires ..." text, which corresponds to "JSMSG_MORE_ARGS_NEEDED", to the docs for that. Unfortunately, we don't currently have a good way to determine which error text corresponds to the "JSMSG_MORE_ARGS_NEEDED" bit.

This matters more for things like "JSMSG_NOT_ITERABLE" where the general shape of the problem may not be really obvious from the message itself...

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

I like this idea a lot.
In case of JSMSG_MORE_ARGS_NEEDED we could even link to the #Syntax anchor, so users are directly at the right spot to check what the code is doing wrong.

For some reason the patch does not work for me? Also, I guess relying on an error message shape is brittle and could lead to breakage if the message changes. Not sure what the alternative would be though.

Comment 4

[Boris Zbarsky [:bzbarsky]]

For some reason the patch does not work for me?

Odd. It applies fine on current m-c:

wget -O - 'https://bug1615518.bmoattachments.org/attachment.cgi?id=9126643' | patch -p1 --dry-run

(or equivalent with curl). Or do you mean something else by "does not work"?

Also, I guess relying on an error message shape is brittle

Yes, though we do control the shape and I am working on getting more messages into that shape. But yes, we would need tests that make sure the shape is as-expected so we catch it changing.

Not sure what the alternative would be though

Sending a more-structured thing up from the original exception point. The fact that we are collapsing the name and message into a single string is one thing we could probably avoid somewhat, I would think. Past that, we'd need to attach more metadata to the actual exception objects. Which we can do, but it takes a bunch of work across DOM and SpiderMonkey.

I'm strongly in favor of not letting the perfect get in the way of the good here. As far as prioritization, I am happy to put some work into this if there's a particular approach that would be acceptable.

The big open question is whether we should just mess with the "Learn More" link like this patch or more generally linkify the relevant part of the message...

Comment 5

[:Harald Kirschner :digitarald]

(clearing ni?, nchevobbe is on it)

The big open question is whether we should just mess with the "Learn More" link like this patch or more generally linkify the relevant part of the message...

We should reach some decision on that in: https://github.com/firefox-devtools/ux/issues/113 – I'll nudge the issue.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Bug 1617657 highlights an other issue with the learn more link

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, I'm not quite sure why the bug 1617657 case behaves like it does or even where it should link....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, I'm not quite sure why the bug 1617657 case behaves like it does

Now I do know. It's because it's using the numeric error identifier, which does not uniquely identify the error: the set of numbers used by SpiderMonkey overlaps the set used by DOM but they mean totally different things. Which is another reason to stop using those for the DOM case, at least, unless we can disambiguate somehow.

For example, right now I am seeing the "Learn More" link point to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Reduce_of_empty_array_with_no_initial_value when the actual error is "you passed a SharedArrayBuffer to a function that doesn't accept shared ones". Or point to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_delete when the actual error is "The argument you tried to pass can't be converted to any of the types in the union that the function is declared to take". Or point to https://developer.mozilla.org/docs/Web/JavaScript/Reference/Errors/Bad_regexp_flag when the error is "Your string had a character outside the [0-255] range, so can't be a ByteString".

Comment 9

[Boris Zbarsky [:bzbarsky]]

By the way, for the SharedArrayBuffer case linking to the actual exception description might in fact be helpful...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly better diff, with various comments about string formats and remaining issues

There are still problems here, but I think it's an improvement over what we are doing right now....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly better diff, with various comments about string formats and remaining issues

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly better diff, with various comments about string formats and remaining issues

Comment 13

[Tom S]

With the patch in bug 1617657, errorMessageName will now contain the names defined in dom/bindings/Errors.msg. That should make it easier to add MDN docs for certain errors.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.