1163956 - [nexus-5-l] Can't perform FOTA from 2.2 to 2.2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Gerry Chang [:gchang]]

Trying to upgrade 2.2 to 2.2 using FOTA, but got errors when entering recovery mode. Which will cause me to use adb sideload to update device.
The log shows
"Rebooting into recovery to apply FOTA update: /storage/emulated/legacy/updates/fota/update.zip"

STR:

1. Build 2.2 fota image locally and put the image in local server.
2. Flash nexus-5-l from pvt
3. Change ota url in "Developer" in Settings app
4. Go to Settings > Device information 
5. Tap "Check now" button
6. Perform upgrade in notification bar

### Expected:
1. The device should be upgraded

### Actual:
1. The device failed to upgrade

### Reproduce rate
always

Comment 1

[Eric Tsai (no more review request accepted)]

Two problems to be solved:
1) Command for recovery is set: "/updates/fota/update.zip", path should be fixed
2) When using sideload, the update-binary is too old to support new function. Need to package a new one with update.zip

Comment 2

[Eric Tsai (no more review request accepted)]

For first problem, set RECOVERY_EXTERNAL_STORAGE to /sdcard in full_hammerhead.mk doesn't work on my build or google's LMY47D recovery image, file not found. TWRP will mount /sdcard and there is update.zip in /sdcard/updates/fota, need to know how android L works.

Comment 3

[Eric Tsai (no more review request accepted)]

For 1) Using stock recovery, only /cache is mounted, it doesn't match with b2g update service's download path. Per-discussed with :kli, we will modify recovery to mount external storage. Then it will work as b2g.
For 2) copy system/bin/updater to device/, add TARGET_UPDATE_BINARY to BoardConfig.mk.

Comment 4

[Eric Tsai (no more review request accepted)]

For 2) Update: our edify script generator uses set_perm and set_perm_recursive, these two functions are deprecated.

Comment 5

[:gerard-majax]

(In reply to Eric Tsai from comment #3)
> For 1) Using stock recovery, only /cache is mounted, it doesn't match with
> b2g update service's download path. Per-discussed with :kli, we will modify
> recovery to mount external storage. Then it will work as b2g.
> For 2) copy system/bin/updater to device/, add TARGET_UPDATE_BINARY to
> BoardConfig.mk.

I think you can also just make TARGET_UPDATE_BINARY to be like:
TARGET_UPDATE_BINARY := $(PRODUCT_OUT)/system/bin.updater

Gerry, we cannot debug blindly this kind of error. Next time you should include the content of /cache/recovery/last_log for example.

Comment 6

[Gerry Chang [:gchang]]

Attachment: last_log

Comment 7

[Eric Tsai (no more review request accepted)]

Summarize problems to solve:
1) Command for recovery is set: "/updates/fota/update.zip", path should be fixed
2) updater-script uses "set_perm" and "set_perm_recursive" those are deprecated and replaced by "set_metadata" and "set_metadata_recursive" in new update-binary

:gsvelto, for this bug, my proposed solution is:
1) add RECOVERY_EXTERNAL_STORAGE to device/lge/hammerhead/hammerhead.mk
2) fork bootable/recovery branch 5.1.0_r1 from CAF, patch install.c with the two functions: "set_perm" and "set_perm_recursive". Then modify manifest.xml for nexus-5-l using the new revision.

If it's ok, :kli and I will finish in this week.

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Eric Tsai from comment #7)
> 1) add RECOVERY_EXTERNAL_STORAGE to device/lge/hammerhead/hammerhead.mk

+1

> 2) fork bootable/recovery branch 5.1.0_r1 from CAF, patch install.c with the
> two functions: "set_perm" and "set_perm_recursive". Then modify manifest.xml
> for nexus-5-l using the new revision.

From what I can tell the edify script generator already supports using set_metadata and set_metadata_recursive, you have to add the 'use_set_metadata=1' flag to the META/misc_info.txt for it to work though.

Comment 9

[Eric Tsai (no more review request accepted)]

Using my proposed solution works to adopt FOTA. But after FOTA, b2g can't start. I use ./build.sh gecko-update-fota-full to generate a full fota package.
It's strange because I can push update.zip and write recovery command as FOTA does, b2g works fine.

