Closed Bug 1313119 Opened 5 years ago Closed 5 years ago

Handle exceptions in reps more gracefully

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: linclark, Assigned: jdescottes)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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.
Here is an example using unstable_handleError, I'll try to work on an alternative approach to have something to compare it against.
Attachment #8822687 - Flags: feedback?(chevobbe.nicolas)
Attached patch bug1313119.wrap.wip.patch (obsolete) — Splinter Review
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 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.
Attachment #8822687 - Flags: feedback?(chevobbe.nicolas) → feedback-
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.
Attachment #8822697 - Flags: feedback+
Attachment #8822697 - Attachment is obsolete: true
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 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 ?
Attachment #8824142 - Flags: review?(chevobbe.nicolas) → review+
> 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.
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 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 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!
Attachment #8824142 - Flags: review+ → review?(chevobbe.nicolas)
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 ?
Attachment #8824142 - Flags: review?(chevobbe.nicolas) → review+
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.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42ea47c0d3d4
wrap all rep render() methods with try/catch;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/42ea47c0d3d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.