Closed Bug 1660435 Opened 4 years ago Closed 4 years ago

Introduce top level Error boundary Component

Categories

(DevTools :: Netmonitor, enhancement, P3)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox84 fixed)

Status:
RESOLVED FIXED
Milestone:
84 Branch
Tracking Flags:
Tracking Status
firefox84 --- fixed

People

(Reporter: Honza, Assigned: codywelsh)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Bug 1660435 - Introduce top level Error boundary Component. r=honza
47 bytes, text/x-phabricator-request
Details | Review
image.png
28.60 KB, image/png
Details

From: https://reactjs.org/docs/error-boundaries.html

Error boundaries are React components that catch JavaScript errors anywhere in their child component tree, log those errors, and display a fallback UI instead of the component tree that crashed. Error boundaries catch errors during rendering, in lifecycle methods, and in constructors of the whole tree below them.

It would be great to have such component in the Network panel. It could help to with bugs where the entire panel goes blank, like e.g. this one:

The component could render:

  • The error stack + description
  • Some instructions about how to file a bug (and copy the error)
  • Link to bugzilla new bug (in the Network component) page

Honza

This would be cool to work on - will start looking into this, if you're confident in assigning it to me. :)

Flags: needinfo?(odvarko)

Assigned to you, thanks for the help!

Honza

Assignee: nobody → codywelsh
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)

Thanks! My thoughts so far:

  • Assuming we only want a single ErrorBoundary component (instead of multiple components for various different portions of the Network Monitor), I'm thinking we should place it around the encapsulating App to catch any errors thrown by the entire panel.

  • For testing, I'm not yet totally familiar with Mozilla's tooling - however, I'm thinking that we probably want to force all child components to throw an error programmatically (if possible), and then verify that our ErrorBoundary component is rendering as expected in each case. I'm open to any suggestions, here, though. Maybe we don't need to be quite that exhaustive?

In any case, I'm glad to be of help! I'll ask questions if I get stuck.

Flags: needinfo?(odvarko)
See Also: → 1659127

Thanks for the update!

