1535572 - [remote-dbg-next] Discussion: this-firefox - page vs runtimeId

Status: NEW

(DevTools :: about:debugging, enhancement, P3)

Description

[Ola Gasidlo [:ogasidlo] [:ola] OOO - please ping :ladybenko:]

We decided to change the order of pages in the sidebar and with this the default page (no runtime, initial load etc.). to connect. This might have surfaced an issue I'd like to discuss.

As this-firefox is our default page right now, we use to check for RUNTIMES.THIS_FIREFOX. But often I feel we mix the two use cases this-firefox stands for as it can be either the runtimeId OR the page.

I feel like we already ran into this here: https://bugzilla.mozilla.org/show_bug.cgi?id=1535470 as e.g. here https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging-new/src/actions/debug-targets.js#160 we are assuming we always have a runtime, which we had while being on the this-firefox page. Now on the connect page we do not have one anymore.

My suggestion would be to keep https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging-new/src/constants.js#111 and add a page constant for this-firefox here https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging-new/src/constants.js#87 like following

const PAGE_TYPES = {
  RUNTIME: "runtime",
  THIS_FIREFOX: "runtime/this-firefox",
  CONNECT: "connect",
};

and go from there.

What do y'all think?

Comment 1

[Julian Descottes [:jdescottes]]

Ola and I paired on this today, here is a summary of the investigation.

The goal was to update the fallback route when navigating to a runtime from "this-firefox" to "setup". So in App.js we changed:

      } else {
        // Also redirect to "This Firefox" if runtime is not found
        return Redirect({ to: `/runtime/${RUNTIMES.THIS_FIREFOX}` });
      }

to

      } else {
        // Redirect to "Setup" if runtime is not found
        return Redirect({ to: `/setup` });
      }

https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/devtools/client/aboutdebugging-new/src/components/App.js

After this change, navigating to "about:debugging#/runtime/bad-runtime-name" would successfully lead to the Setup page.
However, the disconnect feature was not working as expected. When clicking on disconnect, it will redirect to the Setup page and the logs will be full of failures: [ACTION FAILED] REQUEST_TABS_FAILURE, [ACTION FAILED] REQUEST_EXTENSIONS_FAILURE, etc...

Looking at the "disconnectRuntime" action, we can see a hardcoded redirect to "This Firefox":

      if (shouldRedirect) {
        await dispatch(Actions.selectPage(PAGE_TYPES.RUNTIME, RUNTIMES.THIS_FIREFOX));
      }

https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/devtools/client/aboutdebugging-new/src/actions/runtimes.js#155

Let's ignore for now the fact that this redirect is inconsistent with what we just updated in App.js and let's try to understand what happens. First we try to navigate to This-Firefox, and if we look at the ui::selectPage action we see that selectPage normally waits watchRuntime correctly. watchRuntime is the action that should retrieve the debug targets, so it's important that we wait on it.

      // Start watching current runtime, if moving to a RUNTIME page.
      if (page === PAGE_TYPES.RUNTIME) {
        await dispatch(Actions.watchRuntime(runtimeId));
      }

But (!) watchRuntime is not a good citizen here because it will retrieve the debug-targets after firing its "SUCCESS" action and after resuming (we have Bug 1494428 about this). This means that even if we wait for watchRuntime, we are not necessarily done with retrieving debug-targets. This opens the door to race conditions. To confirm this, we added a timeout after the call to selectPage:

      if (shouldRedirect) {
        await dispatch(Actions.selectPage(PAGE_TYPES.RUNTIME, RUNTIMES.THIS_FIREFOX));
        await new Promise(r => setTimeout(r, 2000));
      }

As expected, thanks to the timeout, the errors are gone.

This still leaves us with one unexplained behavior so far. Why, with this code, are we still redirecting to the Setup page? The method renderRuntime from App.js should redirect to the Setup page only if we are trying to navigate to an "invalid" runtime.

So we added a breakpoint in App.js return Redirect({ to:/setup});. When we break here after clicking on the Disconnect button, even though we seem to be on the "This Nightly" page, the router props are still assigned to the runtime that we disconnected (which are therefore invalid now). We can also see that the URL has not been updated to about:debugging#/runtime/this-firefox.

We haven't figured out this last part yet and it would be good to clarify it in order to complete the investigation.

However the correct approach here if we want to update the default page to be Setup page is to update both "App.js" and "disconnectRuntime". They should both navigate to "Setup", rather than having competing navigations. If we do this, there is only one navigation happening, without any error.

As discussed, this is also not an urgent bug to fix, since our routes are pretty well tested. It would be nice to have better grasp of how this router is working. But not an emergency.

Comment 2

[Julian Descottes [:jdescottes]]

Ok I think the root cause of the last issue is that using "selectPage" is not updating the hash at all. All navigation should be done by updating the URL, which will then in turn call selectPage() in componentWillMount().

Let's check with Belén what is the recommended pattern to navigate to another page programmatically from an action. Calling selectPage doesn't work because it doesn't update the URL. Should we use Redirect()? Should we directly modify the hash?

Comment 3

[Belén [:ladybenko]]

Sorry, forgot to update this. Either changing location or using Redirect should work (using Redirect would be more React-ish). However, since componentWillMount is deprecated, we might want to re-think the whole flow if we upgrade React further.