Handle exceptions in reps more gracefully

RESOLVED FIXED in Firefox 53

Status

P2
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: linclark, Assigned: jdescottes)

Tracking

unspecified
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1308219
(Reporter)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8822687 [details] [diff] [review]
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.
Attachment #8822687 - Flags: feedback?(chevobbe.nicolas)
(Assignee)

Comment 3

2 years ago
Created attachment 8822697 [details] [diff] [review]
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

2 years ago
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 5

2 years ago
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+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8822697 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
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

2 years ago
mozreview-review
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+

Comment 9

2 years ago
> 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

2 years ago
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
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
Created attachment 8824389 [details]
reps_render_failure.png
(Assignee)

Comment 14

2 years ago
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 16

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42ea47c0d3d4
wrap all rep render() methods with try/catch;r=nchevobbe

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42ea47c0d3d4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.