1313119 - Handle exceptions in reps more gracefully

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Lin Clark [:linclark]]

Currently if there is an exception in Reps code (e.g. Bug 1313050), then it makes the console unusable.

As pointed out by Simon Lindholm in that bug, there are likely other edge cases that Reps doesn't handle. We should see if there's an easy way to handle those cases generically in web console.

Comment 1

[Julian Descottes [:jdescottes]]

There is no clean way at the moment to catch rendering errors occurring in the component tree (see https://github.com/facebook/react/issues/2461). An error handler is currently under development and the early version can be tested using `unstable_handleError`.

I will attach a patch showing how this could be used here to have a single error handler for all reps. 
Otherwise I think we should introduce an util/helper to wrap the render method of each rep in a try/catch. This requires modifying all reps but doesn't rely on an in-development API.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: bug1313119.wip.patch

Here is an example using unstable_handleError, I'll try to work on an alternative approach to have something to compare it against.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: bug1313119.wrap.wip.patch

And here is another patch, not relying on unstable_handleError, instead we modify all classes used to render reps to add a try / catch around the render method.

The fallback is to render a string rep with an error message at the moment but we could also have a custom fallback rep for each class.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8822687 [details] [diff] [review]
bug1313119.wip.patch

At a first glance, I liked the simplicity of it :)
But then I thought of some case where we create Rep components directly (i.e not by using the Rep component), which would not be handled here.
For now I don't think that there are much of these cases (I know we're directly calling the StringRep in the console for example), but for a long term solution I think this is better if we handle this at a component level.
If someday the inspector uses Reps, it might directly call the DomNodes component and so errors should be handled there directly.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8822697 [details] [diff] [review]
bug1313119.wrap.wip.patch

I really like that, it much smarter that what I would have done :)

One thing I'm not really confident with is the way we get the render function from the class prototype. There was some discussion at some point to have some of the Reps as pure component (from what I understand : only one function used for the rendering), and I don't know if this play nicely with retrieving it from the prototype.
Maybe we could be explicit about what we pass to the wrapRender in each component ? 

```
let RegExp = React.createClass({
  ...
  render: wrapRender(function(props) {
    // same old rendering function
  })
})

let PureComponent = wrapRender(function(prop) {
  //works as well
})
```

Minor nit : I think we could directly use a dom element to output the error since StringRep can also be broken, which would led to another error. I agree this is really unlikely to happen since StringRep is pretty simple, but I prefer to be paranoid :)

Overall, this looks good ! I think we should talk with Brian about what to display, IIRC we was talking about showing a message asking the user to file a bug, which would be nice.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1313119 - wrap all rep render() methods with try/catch;

Review commit: https://reviewboard.mozilla.org/r/102688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102688/

Comment 7

[Julian Descottes [:jdescottes]]

I updated the "wrapRender" patch following your comments Nicolas. The main reason I preferred wrapping classes rather than methods was because it would end up in a smaller diff :) I'm fine with wrapping the method directly. 

For now I use a single hardcoded error message, but maybe we should make it configurable? We could have a new prop supported by all refs that would be used if the render fails.

One thing to note, is that this approach will "inline" the error message: if you render an array, and the second item fails to render, you will see the error message in the middle of the array. I don't think this can be avoided unless we use the other approach I mentioned.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

https://reviewboard.mozilla.org/r/102688/#review103276

Looks good to me !

::: devtools/client/shared/components/reps/prop-rep.js:25
(Diff revision 1)
>    /**
>     * Property for Obj (local JS objects), Grip (remote JS objects)
>     * and GripMap (remote JS maps and weakmaps) reps.
>     * It's used to render object properties.
>     */
> -  let PropRep = React.createFactory(React.createClass({
> +  let PropRep = React.createClass({

Can you explain me why we remove the createFactory on this ?
Are we already applying createFactories on the PropRep where we require it ?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

> For now I use a single hardcoded error message, but maybe we should make it configurable? We could have a new prop supported by all refs that would be used if the render fails.

I agree, this could be a nice thing to have

> One thing to note, is that this approach will "inline" the error message: if you render an array, and the second item fails to render, you will see the error message in the middle of the array. I don't think this can be avoided unless we use the other approach I mentioned.

That's true, and maybe that could be a little misleading. We can think of that in a follow up maybe, see if ideas other than using the `unstable_handleError` emerge.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Oh, I forgot one thing, could you add test to this ? Or at least file a bug for implementing test on this to make sure we wave the expected behavior here. Thanks

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

https://reviewboard.mozilla.org/r/102688/#review103276

> Can you explain me why we remove the createFactory on this ?
> Are we already applying createFactories on the PropRep where we require it ?

As far as I can tell, yes: http://searchfox.org/mozilla-central/search?q=prop-rep&case=false&regexp=false&path=

Also this was the only rep to export a factory rather than a class.

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102688/diff/1-2/

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: reps_render_failure.png

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

List of changes since your last review:
- aded a test 
- changed the string to a shorter "Invalid object"
- added a dedicated class to differentiate it properly from a random string (smaller font size and a border)

Let me know what you think!

Comment 15

[Julian Descottes [:jdescottes]]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00222da19632c1dee1292337dbab1ffb24ca8d6a

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

https://reviewboard.mozilla.org/r/102688/#review103506

One minor nit but it looks good (in the code and visually)

::: devtools/client/shared/components/test/mochitest/test_reps_failure.html:11
(Diff revision 2)
> +<!--
> +Test fallback for rep rendering when a rep fails to render.
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Rep test - RegExp</title>

s/RegExp/Failure ?

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102688/diff/2-3/

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8824142 [details]
Bug 1313119 - wrap all rep render() methods with try/catch;

https://reviewboard.mozilla.org/r/102688/#review103506

> s/RegExp/Failure ?

Thanks for the review! 
I keep having weird failures on try for OSX debug, I don't think it's related to my patch but I'll investigate a bit.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42ea47c0d3d4
wrap all rep render() methods with try/catch;r=nchevobbe

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42ea47c0d3d4