1066543 - [Flame KK] Trying to apply a update without isOSUpdate=true fails with error code 43

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

STR:
1. Build a user or userdebug flame KK build over the 180 build. On this build, set 
pref("app.update.url", "http://SOME_SERVER/APATH/update.xml");
2. Flash that build

3. Prepare a FOTA with:
./build.sh gecko-update-full
3. Create the update.xml as:

./tools/update-tools/build-update-xml.py -v 99.0 -V 99.0 -o update.xml -u http://SOME_SERVER/APATH/update.xml ${PATH_TO_MAR}

4. Copy the update.xml and .mar files to your SOME_SERVER on APATH 

5. Ensure that /system is in ro mode (or just reboot the phone) if you're not sure

6. Search for updates on the phone and download the update.

Expected:

The update is downloaded and the user is offered to apply it

Actual: The update is downloaded, but the user isn't offered the option to apply it. Instead, logcat shows: 

I/Gecko   (  681): *** AUS:SVC readStatusFile - status: failed: 43, path: /data/local/updates/0/update.status
I/Gecko   (  681): UpdatePrompt: Update error, state: failed, errorCode: 43
I/Gecko   (  681): UpdatePrompt: Setting gecko.updateStatus: Install Pending
I/Gecko   (  681): *** AUS:SVC UpdateManager:refreshUpdateStatus - Notifying observers that the update was staged. state: pending, status: failed: 43


And the /data/local/updates/0/update.log shows:

Performing a staged update
MOZ_UPDATER_PRIO=19/-1000/3/0
GonkAutoMounter: Error mounting /system as read-write: Permission denied

GonkAutoMounter: Could not remount /system as read-write.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Set the device block file as RW before remounting

Since 

adb shell mount -o rw,remount /system 

worked but the mount in automounter didn't I just checked out what could be missing. And turns out we have to set the device block file to RW before remounting (and actually only have to do that once per boot, which explains why if I remounted as rw and then as ro it worked).

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

Note that as per the previous comment, the STR is slighty wrong. If the mount permissions have been touched at all, then the device *must* be rebooted for the OTA to fail.

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8488579 [details] [diff] [review]
Set the device block file as RW before remounting

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Partial OTAs cannot be applied.
Testing completed: Manually.
Risk to taking this patch (and alternatives if risky): I believe this is very low risk. In the worst case we won't be worse than now.
String or UUID changes made by this patch: None

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8488579 [details] [diff] [review]
Set the device block file as RW before remounting

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Partial OTAs cannot be applied.
Testing completed: Manually.
Risk to taking this patch (and alternatives if risky): I believe this is very low risk. In the worst case we won't be worse than now.
String or UUID changes made by this patch: None

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

[Blocking Requested - why for this release]: Without this patch we won't be able to do partial OTAs (the ones that don't require booting into recovery mode to apply).

Comment 6

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8488579 [details] [diff] [review]
Set the device block file as RW before remounting

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

Looks like a good approach, just a few concerns. I think it would also make sense to put the block device back in read-only mode in the |GonkAutoMounter| destructor, maybe as part of RemountSystem?

::: toolkit/mozapps/update/updater/automounter_gonk.cpp
@@ +207,5 @@
> +
> +/*
> + * Mark the given block device as read-write, using the BLKROSET ioctl.
> + */
> +static void fs_set_blk_rw(const char *blockdev) {

nit: use the upper camelcase function name style of the rest of this code? :) maybe, |SetBlockReadWrite|

@@ +216,5 @@
> +  if (fd < 0) {
> +    return;
> +  }
> +
> +  ioctl(fd, BLKROSET, &off);

can you check the return code of ioctl, and log if it fails?

it would also be good to assert that the block is now readwrite with another call to |ioctl(fd, BLKROGET, ..)|, but maybe that is pedantic?

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2. Includes previous review comments

The only thing I didn't do was check the value with another ioctl again... Besides hoping the call to fail if it can't proceed, in any case the remount will fail if the device isn't RW, and that's controlled already.

Comment 8

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8488720 [details] [diff] [review]
V2. Includes previous review comments

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

Thanks for addressing this so quickly, it looks great!

::: toolkit/mozapps/update/updater/automounter_gonk.cpp
@@ +204,5 @@
>    return true;
>  }
>  
> +/*
> + * Mark the given block device as read-write, using the BLKROSET ioctl.

nit: just update this to reflect that the function can do read-only or read-write :)

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V3. Includes review comments

r=marshall

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8488735 [details] [diff] [review]
V3. Includes review comments

Set the device block file as RW before remounting
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Partial OTAs cannot be applied.
Testing completed: Manually.
Risk to taking this patch (and alternatives if risky): I believe this is very low risk. In the worst case we won't be worse than now.
String or UUID changes made by this patch: None

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8488735 [details] [diff] [review]
V3. Includes review comments

Set the device block file as RW before remounting

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Partial OTAs cannot be applied.
Testing completed: Manually.
Risk to taking this patch (and alternatives if risky): I believe this is very low risk. In the worst case we won't be worse than now.
String or UUID changes made by this patch: None

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Just a small clarification, for the blocking/approval. While testing the FOTAs (which are also failing on Flame ATM, see bug 1066723) I've just noticed that FOTAs can't be applied either without this patch (the process fails before rebooting into recovery).

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #13)
> Just a small clarification, for the blocking/approval. While testing the
> FOTAs (which are also failing on Flame ATM, see bug 1066723) I've just
> noticed that FOTAs can't be applied either without this patch (the process
> fails before rebooting into recovery).

Can you confirm this is a regression in 2.0 ?

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

I don't think this is a regression in 2.0 per se. It works in 2.0 with JB. I believe this is a regression introduced by switching to KK. But since 2.0 is going to be supported only in KK we have to fix it in 2.0 also. And maybe even in 1.4 although I haven't tested it there.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21c3f268d27f
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/0ef9688da1b6

Comment 17

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/0ef9688da1b6