1275559 - RDM doesn't handle properly empty device list

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We should definitely fix this, it would be sad for the tool to require access to the CDN like this.

We could add a catch like you say, but that sounds like it would mean waiting until the request fails / times out before we can open RDM, is that correct?

I think we should instead stop yielding on initDevices[1] if we can.  (Actually maybe we still want the catch you are saying, so the initDevices code can complete at least.)  Then the devices can populate in the background without blocking RDM.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/index.js#46

Comment 1

[Benoit Chabod [:bigben]]

I agree, we should stop yielding on initDevices so that RDM can be rendered as soon as possible, even if the request times out and there is no storage cache available.

Perhaps the device list could contain a message like "Fetching devices...", that can either disappear to let the device list appear or turn into "Can't fetch devices, please make sure you are connected to internet". Does that seem right to you?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, that sounds reasonable to me.

Comment 3

[Benoit Chabod [:bigben]]

Thanks to Bug 1241714 we have a device selector now, with presets for the HTML RDM. The device list is recovered from the CDN and cached in preferences, although it should be stored in IDB storage once Bug 1254392 has been solved.

However, if you start firefox for the first time (or if you cleared the corresponding preference) and if the CDN is unreachable (e.g internet problem), the RDM won't open because it doesn't have any device list available. In fact, there is even an uncaught exception!

[Steps to reproduce]:
1. Turn off internet.
2. Launch Firefox.
3. Please make sure this is your first Firefox launch, or clear the devtools.devices.url_cache preference from about:config
4. Enable the devtools.responsive.html.enabled pref.
5. Try to open RDM.

[Expected result]:
- The RDM opens and the device list is empty, perhaps with a helpful message

[Actual result]:
- The RDM won't open and an uncaught exception is logged in the browser console

Comment 4

[Benoit Chabod [:bigben]]

I can solve this bug if no one else is interested to take it :)
The problem lies in GetDevices [1], which doesn't handle eventual promise rejection from getJSON, and the function that calls GetDevices [2]. I think we should add a .catch block and make sure an empty device list doesn't prevent the RDM from launching.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/devices.js#54
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/devices.js#22

Comment 5

[Benoit Chabod [:bigben]]

Attachment: 1st patch - handle asynchronous loading and errors for device list

Here's a first patch attempt to solve this bug!
The device list is now loaded asynchronously, and the RDM view can launch without waiting upon it.
When the device list isn't loaded yet, the device selector just contains a "Loading..." placeholder and the DOM element is disabled.
Then, depending on the device API response, the placeholder can become "No list available" (in case of error) or can disappear to let the device list pop up.

To do so, I added a listState string property in the React store.
I don't know if RDM supports l10n yet, but there are a few new strings that should be localized.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8758332 [details] [diff] [review]
1st patch - handle asynchronous loading and errors for device list

Review of attachment 8758332 [details] [diff] [review]:
-----------------------------------------------------------------

The idea is reasonable overall, but let's use the async action creator pattern which I mention in more detail below.

