1093093 - Log shaking notification does not explain the reason of a save failure (like when the SD card is in use), shows "[object Object]"

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Johan Lorenzo [:jlorenzo]]

Attachment: Log shake and screenshot failures - Screenshot

STR
1. Enable shake to log
2. Plug a USB cable and activate USB storage
3. Shake it up!

Expected results
Like when you try to save a screenshot when the memory card is in use, you should see the reason of the failure.

Actual
"The logs could not be saved [object Object]". See attached screenshot for details

Comment 1

[:gerard-majax]

On this specific case, we have an error coming from Gecko:
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: handling event capture-logs-error
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: handling capture logs error
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error is: object
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error: error.operation=makeDir
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error: error.path=/storage/sdcard/logs
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error: error.unixErrno=30
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error: error.stack=
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error: error.fileName=(unknown module)
> 11-04 14:59:41.208  2440  2440 I GeckoDump: LogShake: Error: error.lineNumber=0

Johann/Janx, how would you expose this kind of error in the notification ? I don't see any simple way to do so.

Comment 2

[:gerard-majax]

Errno 30 is EROFS: File system is only accessible in read-only mode. In this case, it happens  because the sdcard is mounted via UMS on the computer. Hence, /storage/sdcard/logs does not exist and /storage/sdcard/ is just a subdirectory of the / file system. This one is obviously ro mounted.

Comment 3

[:gerard-majax]

Confere comment 1 and comment 2.

Comment 4

[Jan Keromnes [:janx]]

I'd say don't provide details unless you can and they're useful info.

In the case of comment 2, maybe:

> The logs could not be saved
> Operation makeDir failed

If `error` always has an `operation` attribute it could be helpful. That or you write short messages for every Errno (e.g. 30 could be "SD card is read-only") but that sounds like a lot of work.

Comment 5

[Johan Lorenzo [:jlorenzo]]

The screenshot feature already handles this kind of cases. Is it possible to share this part of code?

Comment 6

[:gerard-majax]

Attachment: Gaia PR

WIP: First patch. That does the trick for the sdcard in UMS mode case.

Comment 7

[:gerard-majax]

This obviously lacks tests.

Comment 8

[Johan Lorenzo [:jlorenzo]]

Tested with your patch, the error message is displayed. I love the fact that you can tap the notification to get a more detailed message. That feature doesn't exist for screenshot.

Comment 9

[:gerard-majax]

Attachment: Gaia PR

Using the proper link ...

Comment 10

[:gerard-majax]

PR updated with tests: sending notification on error is covered for the new use cases. Let's add more tests for notification click handler, now.

Comment 11

[:gerard-majax]

Reworked tests and enabled the pending one for testing success notification click handler.

Comment 12

[:gerard-majax]

Added tests for all click handlers.

Comment 13

[:gerard-majax]

Comment on attachment 8516760 [details] [review]
Gaia PR

Jan, we need your stamping :)

Comment 14

[Jan Keromnes [:janx]]

Comment on attachment 8516760 [details] [review]
Gaia PR

r+ with two remarks that I'll copy below:

1)
As an afterthought, it wouldn't be too hard to show unix error names and messages for all errnos (e.g. 28 to "ENOSPC: No space left on device") see https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/errno-base.h

Not sure we want to add and re-localize all unix error messages for the sake of it, so just suggesting.

2)
In general, if you change localized strings, please also change their names, so that localizers can re-translate them. This change seems pretty subtle though, so I'll needinfo l10n before requesting you change the names.

Francesco, do localizers need a string-name change for changes that:
- don't change the string's meaning (e.g. "The logs could not be saved" to "Error while saving logs")
- change punctuation (e.g. "Capturing logs ..." to "Capturing logṣ̣…")
?

Comment 15

[:gerard-majax]

(In reply to Jan Keromnes [:janx] from comment #14)
> Comment on attachment 8516760 [details] [review]
> Gaia PR
> 
> r+ with two remarks that I'll copy below:
> 
> 1)
> As an afterthought, it wouldn't be too hard to show unix error names and
> messages for all errnos (e.g. 28 to "ENOSPC: No space left on device") see
> https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/errno-
> base.h
> 
> Not sure we want to add and re-localize all unix error messages for the sake
> of it, so just suggesting.

That's also my doubt: we could code all errno cases, but that's a lot of work, and I'm not sure it's useful. So for now, I prefer that we stick to just adding on a per-discovered basis.

> $ errno --list

Also gives a comprehensive list :)

> 
> 2)
> In general, if you change localized strings, please also change their names,
> so that localizers can re-translate them. This change seems pretty subtle
> though, so I'll needinfo l10n before requesting you change the names.
> 
> Francesco, do localizers need a string-name change for changes that:
> - don't change the string's meaning (e.g. "The logs could not be saved" to
> "Error while saving logs")
> - change punctuation (e.g. "Capturing logs ..." to "Capturing logṣ̣…")
> ?

Forgot this, I'll wait feedback from l10n people.

Comment 16

[Francesco Lodolo [:flod]]

It's on the edge, but in this specific case I'd change the IDs to make sure that localizers check their existing strings (also noted a typo on "occured").

> logsOperationFailed = Operation {{operation}} failed.
So, what are the possible values of {{operation}}? EPERM, ESRCH, etc.?

Are these strings going to be displayed only in the log or exposed to users?

Comment 17

[:gerard-majax]

(In reply to Francesco Lodolo [:flod] from comment #16)
> It's on the edge, but in this specific case I'd change the IDs to make sure
> that localizers check their existing strings (also noted a typo on
> "occured").
> 
> > logsOperationFailed = Operation {{operation}} failed.
> So, what are the possible values of {{operation}}? EPERM, ESRCH, etc.?

No, that's coming from the error object itself. One example is the one exposed in the test: mkdir.

> 
> Are these strings going to be displayed only in the log or exposed to users?

For now it's exposed in the notification. Since logshake is mostly useful for developpers, I thought it would be good to provide a bit more info/context.

Comment 18

[:gerard-majax]

Comment on attachment 8516760 [details] [review]
Gaia PR

Jan, Francesco, feedback? to assert you agree with the nits fixes.

Comment 19

[Francesco Lodolo [:flod]]

Comment on attachment 8516760 [details] [review]
Gaia PR

Looks good to me l10n-wise

Comment 20

[Jan Keromnes [:janx]]

Comment on attachment 8516760 [details] [review]
Gaia PR

Fixes look good to me, but two remarks about your test:

From your quote in comment 1, error.operation is actually "makeDir", not "mkdir".

Why use 11 EAGAIN "Try again" as generic error, wouldn't errno 0 have done the job here? I ask because if we support 11 one day, it won't be the generic error anymore.

Comment 21

[:gerard-majax]

No, you're right.

Comment 22

[:gerard-majax]

Addressed.
Previous try was green at https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=22df2f7343da, let's wait for the latest push to be.

Comment 23

[:gerard-majax]

All green: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9a5e5877c8b0

Comment 24

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/688aa8c2f9aa6a96bde6845ea8a017d044aedc83