No matter download or manually FOTA, the FOTA log is the same, with unmount data partition fail, device busy.

:kli and I did some experiments and we found two ways to recovery from the status:
1. reboot to recovery mode, use vol/power button, reboot to system, then b2g starts.
2. flash clear data or cache partition by fastboot and reboot, then b2g starts

Still trying what can help.

Comment 10

[Eric Tsai (no more review request accepted)]

Attachment: last_log FOTA success but can't start b2g

Comment 11

[Eric Tsai (no more review request accepted)]

I think :gsvelto said 'use_set_metadata=1' flag to META/misc_info.txt doesn't works for "./build.sh gecko-fota-update-full". Let updater-binary support set_perm should be better?

Comment 12

[Eric Tsai (no more review request accepted)]

More experiments on FOTA pcakage
1. Remove all mount/extract/delete command in updater-script, do FOTA->b2g can't start
2. Download and extract to update.zip, don't write command and reboot to recovery. Manually reboot to system or reboot to recovery then to system->b2g starts
3. Download and extract to update.zip, don't write any command but auto reboot to recovery->b2g starts
4. Change EXTERNAL_STORAGE to /cache, but RECOVERY_EXTERNAL_STORAGE in /data/media/0, make a fail update->b2g can't start
5. Change EXTERNAL_STORAGE and RECOVERY_EXTERNAL_STORAGE to /cache and do FOTA->b2g can't start

Comment 13

[Eric Tsai (no more review request accepted)]

Some more experiments, when b2g can't start, move /cache/recovery to /cache/recovery2, then b2g starts.
Not sure why but may be I can check bootable/recovery and see something related with /cache/recovery

Comment 14

[Eric Tsai (no more review request accepted)]

Only /cache/recovery/last_install will cause b2g can't start. After change recovery.cpp not copy /tmp/last_install to /cache/recovery/last_install, the FOTA works fine and b2g can start. I think the last_install format should be fine but need to figure out why can't start b2g.

Comment 15

[:gerard-majax]

Eric, can you be more precise for what happens when you say "b2g cannot start"?

Does the system not even boot?
Does it boots but the process /system/b2g/b2g never gets created?
Does /system/b2g/b2g gets executed but stalls at some point?

Comment 16

[Eric Tsai (no more review request accepted)]

Hi Alexandre, in my case b2g starts but crash. Some watchdog will start b2g again. I can use b2g-ps see the PID increase. Screen stay in Google logo, doesn't change to b2g animation.
When I remove (or rename) last_install, b2g starts and I can boot to home screen
I have a back trace log but I can't find the root cause.

Comment 17

[Eric Tsai (no more review request accepted)]

Attachment: back trace after FOTA success, b2g crashes

Comment 18

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1163956-Add-RECOVERY_EXTERNAL_STORAGE.patch

Fix RECOVERY_EXTERNAL_STORAGE for nexus 5

Comment 19

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1163956-Add-set_perm-for-backward-compatible.patch

Add set_perm for backward compatible for gecko-update-fota-full which will not use set_metadata

Comment 20

[Eric Tsai (no more review request accepted)]

Hi Alexandre, the two patches can helps to duplicate my current status.

Comment 21

[:gerard-majax]

That's strange, I cannot find any trace of playing with last_install in Gecko

Comment 22

[:gerard-majax]

Hm are you sure Gecko is at fault? It's only librecovery and recovery itself that manipulates this last_install file. And you did hack recovery ...

Comment 23

[:gerard-majax]

Eric, last_install is being read by librecovery in |getFotaUpdateStatus()| and this is being used by Gecko https://dxr.mozilla.org/mozilla-central/search?q=getFotaUpdateStatus&case=true

Maybe it's this code that is choking ?

Comment 24

[Eric Tsai (no more review request accepted)]

I can try this later. But in my experience, I didn't see any related log from logcat.

Comment 25

[Eric Tsai (no more review request accepted)]

Alexandre, I did some experiments and find

https://dxr.mozilla.org/mozilla-central/source/b2g/components/RecoveryService.js#145