::: devtools/client/responsive.html/actions/devices.js
@@ +37,5 @@
>        displayed,
>      };
>    },
>  
> +  updateDeviceListState(newState) {

I think we want to use the async Redux action creator pattern[1] here.

An existing example of this is in actions/screenshot.js.  The summary is the action creator can use a generator so that it can yield on promises on other async work.  The action create might be called something like `loadDeviceList`, and it would dispatch _multiple_ actions to signal the loading starts, ends, errors, etc.

The `loadDeviceList` action creator would then become the thing that actually call the devices module to load devices, and that would be removed from index.js.  index.js would probably be the place to dispatch this new `loadDeviceList` action.

[1]: http://redux.js.org/docs/advanced/AsyncActions.html

Comment 7

[Benoit Chabod [:bigben]]

Attachment: 2nd patch - handle asynchronous loading and errors for device list

Okay, here's a second patch that complies with the async redux action creator pattern!
You're right, that's exactly what I tried to do in a "custom" way.

This patch adds dispatched actions to signal the device list loading progress, such as LOAD_DEVICE_START.
I've kept the listState string attribute in the store, since I find it easy to manage both in the backend and for display purposes.

As you suggested, the real routine that loads the list is now inside actions/devices.js, and index.js just dispatches that new action.
To avoid any circular requirement, I had to move most of the code from ./devices.js->initDevices() to ./actions/devices->loadDevices().
But ./devices.js still contains the methods needed to get and set the user preferences regarding which devices should be displayed in the list.

I also used this opportunity to fix a small problem in getjson: asyncStorage doesn't throw any error when the item isn't found, so I added a special case.
What do you think?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I'm hoping to review this soon, but I've been a bit busy with prep for London and other things.  Sorry for the delay!

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8760745 [details] [diff] [review]
2nd patch - handle asynchronous loading and errors for device list

Review of attachment 8760745 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this is looking good so far!

I think we'll want some test coverage of this one way or another.  Either extend an existing device list to check the new states or add a new test.

::: devtools/client/responsive.html/actions/devices.js
@@ +57,5 @@
> +        return;
> +      }
> +
> +      for (let type of devices.TYPES) {
> +        dispatch({

It's fine to keep calling `addDeviceType` like before.

@@ +77,5 @@
> +          if (newDevice.displayed) {
> +            deviceList.add(newDevice.name);
> +          }
> +
> +          dispatch({

Same here, use `addDevice`.

::: devtools/client/responsive.html/actions/index.js
@@ +43,5 @@
>  
> +  // Indicates that the device list is being loaded
> +  "LOAD_DEVICE_LIST_START",
> +
> +  // Indicates that the device list loading action throwed an error

Nit: throwed -> threw

@@ +46,5 @@
> +
> +  // Indicates that the device list loading action throwed an error
> +  "LOAD_DEVICE_LIST_ERROR",
> +
> +  // Update the device list has been loaded successfully

Nit: Indicates that the...

::: devtools/client/responsive.html/devices.js
@@ +43,1 @@
>  exports.loadDeviceList = loadDeviceList;

Since these two are only now used from `actions/devices`, let's move them to that module and remove this module.  (There are a few usages from tests, but those can be updated to the new location.)

However I guess that is a bit unusual since you would have to export them from the actions module, but they are not actions...  You could export them with a _ prefix to suggest internal for testing?  Not sure what's best.

::: devtools/client/responsive.html/index.js
@@ +42,5 @@
>                "resource://devtools/client/responsive.html/responsive-ua.css",
>                "agent");
>      this.telemetry.toolOpened("responsive");
>      let store = this.store = Store();
> +    this.dispatch(loadDevices());

Since we're letting this proceed in the background (no yield), we need to be careful in tests that rely on having the device list available.

Can you check through the tests for the device list?  We may need to waitUntilState() for the loading state if the access the device list from the store.

::: devtools/client/responsive.html/reducers/devices.js
@@ +16,5 @@
>  
>  const INITIAL_DEVICES = {
>    types: [],
>    isModalOpen: false,
> +  listState: "LOADING",

It seems like the initial state should be something else...  It should not be LOADING until the start action.  Maybe you need a fourth "INITIALIZED" state or something?

@@ +56,5 @@
> +  },
> +
> +  [LOAD_DEVICE_LIST_ERROR](devices, action) {
> +    return Object.assign({}, devices, {
> +      listState: "NA",

Let's use the state value "ERROR" for this one.

@@ +62,5 @@
> +  },
> +
> +  [LOAD_DEVICE_LIST_END](devices, action) {
> +    return Object.assign({}, devices, {
> +      listState: "READY",

Let's use the state value "LOADED" for this one.

::: devtools/client/responsive.html/types.js
@@ +69,5 @@
>    // Whether or not the device modal is open
>    isModalOpen: PropTypes.bool,
>  
> +  // Device list state, i.e. "LOADING", "READY" or "NA"
> +  listState: PropTypes.string,

Use a string to represent the different states seems good, but we should lock down the raw strings and export them as an enum-like thing for use elsewhere.

I would say:

1. Move createEnum from actions/index to utils/enum
2. In this file, do something like:

createEnum([
  "A",
  "B", ...
], exports.deviceListState);

Other modules that use the state values would use this new enum instead.

For `listState` above, you could use PropTypes.oneOf[1].

[1]: https://facebook.github.io/react/docs/reusable-components.html

::: devtools/client/shared/devices.js
@@ +59,5 @@
>        }
>        devices[type] = localDevices[type].concat(devices[type]);
>      }
>      deferred.resolve(devices);
> +  }).catch(e => {

I think a simpler approach is to change this function to:

function GetDevices() {
  return getJSON(DEVICES_URL).then(devices => {
    ... process devices as before ...
    return devices;
  });
}

Then the wrapping deferred is not needed and the caller can catch errors.

::: devtools/client/shared/getjson.js
@@ +41,5 @@
>  
>    function readFromStorage(networkError) {
>      asyncStorage.getItem(prefName + "_cache").then(function (json) {
> +      if (!json) {
> +        return Promise.reject("Empty cache");

Use `promise` not `Promise`.  Maybe cite the prefName in the error?

Comment 10

[Benoit Chabod [:bigben]]

Attachment: 3rd patch - handle asynchronous loading and errors for device list

Okay, I've made many changes to my previous patch to comply with your requests, and this is the new version!
Here are the main differences:

- I've added a new test, browser_device_modal_error, that checks what happen when the device list can't be loaded.
In the other tests using the device list, I've changed the waitUntilState call to avoid race conditions as the device list is now loaded asynchronously.

- The raw strings for the device list state are declared in the types module, using the createEnum function (moved to utils/enum.js).
There is now a fourth state, called INITIALIZED.

- loadDeviceList and updateDeviceList from the old devices module have been moved to the actions module, and I renamed them loadDisplayedList and updateDisplayedList.
As you suggested, I added a _ prefix to export loadDisplayedList for the tests. I think this is a good idea. However, updateDisplayedList is used in the app.js file, so I exported this just like a normal action.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8763600 [details] [diff] [review]
3rd patch - handle asynchronous loading and errors for device list

Review of attachment 8763600 [details] [diff] [review]:
-----------------------------------------------------------------

Great work overall, it works quite nicely in my testing.

A few small things to cleanup, but no need for another review.

::: devtools/client/responsive.html/test/browser/browser_device_modal_error.js
@@ +9,5 @@
> +const Types = require("devtools/client/responsive.html/types");
> +const { getStr } = require("devtools/client/responsive.html/utils/l10n");
> +
> +// Set a wrong URL for the device list file
> +Services.prefs.setCharPref("devtools.devices.url",

Use the form:

add_task(function* () {
  yield SpecialPowers.pushPrefEnv({
    set: [["devtools.devices.url", TEST_URI_ROOT + "wrong_devices_file.json"]],
  });
});

instead, which has the advantage that it cleans up automatically, even if something goes wrong in the middle of the test (so later tests still see the expected pref value).  Then you can remove the step at the end of this test that resets the pref value.  See [1] as an example.

[1]: https://dxr.mozilla.org/mozilla-central/rev/51377a64158941f89ed73f388ae437cfa494c030/browser/modules/test/browser_CaptivePortalWatcher.js#10

::: devtools/client/responsive.html/types.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  const { PropTypes } = require("devtools/client/shared/vendor/react");
> +const { createEnum } = require("devtools/client/responsive.html/utils/enum");

Use "./utils/enum" as the path

@@ +44,5 @@
>  /**
> + * An enum containing the possible values for the device list state
> + */
> +exports.deviceListState = {};
> +createEnum([

Maybe change `createEnum` to use a default value for the target arg (`target = {}`), and then this can be:

exports.deviceListState = createEnum([...]);

which seems more natural.

Comment 12

[Benoit Chabod [:bigben]]

Attachment: Final patch - handle asynchronous loading and errors for device list

Here's a new version of the patch, that should be ready to land!
Ryan, I asked for your review again because I had a huge rebase to do when bug 1275520 has landed and I want to make sure I haven't done silly mistakes merging the code.
Thanks for add_task tip, it's nice that it cleans up automatically regardless of the test success/failure :)

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8765418 [details] [diff] [review]
Final patch - handle asynchronous loading and errors for device list

Review of attachment 8765418 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, good job handling the rebase!

Comment 14

[Benoit Chabod [:bigben]]

Attachment: Final patch v2 - handle asynchronous loading and errors for device list

Alright, this is the final patch rebased on today's master and ready to land!
There was a just an ellipsis problem so that's the only difference with the one Ryan reviewed (I used dots instead of the unicode character).

Corresponding try push with intermittent errors only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd0e4afa7e74

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c0ec1af2e8c5
Handle asynchronous loading and errors for device list, r=jryans

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0ec1af2e8c5

Comment 17

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 49.0a1 (2016-05-25) and on Windows 10 x64.
The issue is no longer reproducible on Firefox 50.0a1(2016-07-06).
Tests were performed on Windows 10 x64, Ubuntu 16.04 x64 and on Mac OS X 10.11.1.