840160 - Selecting to enter edit mode on an app that is downloading will disable the ability to launch any app until the phone is restarted or upon entering edit mode on a non-downloading app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 2/11/2013
Device: Unagi

Steps:

1. Start installing a large packaged app
2. Hold one of the icons down to enter edit mode

Expected:

The icons should shake, allowing you to move around icons that are not actively being downloaded. An app that is currently not being downloaded should be able to be uninstalled.

Actual:

The icons do not shake. Additionally, any app that is currently not being downloaded can't be uninstalled.

Comment 1

[Jason Smith [:jsmith]]

This seems like a recent regression, potentially from a patch that recently landed for app install.

Comment 2

[Jason Smith [:jsmith]]

Narrowed this down - actually looks like this happens if you try to enter edit mode on an app that is downloading. When this happens, every single app can no longer be launched, including after the app finishes installing.

Comment 3

[Jason Smith [:jsmith]]

Actually, looks like a workaround does exist here - you enter edit mode on an app that isn't downloading. Still quite a bad UX, but probably won't block given that it requires a well-targeted usage scenario.

Comment 4

[Julien Wajsberg [:julienw]]

I entered edit mode on an app that isn't downloading but then I tried moving the downloading app. It kind of worked (but was jerky) but then the homescreen was broken: I couldn't swype the panel anymore.

Comment 5

[Julien Wajsberg [:julienw]]

error is :

E/GeckoConsole( 4013): [JavaScript Error: "NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMMozURLProperty.createObjectURL]" {file: "app://homescreen.gaiamobile.org/js/page.js" line: 289}]
E/GeckoConsole( 4013): [JavaScript Error: "TypeError: draggableElem.querySelector(...) is null" {file: "app://homescreen.gaiamobile.org/js/page.js" line: 476}]

I don't know why we are in a state where createObjectURL fails (this is in the middle of a quite complicated code) but maybe we should try∕catch each call to createObjectURL to recover sanely.

Comment 6

[Julien Wajsberg [:julienw]]

We're calling "displayRenderedIcon" during "onDragStart". I believe that the icon for a downloading app has no descriptor.renderedIcon and that triggers the bug. I still don't know why we don't have this.

Jason, do you have the possibility to test if the bug is here without my fix to Bug 834117 ? Otherwise I'll test this tomorrow.

Comment 7

[Julien Wajsberg [:julienw]]

The result of this bug is quite dramatic because the user has to reboot the phone. I'd love to fix that and uplift this too, that's why I nominate for leo.

Comment 8

[Julien Wajsberg [:julienw]]

WIP :
https://github.com/julienw/gaia/tree/840160-fix-drag-drop-downloading-icon

Added some tests, make the "home" button always fix panning (which at least enables the user to escape the strange state), and I have a failing test to fix the initial bug \o/.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

We definitely want this for v1.1, leaving the tef? for Mvines to decide on potential uplift to v1.0.1

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: patch v1

(see also PR https://github.com/mozilla-b2g/gaia/pull/8477)

* optimize png sizes
* add basic tests for grid
* always enable panning when the user presses the home button
* don t spin the icon while drag&dropping
* always create a renderedIcon, even in the downloading state
* don't hide the icon if it's already rendered-ready
* add the img as late as possible to the DOM
* change GridManager from const to var to be able to mock and test it
---
 apps/homescreen/js/dragdrop.js                     |    3 +
 apps/homescreen/js/grid.js                         |   59 +++++---
 apps/homescreen/js/homescreen.js                   |    5 +-
 apps/homescreen/js/page.js                         |   22 ++-
 apps/homescreen/style/grid.css                     |    7 +
 apps/homescreen/style/images/app_downloading.png   |  Bin 3518 -> 2514 bytes
 apps/homescreen/style/images/app_paused.png        |  Bin 3709 -> 2546 bytes
 apps/homescreen/style/images/default.png           |  Bin 1552 -> 1443 bytes
 .../homescreen/style/images/default_background.png |  Bin 2777 -> 2642 bytes
 apps/homescreen/style/images/loading_spinner.png   |  Bin 3400 -> 3071 bytes
 apps/homescreen/test/unit/grid_test.js             |  114 ++++++++++++++
 apps/homescreen/test/unit/mock_apps_mgmt.js        |   64 ++++++++
 apps/homescreen/test/unit/mock_configurator.js     |    5 +
 apps/homescreen/test/unit/mock_dock_manager.js     |    9 ++
 apps/homescreen/test/unit/mock_grid_manager.js     |    1 +
 apps/homescreen/test/unit/mock_home_state.js       |   11 ++
 apps/homescreen/test/unit/mock_page.js             |   23 +++
 apps/homescreen/test/unit/mock_pagination_bar.js   |    6 +
 apps/homescreen/test/unit/mock_xmlhttprequest.js   |   27 +++-
 apps/homescreen/test/unit/mocks_helper.js          |   24 ++-
 apps/homescreen/test/unit/page_test.js             |  155 ++++++++++++--------
 apps/homescreen/test/unit/setup.js                 |    5 +
 22 files changed, 436 insertions(+), 104 deletions(-)
 create mode 100644 apps/homescreen/test/unit/grid_test.js
 create mode 100644 apps/homescreen/test/unit/mock_apps_mgmt.js
 create mode 100644 apps/homescreen/test/unit/mock_configurator.js
 create mode 100644 apps/homescreen/test/unit/mock_dock_manager.js
 create mode 100644 apps/homescreen/test/unit/mock_home_state.js
 create mode 100644 apps/homescreen/test/unit/mock_page.js
 create mode 100644 apps/homescreen/test/unit/mock_pagination_bar.js
 create mode 100644 apps/homescreen/test/unit/setup.js

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 721359 [details] [diff] [review]
patch v1

Some console.log statements were left in my patch, sorry, I'll remove them tomorrow.

Comment 13

[Cristian Rodriguez (:crdlc)]

ok

Comment 14

[Cristian Rodriguez (:crdlc)]

Comment on attachment 721359 [details] [diff] [review]
patch v1

All is ok but please squash commits. Thanks for your work!

Comment 15

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/871aba5cbdb50dc48b352dbb0be5474f44e4a09c

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: patch v2

carrying r=crdlc

This is the patch that landed.

Comment 17

[Julien Wajsberg [:julienw]]

One of the tests I added here fails very occasionally, I'll have a look and see if I can fix that easily.

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  871aba5cbdb50dc48b352dbb0be5474f44e4a09c
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 19

[Julien Wajsberg [:julienw]]

v1-train: b2fd277706a9d55efd3b30e1619240bbce19a425

Comment 20

[Julien Wajsberg [:julienw]]

v1.0.1: c6bc877a8dc495a3ec4650e5cb9fd50964b2d361

Comment 21

[amiller]

TC would be OOS, no need to create TC in Moztrap.

Comment 22

[:CSchmoeckel]

While a large app is being downloaded/installed it is possible to edit the app and move/delete other (installed) apps. 
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ccec751a468e
Gaia: ee0bef61c0a25c806dd1eec5a4e047bc418a5f73

Also verified on OTA V1 build
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/d467369d1b0c
Gaia: 06e0e5ce42bdfb62bdbe38271de6b5b2d9e40e75