Accessing cStatus.result cause b2g crash, any idea?

Comment 26

[:gerard-majax]

No idea, at all.

Comment 27

[:gerard-majax]

(In reply to Eric Tsai from comment #25)
> Alexandre, I did some experiments and find
> 
> https://dxr.mozilla.org/mozilla-central/source/b2g/components/
> RecoveryService.js#145
> 
> Accessing cStatus.result cause b2g crash, any idea?

Vivien suggested maybe we have simply a discrepency with the ctypes ?

Comment 28

[Eric Tsai (no more review request accepted)]

Vivien, may I know what's "simply a discrepency with the ctypes" means? I don't have any idea about this.

I print the status object/structure address in 1) RecoveryService.js, after create cStatus, 2) librecovery.c, before leave getFotaUpdateStatus and 3) RecoveryService.js, after calling getFotaUpdateStatus.
1 and 2 are the same address, but 3 is different. Is this why b2g crashes?

> I/Gecko   (  736): -*- RecoveryService: before getFotaUpdateStatus, cStatus.address: FotaUpdateStatus.ptr(ctypes.UInt64("0xb1e4ee10"))
> W/librecovery(  736): before leave getFotaUpdateStatus, status address: 0xb1e4ee10
> I/Gecko   (  736): -*- RecoveryService: after getFotaUpdateStatus, cStatus.address: FotaUpdateStatus.ptr(ctypes.UInt64("0x656d2f61"))

Comment 29

[:gerard-majax]

(In reply to Eric Tsai from comment #28)
> Vivien, may I know what's "simply a discrepency with the ctypes" means? I
> don't have any idea about this.
> 
> I print the status object/structure address in 1) RecoveryService.js, after
> create cStatus, 2) librecovery.c, before leave getFotaUpdateStatus and 3)
> RecoveryService.js, after calling getFotaUpdateStatus.
> 1 and 2 are the same address, but 3 is different. Is this why b2g crashes?
> 
> > I/Gecko   (  736): -*- RecoveryService: before getFotaUpdateStatus, cStatus.address: FotaUpdateStatus.ptr(ctypes.UInt64("0xb1e4ee10"))
> > W/librecovery(  736): before leave getFotaUpdateStatus, status address: 0xb1e4ee10
> > I/Gecko   (  736): -*- RecoveryService: after getFotaUpdateStatus, cStatus.address: FotaUpdateStatus.ptr(ctypes.UInt64("0x656d2f61"))

Eric, what Vivien said is that maybe the ctypes binding are not in sync with your current librecovery's prototype.

Comment 30

[Eric Tsai (no more review request accepted)]

Looks the same in RecoveryService.js
>  let FotaUpdateStatus = new ctypes.StructType("FotaUpdateStatus", [
>                                                { result: ctypes.int },
>                                                { updatePath: ctypes.char.ptr }
>                                              ]);

and librecovery.h
> typedef enum {
>   FOTA_UPDATE_UNKNOWN = 0,
>   FOTA_UPDATE_FAIL = 1,
>   FOTA_UPDATE_SUCCESS = 2
> } FotaUpdateResult;
> 
> typedef struct {
>   FotaUpdateResult result;
>   // The path to the update that was installed
>   char updatePath[PATH_MAX];
> } FotaUpdateStatus;

Comment 31

[:gerard-majax]

Attachment: Properly make use of librecovery FotaUpdateStatus struct r=...

Comment 32

[:gerard-majax]

Attachment: [v2.2] Properly make use of librecovery FotaUpdateStatus struct r=...

Eric, could you give a try to this patch on v2.2 branch ? I suspected maybe
there was something wrong with ctypes itself so we checked with :nbp but while
verifying he suspecting the line making use of FotaUpdateStatus struct was quite
strange, so we dig up into Gecko for other uses and it showed that all others
were declaring the struct like it is done right now, but we using it in another
way:
 - either accessing directly the variable declared as ctypes.StructType()
   without any use of '()', 'new' and playing around with '.ptr'
 - or accessing the variable declared as ctypes.StructTypes() with '()', a 'new'
   statement and using '.address()'