(In reply to Cody Welsh from comment #3)

Thanks! My thoughts so far:

  • Assuming we only want a single ErrorBoundary component (instead of multiple components for various different portions of the Network Monitor), I'm thinking we should place it around the encapsulating App to catch any errors thrown by the entire panel.

Yes, this makes sense.
The name of the component could be AppErrorBoundary
It could live in the same directory
And we should update the src/app.js file to render the new error boundary component.

The UI rendered by AppErrorBoundary could:

  • For testing, I'm not yet totally familiar with Mozilla's tooling - however, I'm thinking that we probably want to force all child components to throw an error programmatically (if possible), and then verify that our ErrorBoundary component is rendering as expected in each case. I'm open to any suggestions, here, though. Maybe we don't need to be quite that exhaustive?

What about programmatically damage data in a reducer e.g. set requests in the requests reducer to null and rerender, this should cause an exception - and make the entire panel blank. You can use it for testing as well as for automated test eventually.

Thanks for working on this, this will be great architecture improvement!

Honza

Flags: needinfo?(odvarko)

What about programmatically damage data in a reducer e.g. set requests in the requests reducer to null and rerender, this should cause an exception - and make the entire panel blank. You can use it for testing as well as for automated test eventually.
Sounds good to me - should propagate through and cause the ErrorBoundary to catch, so this would be a good case.

Maybe we should also allow users to save logs of the errors? Might be helpful for future debugging efforts, but I don't know if this would be difficult to implement in this context. Might be something for another filed bug.

Flags: needinfo?(odvarko)

One additional query:

Is it standard within DevTools to keep the implementation of the class in a given file, and then import a component from devtools/client/shared/components to bring in the presentation? I took a glance at the available components, and I don't think that there's an appropriate one we can reuse here, so I'm assuming we'll need to add some ErrorWrapper that's defined in that directory (along with its styles), unless we intend to use bare elements in the error pane itself.

(In reply to Cody Welsh from comment #5)

Maybe we should also allow users to save logs of the errors? Might be helpful for future debugging efforts, but I don't know if this would be difficult to implement in this context. Might be something for another filed bug.

Yes, this would be great. The user should be able to copy anything that could help us to solve/debug the issue: errors, stack trace, etc.

And, yes, might be done in a follow up.

Is it standard within DevTools to keep the implementation of the class in a given file, and then import a component from devtools/client/shared/components to bring in the presentation? I took a glance at the available components, and I don't think that there's an appropriate one we can reuse here, so I'm assuming we'll need to add some ErrorWrapper that's defined in that directory (along with its styles), unless we intend to use bare elements in the error pane itself.

Yes, there is no component we could reuse (from the shared directory)

So, as I mentioned in comment #4 we should do the following:

  • Introduce a new AppErrorBoundary component - wrapping the top level App component (i.e. this component will become new root component)
  • Add it into the netmonitor/src dir (along side the App component)
  • Introduce a AppErrorBoundary.css file within netmonitor/src/assets/styles dir (we are maintaining all *.css styles in this dir atm)
  • Include the new CSS file in netmonitor/src/assets/styles/netmonitor.css

Honza

Flags: needinfo?(odvarko)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #7)

Thanks for the insightful comments! This should be all I need for now, but if I stumble at any point, I'll add more.

Just a quick note that I am actually still working on this - had a while there where I couldn't do anything; sorry for that! Response time will be significantly shorter, now. :)

Great, thanks for the update!

Honza

I seem to be having some trouble with base-loader loading the module I've created - is there anything I should look out for? (The module path and name are correct, so my mind is pointing me to maybe a Redux connection requirement I'm unaware of; I'm currently just attempting to do a simple export to render the error boundary before proceeding.)

In the App component, I've added:

loader.lazyGetter(this, "AppErrorBoundary", function() {
  return createFactory(
    require("devtools/client/netmonitor/src/components/AppErrorBoundary")
  );
});

..and the export is just a simple module.exports = AppErrorBoundary;, which is currently just a simple class, similar to the React example (with prop type definitions).

The end result is:

console.error: (new Error("Module `resource://devtools/client/netmonitor/src/components/AppErrorBoundary.js` is not found at resource://devtools/client/netmonitor/src/components/AppErrorBoundary.js", "resource://devtools/shared/base-loader.js", 169))

In any case, I'll be continuing to plug at it. :) My next step is just connecting the export to the Redux store, and seeing what happens next.

Flags: needinfo?(odvarko)

Can you please attach a patch, so I can try it on my machine?

Btw. Have you appended the new file into the moz.build file located in the same dir?

Thanks for working on this! There is another bug where we'd benefit from having the error boundary implemented.
See Bug 1665574

Honza

Flags: needinfo?(odvarko) → needinfo?(codywelsh)
See Also: → 1665574

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #12)

Can you please attach a patch, so I can try it on my machine?

Btw. Have you appended the new file into the moz.build file located in the same dir?

Thanks for working on this! There is another bug where we'd benefit from having the error boundary implemented.
See Bug 1665574

Honza

The missing moz.build info was totally the issue; thanks! (I had no idea!) I can now continue working on the bug. :) Patch to come soon.

Flags: needinfo?(codywelsh)

Just a quick line of inquiry, which I'm sure has been asked before (I can't seem to find anything on it):

Is it currently impossible to inspect elements of DevTools itself with React DevTools inside of a remote Browser Toolbox? I can inspect the panels themselves with a Toolbox, but not with any development-specific addons - just wanted to make sure I wasn't missing anything. (I suppose one could always attempt rendering the DevTools panels as individual projects, but it seems like this is something that may already have a solution!)

Thanks!

Flags: needinfo?(odvarko)

(In reply to Cody Welsh from comment #13)

The missing moz.build info was totally the issue; thanks! (I had no idea!)

@bomsy thanks! (it was his idea, when we triaged this :-)

I can now continue working on the bug. :) Patch to come soon.

Awesome, thanks!

(In reply to Cody Welsh from comment #14)

Is it currently impossible to inspect elements of DevTools itself with React DevTools inside of a remote Browser Toolbox? I can inspect the panels themselves with a Toolbox, but not with any development-specific addons - just wanted to make sure I wasn't missing anything. (I suppose one could always attempt rendering the DevTools panels as individual projects, but it seems like this is something that may already have a solution!)

Good question, I don't know. But, you can ask in our public channel: https://chat.mozilla.org/#/room/#devtools:mozilla.org

Honza

Flags: needinfo?(odvarko)

Add a needed top-level error boundary component to the Net Monitor panel
in DevTools, to catch errors that would otherwise render the pane blank.
(incomplete; this message to be amended)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #15)

Awesome - I've pushed a WIP commit while I work out the testing and styling, but it appears to be functionally adequate.

As for testing - in what way do you believe we should progress with programmatic damage for reducers? Is it possible to instantiate a "test-only reducer" for our purposes that mirrors the structure of our actual reducer? Certainly some poking around to do here. :)

Flags: needinfo?(odvarko)

