1057965 - [Devices][Storage][Helix] SD card section does not show on "Media storage" after detected unknown SD card

Status: VERIFIED FIXED

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

Description

[Alison Shiue]

Gaia      e424c85eda87a40c0fa64d6a779c3fa368bf770b
Gecko     https://hg.mozilla.org/mozilla-central/rev/daa84204a11a
BuildID   20140824160205
Version   34.0a1

STR:
1. Insert an unknown SD card 
2. Tap the notification to proceed to "Settings" -> "Media storage"
3. Check SD card section

Expect result:
"SD Card Storage" section has "Format SD card" button

Actual result: 
No "SD Card Storage" section exist

Comment 1

[Alison Shiue]

[Blocking Requested - why for this release]:

Comment 2

[howie [:howie]]

Triage: blocking.

Comment 3

[No-Jun Park [:njpark]]

I think Bug 1058687 is related to this issue

Comment 4

[No-Jun Park [:njpark]]

I made a comment in Bug 1058687 that this bug happens only in JB base image, so Tony and I think it should not be a priority blocker.  ni?ing Jason for assessment.

Comment 5

[Jason Smith [:jsmith]]

(In reply to No-Jun Park [:njpark] from comment #4)
> I made a comment in Bug 1058687 that this bug happens only in JB base image,
> so Tony and I think it should not be a priority blocker.  ni?ing Jason for
> assessment.

I don't think we should consider going down this path until we officially move mainline testing over to Kitkat. Otherwise, we'll lock us into situations without an ability to test certain functionality in mainline testing.

Comment 6

[[e-mail], [e-mail]]

Per offline discussion with Arthur and Alive, we'll have workaround patch for this use case. The proposal is as following. If System::ExternalStorageMonitor detected storage action 'unrecognised', it will write a settings key 'volume.external.unrecognised' to be true. Settings::MediaStorage will observe the key to enable 'Format SD Card' button. 

Somehow it's not a good way to use mozSettings to pass the event from system app to settings app. So we will request platform support to pass volume storage activity instead of storage status(bug 1060252). And system app will not maintain a state machine after device start-up. That will make system app light in the future.

Comment 7

[Jason Smith [:jsmith]]

QA Wanted for branch checks.

Comment 8

[[e-mail], [e-mail]]

Attachment: pull request 23713

Alive and Arthur, 

Per offline discussion, Gaia will support the functionality before Gecko is ready. It will provide format button while inserting an unrecognised storage. Could you please help to review the pr? Thanks.

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8484078 [details] [review]
pull request 23713

Had some tests and it worked well. r=me for the settings part with the comments addressed. Thanks!

Comment 10

[[e-mail], [e-mail]]

Thanks for Arthur's reviewing effort. I have removed typo nits and added unit test in part of system app.

Comment 11

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue DOES occur on 2.2 Flame, 2.1 Flame, 2.0 Flame, 1.4 Flame, and 2.2 OpenC.

Environmental Variables:
Device: Flame 2.2
BuildID: 20140905144737
Gaia: 0de5fcdc11a15abdf8d64f28bed2abb30041ea4d
Gecko: 25efb1b16a5e
Version: 35.0a1 (2.2) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20140905134639
Gaia: abec74ccc792617e0d2fa5fa9490810555327003
Gecko: e74b772d7ac9
Version: 34.0a2 (2.1) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Flame 2.0
BuildID: 20140905133739
Gaia: f02a8e4ef0fabeb28593fa660a0a4ca8a5fe67e5
Gecko: 498e132eb800
Version: 32.0 (2.0) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Environmental Variables:
Device: Flame 1.4
BuildID: 20140903033752
Gaia: 2ee5b00bfbb8a67a967094804390b4afce8ecf54
Gecko: f5a75c0dd74e
Version: 30.0 (1.4) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Environmental Variables:
Device: Open_C 2.2
BuildID: 20140905090738
Gaia: 0de5fcdc11a15abdf8d64f28bed2abb30041ea4d
Gecko: 0d962e459db5
Version: 35.0a1 (2.2) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 12

[Joshua Mitchell (Inactive)]

not a regression - so the patch in the works might need an uplift to the older branches

Comment 13

[[e-mail], [e-mail]]

(In reply to Joshua Mitchell [:Joshua_M] from comment #12)
> not a regression - so the patch in the works might need an uplift to the
> older branches

Yes, it's not a regression. In branch v1.4/v2.0, we don't support SD card management in this use case. Because the patch is based on module ExternalStorageMonitor in system app. And it's needed devicesStorage API with event 'storage-state-change'. This volume status event is ready to support in v2.1. So I reset affected flag from the two branches.

Comment 14

[[e-mail], [e-mail]]

Comment on attachment 8484078 [details] [review]
pull request 23713

Since Gip test failed on try-server, I updated the patch for fixing race condition. I think the change is needed to review again from reviewer.

Arthur, could you please help to review the calling sequence in init()? Thanks.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8484078 [details] [review]
pull request 23713

Please check my comments, thanks.

Comment 16

[[e-mail], [e-mail]]

Comment on attachment 8484078 [details] [review]
pull request 23713

Arthur, I have updated the patch per your concern. Since it is more quick to get settings key via SettingsCache, I think we could avoid to put callback in updateListeners(). There will not be more flag checking in updateListeners(). After test on desktop client, it's working for me. Thanks.

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8484078 [details] [review]
pull request 23713

Although using SettingsCache solves the problem but the potential race condition still exists. I would still suggest to use the callback ensuring the calling sequence, so it won't be affected by the performance of mozSettings.

Comment 18

[[e-mail], [e-mail]]

Comment on attachment 8484078 [details] [review]
pull request 23713

Revised the patch to ensure no race condition. Execute the callback function when we get settings key from SettingsCache. Still need a review for the change.

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8484078 [details] [review]
pull request 23713

r=me, thanks!

Comment 20

[[e-mail], [e-mail]]

Thanks for Arthur's reviewing effort. Since the patch is landed, we can close the issue here.

Gaia/master:  238d437f17354fd772d5bf6b593e03166574bbcc

Comment 21

[[e-mail], [e-mail]]

Comment on attachment 8484078 [details] [review]
pull request 23713

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 1057188
[User impact] if declined: Cannot see/format an unrecognised SD card.
[Testing completed]: Try-server test passed.
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: None

Comment 22

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/f421f9f61dfd8a56c8aee3dd3432bb3a641305e6

Comment 23

[Alison Shiue]

Verified on Helix
Gaia-Rev        8061ab487d42cbc49b329fd68b9ca90e0fe477e6
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/e970bc96f8b5
Build-ID        20140925000204
Version         34.0a2

Comment 24

[Sue]

Attachment: video of issue verify

This issue has been successfully verified on Flame 2.1
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.1 versions:
Gaia-Rev        1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152
Build-ID        20141119001205
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141119.035246
FW-Date         Wed Nov 19 03:52:56 EST 2014
Bootloader      L1TC00011880