This patch should make us in the second case properly, by adding the 'new'
statement. I have no idea why this would not have been caught previously :(

Comment 33

[Eric Tsai (no more review request accepted)]

Alexandre, patch from attachment 8621539 [details] [diff] [review] doesn't help :( How could I try the first case?

Comment 34

[Eric Tsai (no more review request accepted)]

I run the same code on flame-2.2, cStatus.address() is the same before/after getFotaUpdateStatus. I think if we can figure out why address change on nexus 5, this bug can be solved.

Comment 35

[:gerard-majax]

Attachment: Properly make use of librecovery FotaUpdateStatus struct r=...

Comment 36

[:gerard-majax]

Attachment: Properly make use of librecovery FotaUpdateStatus struct r=...

Comment 37

[:gerard-majax]

Eric, can you try attachment 8622474 [details] [diff] [review] ?

Comment 38

[Eric Tsai (no more review request accepted)]

attachment 8622474 [details] [diff] [review] doesn't work for me and will get the following gecko error:

W/GeckoConsole(  196): [JavaScript Error: "TypeError: expected type pointer, got ctypes.StructType("FotaUpdateStatus", [{ "result": ctypes.int }, { "updatePath": ctypes.char.ptr }]).ptr" {file: "jar:file:///system/b2g/omni.ja!/components/RecoveryService.js" line: 144}]
W/GeckoConsole(  196): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "expected type pointer, got ctypes.StructType("FotaUpdateStatus", [{ "result": ctypes.int }, { "updatePath": ctypes.char.ptr }]).ptr" {file: "jar:file:///system/b2g/omni.ja!/components/RecoveryService.js" line: 146}]'[JavaScript Error: "expected type pointer, got ctypes.StructType("FotaUpdateStatus", [{ "result": ctypes.int }, { "updatePath": ctypes.char.ptr }]).ptr" {file: "jar:file:///system/b2g/omni.ja!/components/RecoveryService.js" line: 144}]' when calling method: [nsIRecoveryService::getFotaUpdateStatus]" {file: "jar:file:///system/b2g/omni.ja!/components/nsUpdateService.js" line: 1848}]
W/GeckoConsole(  196): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "expected type pointer, got ctypes.StructType("FotaUpdateStatus", [{ "result": ctypes.int }, { "updatePath": ctypes.char.ptr }]).ptr" {file: "jar:file:///system/b2g/omni.ja!/components/RecoveryService.js" line: 146}]'[JavaScript Error: "expected type pointer, got ctypes.StructType("FotaUpdateStatus", [{ "result": ctypes.int }, { "updatePath": ctypes.char.ptr }]).ptr" {file: "jar:file:///system/b2g/omni.ja!/components/RecoveryService.js" line: 144}]' when calling method: [nsIRecoveryService::getFotaUpdateStatus]" {file: "jar:file:///system/b2g/omni.ja!/components/nsUpdateService.js" line: 1848}]

Comment 39

[Eric Tsai (no more review request accepted)]

Bobby, could you help me to figure out why cStatus.address() changes after:
https://dxr.mozilla.org/mozilla-central/source/b2g/components/RecoveryService.js#144

I did some experiments and find on my Nexus 5, the status structure's address changes then cause b2g crashes. But this will not happen on flame. RecoveryService.js and librecovery.c are the same between nexus 5 and flame's code base.

Comment 40

[Bobby Holley (:bholley)]

cStatus is |new ctypes.StructType("FotaUpdateStatus", ...)| right? That means that it represents a type, not data. You can't take the address of a type. ;-)

You should really rename cStatus to "cStatus_t", and then do "cStatus = new cStatus_t(...)", and take .address() of that.

Comment 41

[Eric Tsai (no more review request accepted)]

Boddy, I think FotaUpdateStatus represents a type now, from:
https://dxr.mozilla.org/mozilla-central/source/b2g/components/RecoveryService.js#29
I guess Alexandre's attachment 8621539 [details] [diff] [review] works the same as you said? I've tried this but still get two different addresses, b2g crashes when access cStatus.result.

Comment 42

[Eric Tsai (no more review request accepted)]

Looks like the RecoveryService.js jsctype structure doesn't match with librecovery stucture. I fix updatePath with:
 let FotaUpdateStatus = new ctypes.StructType("FotaUpdateStatus", [
                                                { result: ctypes.int },
                                                { updatePath: ctypes.char.array(4096) }
                                              ]);

then everything works good now.

Comment 43

[Eric Tsai (no more review request accepted)]

Attachment: Add RECOVERY_EXTERNAL_STORAGE

Comment 44

[Eric Tsai (no more review request accepted)]

Attachment: Support USE_SET_METADATA for build fota package

Comment 45

[Eric Tsai (no more review request accepted)]

Attachment: 0001-Bug-1163956-Modify-updatePath-to-fixed-char-array.patch

Change FotaUpdateStatus.updatePath from ctypes.char.ptr to ctyps.char.array(4096), sync with librecovery.h

Comment 46

[:gerard-majax]

Eric, did you had time to check on other devices that this was not regressing?

Comment 47

[Eric Tsai (no more review request accepted)]

I've tested attachment 8630335 [details] [diff] [review] on flame-kk at branch 2.2 only, it works fine so I think will not cause regression. 
As I mentioned in Comment 39, I don't know why original code works on flame but crashes nexus 5. Maybe Gerry can helps to verify attachment 8630335 [details] [diff] [review] for more devices?

Comment 48

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8630335 [details] [diff] [review]
0001-Bug-1163956-Modify-updatePath-to-fixed-char-array.patch

LGTM but it be worth mentioning in a comment that this is due to librecovery.h using PATH_MAX as the size for that particular field so that someone reading the code doesn't wonder where that number is coming from.

Comment 49

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8628672 [details] [review]
Add RECOVERY_EXTERNAL_STORAGE

LGTM.

Comment 50

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8629267 [details] [review]
Support USE_SET_METADATA for build fota package

LGTM, I've just added a nit about commenting this change a little more. Our scripts are already fairly obscure and I think we should leave small comments in our code to explain why we're doing certain things otherwise it will be hard for future readers to figure out why we did.

Comment 51

[Eric Tsai (no more review request accepted)]

Attachment: [final]0001-Bug-1163956-Modify-updatePath-to-fixed-char-array-v2.patch, r=gsvelto

Update with comment for changes to char.array, and where the size from

Comment 52

[:gerard-majax]

(In reply to Eric Tsai from comment #51)
> Created attachment 8630924 [details] [diff] [review]
> [final]0001-Bug-1163956-Modify-updatePath-to-fixed-char-array-v2.patch,
> r=gsvelto
> 
> Update with comment for changes to char.array, and where the size from

Just my 2 cents, maybe to avoid potential issues we should reuse the PATH_MAX define rather than hardcoding 4096 ? Long shot, but still ...

Comment 53

[Eric Tsai (no more review request accepted)]

Agree, my original idea is to reuse the PATH_MAX define from limits.h. But I don't have proper way so finally hardcoding 4096.

Comment 54

[Gabriele Svelto [:gsvelto]]

(In reply to Eric Tsai from comment #53)
> Agree, my original idea is to reuse the PATH_MAX define from limits.h. But I
> don't have proper way so finally hardcoding 4096.

Yeah that would be best but I don't know of any reasonable way to pull a constant such as PATH_MAX defined in a header into JavaScript :-( Maybe we can put it in a global somewhere and read that using jsctypes but that feels like overkill for this patch. We can open a bug for that though, it might be a good first bug (low priority, high reward in terms of learning how our stuff works).

Comment 55

[Eric Tsai (no more review request accepted)]

Please help to land attachment 8630924 [details] [diff] [review], try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e7ed693d7cb
After that, land attachment 8629267 [details] [review] then attachment 8628672 [details] [review]

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/2c678f1b4207

Comment 57

[Ryan VanderMeulen [:RyanVM]]

b2g-5.1.0_r1: https://github.com/mozilla-b2g/device-hammerhead/commit/1ad7478309fd43981973e297f5a68a480f3a3c07

Master: https://github.com/mozilla-b2g/B2G/commit/aaf0dcca17fea2902395d60167dc4dc5bf9a9e01

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c678f1b4207