(In reply to Cody Welsh from comment #17)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #15)
As for testing - in what way do you believe we should progress with programmatic damage for reducers? Is it possible to instantiate a "test-only reducer" for our purposes that mirrors the structure of our actual reducer? Certainly some poking around to do here. :)

See my comment #4:

What about programmatically damage data in a reducer e.g. set requests in the requests reducer to null and reender, this should cause an
exception - and make the entire panel blank. You can use it for testing as well as for automated test eventually.¨

I recall that it worked for me when I tried it. The test should directly access the reducer and set store.requests.requests to null to damage it.

Honza

Flags: needinfo?(odvarko)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #18)

I recall that it worked for me when I tried it. The test should directly access the reducer and set store.requests.requests to null to damage it.

Okay, cool. It hadn't actually occurred to me that I could just modify the store like that, but definitely makes sense for this context. Haha. Will try that and update the differential as soon as I can.

Other considerations: As per the React docs, if we get this to a good state, we may even be able to consider shared top-level error boundaries for all other panels, but that's something for down the line. It shouldn't be too difficult to abstract this, though.

Flags: needinfo?(odvarko)

(In reply to Cody Welsh from comment #19)

Other considerations: As per the React docs, if we get this to a good state, we may even be able to consider shared top-level error boundaries for all other panels, but that's something for down the line. It shouldn't be too difficult to abstract this, though.

Yes, I agree, but let's do this as part of a follow up bug.

Flags: needinfo?(odvarko)
See Also: → 1667771

In trying to write a test that works for this, it seems like I can't get the pane to rerender after setting store.requests.requests to null. I may be missing something - do I need to dispatch a special action with a mock reducer, here? I assume that the update logic is contained within that, but it sounds like you arrived at another solution, so I wanted to make sure. (Apologies, I'm probably glossing over something obvious!)

Flags: needinfo?(odvarko)

(In reply to Cody Welsh from comment #21)

In trying to write a test that works for this, it seems like I can't get the pane to rerender after setting store.requests.requests to null. I may be missing something - do I need to dispatch a special action with a mock reducer, here? I assume that the update logic is contained within that, but it sounds like you arrived at another solution, so I wanted to make sure. (Apologies, I'm probably glossing over something obvious!)

Yes, you need to fire an action. Actions should be the only way the application state is changed (except of our little hack in this test). So, you should fire an action (something innocent e.g. openNetworkAction? - opens the left sidebar, https://searchfox.org/mozilla-central/rev/4352fb7b0d17c1febff9569ed311e0e42c93093e/devtools/client/netmonitor/src/actions/ui.js#55) to change the state and cause React to rerender.

Flags: needinfo?(odvarko)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #22)

Yes, you need to fire an action. Actions should be the only way the application state is changed (except of our little hack in this test). So, you should fire an action (something innocent e.g. openNetworkAction? - opens the left sidebar, https://searchfox.org/mozilla-central/rev/4352fb7b0d17c1febff9569ed311e0e42c93093e/devtools/client/netmonitor/src/actions/ui.js#55) to change the state and cause React to rerender.

Okay, that definitely causes the pane to rerender, but now it looks like simply setting store.requests.requests = null isn't sufficient. From some early searching, it looks like I can't modify the store in this context (I believe this is intentional, granted, but makes testing this potentially a little more annoying).

Is there anything in particular you've done to allow that operation? The only way I can see moving forward is to use store.replaceReducer if we want to damage the store, but am happy to hear any other options.

Flags: needinfo?(odvarko)

(In reply to Cody Welsh from comment #23)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #22)

Is there anything in particular you've done to allow that operation? The only way I can see moving forward is to use store.replaceReducer if we want to damage the store, but am happy to hear any other options.

I like the idea with store.replaceReducer it feels less hacky. Definitely worth of trying.

Honza

Flags: needinfo?(odvarko)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #24)

(In reply to Cody Welsh from comment #23)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #22)

Is there anything in particular you've done to allow that operation? The only way I can see moving forward is to use store.replaceReducer if we want to damage the store, but am happy to hear any other options.

I like the idea with store.replaceReducer it feels less hacky. Definitely worth of trying.

Honza

Well, it looks like this function replaces every reducer with the one passed in. I would normally expect this to work, but the problem is that the panel freezes up instead of erroring out - imagine this is because it's not getting the data it expects for each action (such as recording pause/unpause, etc), so nothing happens. I guess you'd have to pass in a reducer that is identical to the one passed in, except for one specific portion, but that sounds unwieldy (and duplicates a lot of code).

My next approach might be to see if we can manually cause one of the children components to error out. Perhaps instantiate a version of the monitor that will error out? Definitely looking into more options.

Flags: needinfo?(odvarko)

Additional info: This question makes me think that maybe we could reduce the test size to just rendering the monitor and passing ...{ hasError: true, errorMsg: "..." } or similar to AppErrorBoundary, thus forcing it to think that there is an error. Hmm..

Thanks for working on this!

The following test works for me:

add_task(async function() {
  const { tab, monitor } = await initNetMonitor(API_CALLS_URL, {
    requestCount: 1,
  });
  info("Starting test... ");

  const { document, store, windowRequire } = monitor.panelWin;
  const Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
  const { getDisplayedRequests, getSortedRequests } = windowRequire(
    "devtools/client/netmonitor/src/selectors/index"
  );

  store.dispatch(Actions.batchEnable(false));

  const state = store.getState();
  state.ui.columns = null;

  tab.linkedBrowser.reload();

  // TODO: verify the UI

  await teardown(monitor);
});

See also my review comments in Phab
https://phabricator.services.mozilla.com/D91511

Honza

Flags: needinfo?(odvarko)

Awesome! I'll address everything else after verifying the test works (and adding the UI state test at the end). Thanks for the help with this.

(This should be pretty much ready for review.) I styled it as best I could according to some of the recommendations in the Photon style guide, but I'm sure that it could be polished more. Will keep an eye on things. :)

Flags: needinfo?(odvarko)
Attached image image.png —

Victoria, this bug is introducing a new UI that informs the user about an error happening in the Network panel (it should later be applied on other panels).

If there is an error today there is just an empty screen, which results in bug reports like "The panel went blank ...".

We want to show some useful information if an error happens (a stack trace + error message) and encourage users to file a bug (by providing a link to bugzilla) and copying the data to the bug report.

What do you think about the UI (I am attaching a screenshot of the current state)

I think there could also be a button for copying the error data into the clipboard...

Honza

Flags: needinfo?(odvarko) → needinfo?(victoria)

Great to see this work happening!

Some thoughts:

  • The red is a bit hard to read, so I'd use the "Console Error Red" variable which is much darker. Alternatively, it would look less alarming if we make the text black, and add an error icon next to the error heading.
  • Instead of the two boxes for the error, what about just one that contains both the error title and error text?
  • We could shorten the top heading to just "The Network panel has crashed" (I think this is similar to the language that Firefox uses when a tab crashes. No reason to hide that it's a crash, I think :))
  • I'd replace the file a bug section with just a single button in a paragraph entitled "File Bug Report". (We could potentially pre-fill the bugzilla textarea with their error data, if that isn't too intrusive. If we can't do that, then clibpboard button makes sense.) Could use the blue button from Perf panel.
  • Could add a second button in gray that says "Reload the Page", because I think that's the next step, correct? It could be helpful in case the user doesn't immediately know to do that and feels stuck.
Flags: needinfo?(victoria)

Great! I'll go ahead and implement the aforementioned suggestions, and possibly provide another screenshot of the resulting interface. Thanks.

Couple comments I might need to explore more:

I'd replace the file a bug section with just a single button in a paragraph entitled "File Bug Report". (We could potentially pre-fill the bugzilla textarea with their error data, if that isn't too intrusive. If we can't do that, then clibpboard button makes sense.)
I'm not sure that this is possible to do on Bugzilla, but someone else might know more. The clipboard button certainly is, of course. :)

Could add a second button in gray that says "Reload the Page", because I think that's the next step, correct? It could be helpful in case the user doesn't immediately know to do that and feels stuck.
Honza, is this possible inside of the monitor? We would have to close and reload the toolbox itself, I think.

Otherwise, everything else is done locally; just need to figure out these two points.

Flags: needinfo?(victoria)
Flags: needinfo?(odvarko)

(Oops, can't edit my comment, but the last lines in the quotes are my questions)

(In reply to Cody Welsh from comment #33)

Couple comments I might need to explore more:

I'd replace the file a bug section with just a single button in a paragraph entitled "File Bug Report". (We could potentially pre-fill the bugzilla textarea with their error data, if that isn't too intrusive. If we can't do that, then clibpboard button makes sense.)
I'm not sure that this is possible to do on Bugzilla, but someone else might know more. The clipboard button certainly is, of course. :)

You can provide default description through comment field as follows:
https://bugzilla.mozilla.org/enter_bug.cgi?product=DevTools&component=Netmonitor&comment=some%20text

Could add a second button in gray that says "Reload the Page", because I think that's the next step, correct? It could be helpful in case the user doesn't immediately know to do that and feels stuck.
Honza, is this possible inside of the monitor? We would have to close and reload the toolbox itself, I think.

Otherwise, everything else is done locally; just need to figure out these two points.

We can do the same as the "Reload" button displayed when the Network panel is empty.
See this code
https://searchfox.org/mozilla-central/rev/e0eb861a187f0bb6d994228f2e0e49b2c9ee455e/devtools/client/netmonitor/src/components/request-list/RequestListEmptyNotice.js#90

Thanks this will be great feature!

Honza

Flags: needinfo?(odvarko)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #35)

We can do the same as the "Reload" button displayed when the Network panel is empty.
See this code
https://searchfox.org/mozilla-central/rev/e0eb861a187f0bb6d994228f2e0e49b2c9ee455e/devtools/client/netmonitor/src/components/request-list/RequestListEmptyNotice.js#90

Thanks this will be great feature!

Honza

This works, but only updates the browser page. Is that what we're going for, here? (I'd assumed the goal was to reload the network monitor, but if not, my bad!)

Flags: needinfo?(odvarko)

(Also, I think that the only way to resolve the error boundary "normally" is to cause a total remount of the component, hence the need to close/reopen the toolbox. Not sure if there's another way to invoke that.)

Okay, did some research on this.

Firstly, this comment thread suggests that we can set the key of AppErrorBoundary upon clicking the "reload" button, and alternatively call this.forceUpdate(), though the first solution is preferred. There is also a decent example on this.

If we decide to go this route, we'd have to create an additional component to hold the buttons, and update the key from there. Might take some reshuffling around of a bunch of the logic.

Thanks for the update!

(In reply to Cody Welsh from comment #38)

Firstly, this comment thread suggests that we can set the key of AppErrorBoundary upon clicking the "reload" button,

Sounds good to me.

If we decide to go this route, we'd have to create an additional component to hold the buttons, and update the key from there. Might take some reshuffling around of a bunch of the logic.

ok

Also note that the content of the Network panel is rendered within an iframe, so re-rendering just that could be enough.

Honza

Flags: needinfo?(odvarko)

Well, looks like simply remounting AppErrorBoundary is insufficient (it simply re-renders, errors out again, and we're back at the same view. It's hard to say exactly why, but I'd guess it's because the store doesn't change in the interim.).

Past that, since the rendering of the Network Panel essentially happens in the src/app.js file within the bootstrap process, I'm having a bit of trouble working out how we should approach re-rendering the iframe there from our reset button. (I figure we might somehow pass a local method down, but I'm not sure.) Will be continuing to try anything I can think of, though.

Flags: needinfo?(victoria) → needinfo?(odvarko)

Julian, do we have some API that allows to re-open the Toolbox?
It would help to recover from unexpected error in this case.

Honza

Flags: needinfo?(odvarko) → needinfo?(jdescottes)

The only thing I can think about is our logic to handle a localTab remoteness change, ie what we used to do before we had target switching:

    await toolbox.destroy();

    // Fetch the new target for this tab
    const newTarget = await TargetFactory.forTab(this.localTab, null);

    gDevTools.showToolbox(newTarget);

https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/devtools/client/fronts/descriptors/tab.js#281-289

So 3 steps: close the toolbox, recreate the target, reopen the toolbox. The challenge is to be able to recreate the correct target. It's easy for local tabs, because the TabDescriptorFront keeps a pointer on the actual XULTab of the target and can easily recreate a new target. I would suggest exposing a new method on TabDescriptorFront, called something like reloadLocalTabToolbox (I suggest this front, because that's the only place that should handle specifics about local tabs).

An easier option would be to tell the user to close and reopen the toolbox?

Flags: needinfo?(jdescottes)

Thanks for quick response Julian!

(In reply to Julian Descottes [:jdescottes] from comment #42)

An easier option would be to tell the user to close and reopen the toolbox?

Yes, sounds like the option for now. We can automated that in a follow up if needed.

@Cody - let's just update the description and instruct the user to reopen the Toolbox to workaround the issue.

Honza

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #43)

@Cody - let's just update the description and instruct the user to reopen the Toolbox to workaround the issue.

Sounds good; it would certainly be easier to do this for now. Will go ahead and implement it. :)

Diff should be updated; will keep an eye out for any other change requests!

Flags: needinfo?(odvarko)

Just commented on Phab.

Flags: needinfo?(odvarko)
Pushed by hmanilla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5129f2522092
Introduce top level Error boundary Component. r=Honza,bomsy

https://hg.mozilla.org/mozilla-central/rev/5129f2522092

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox84: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Depends on: 1674607
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: