927291 - [B2G][Helix][OTA][ChenHoulai]The handset has no response if fill the sdcard space when a update file is downloading.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[lecky]

User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; aff-kingsoft-ciba)

Steps to reproduce:

【Detail Description*】：
    [B2G][Helix][OTA][ChenHoulai]The handset has no response if fill the sdcard space when  a update file is downloading.
【Repro Steps*】：
  First case:
    1、Make sure the sdcard(/mnt/sdcard/) volume has enough space the do update(has space more than twice update file size).
    2、Enter into the settings, click 'check now' to get update info.
    3、Download the update file.
	4、When it start to download the update file, suspend the download. 
    5、Push some file into the /mnt/sdcard/  volume, let the remain space less the update file size(use adb push command)
	6、Resume the download.
    7、When the download execute to the case there is no space(has not begin to uncompress), the handset has no response. You can only remove the battery to restart.
	
	
  Second case:
    1、Make sure the sdcard(/mnt/sdcard/) volume has enough space the do update(has space more than twice update file size).
	2、Enter into the settings, click 'check now' to get update info.
    3、Download the update file.
	4、When it start to download the update file, suspend the download. 
    5、Push some file into the /mnt/sdcard/  volume, let the remain space less than twice the update file size but more than the update file size(use adb push command)
	6、Push some file into the /data/local/  volume, let the remain space less the update file size(use adb push command)
	7、Resume the download.
    8、When the download execute to the case there is no space(has begin to uncompress), the handset has no response.  You can not boot the handset in any way. It will get a freeze device at booting logo. 
【Expect Result*】：
  First case:
    7、There is a tip that there is no space and the handset has response.
	
  Second case:
    8、There is a tip that there is no space and the handset has response.	
	
  Second case:
【Real Result*】：
  First case:
    7、The handset has no response. You can only remove the battery to restart.
	
  Second case:
    8、The handset has no response.  You can not boot the handset in any way.  It will get a freeze device at booting logo. 

【Test Count*】：5
【Found Count*】：5
【Gaia commit ID*】: f601fe6e5edb7d5016c07abfad8f069af7354f7b
【Gecko commit ID*】: 60d9e982e07e8d09e9d0e52bad6433c302c17f32
【Log*】：NA
【Network environment】：
【Resume operation】：
【Carrier】：

Comment 1

[Wayne Chang [:wchang]]

kaizhen,

Can you check if these behaviours are expected with our current implementation and consistent with other v1.1 release devices?

I am removing hd? as this is requesting for new UX, but please renom if this is a regression from other v1.1 release devices.

Comment 2

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

Yes, this could be an issue. The require space is only checked before start downloading update.
I think "nsIncrementalDownload" module need to handle the require/available space and this case will get fixed.

Comment 3

[Wayne Chang [:wchang]]

Thanks Kaizhen,

I also need to confirm this is the expected behavior on the current v1.1 implementation? I.e. It would behave the same on other v1.1 devices?

Comment 4

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

AFAIK, this can be expected on current v1.1 implementation, and behave the same on other v1.1 devices. 
Can you loop the related module owner and have a discussion?

Comment 5

[Wayne Chang [:wchang]]

Thanks. 

Setting this to koi? So it gets some attention from owners and consideration for v1.2. 

As this is expected with v1.1 implementation and is a rather corner case, we won't block v1.1/HD on this.

Comment 6

[lecky]

hi Beatriz,
 will you agree this issue not to be fixed currently?

Comment 7

[Maria]

Hi,

Beatriz had already confirmed that behaviors consistent with all v1.1 devices wouldn't be blocking for v1.1hd.

Removing ni.

Br

Comment 8

[lecky]

(In reply to Maria from comment #7)
> Hi,
> 
> Beatriz had already confirmed that behaviors consistent with all v1.1
> devices wouldn't be blocking for v1.1hd.
> 
> Removing ni.
> 
> Br

Thanks.

Comment 9

[lecky]

(In reply to Kai-Zhen Li from comment #2)
> Yes, this could be an issue. The require space is only checked before start
> downloading update.
> I think "nsIncrementalDownload" module need to handle the require/available
> space and this case will get fixed.


hi Kai-Zhen Li:
I want to know whether the "nsIncrementalDownload" module can handle the require/available space on 1.2?
If i want to modify this issue , can you give me some suggest that which file i need to trace in?

Comment 10

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

"nsIncrementalDownload" is implemented in "netwerk/base/src/nsIncrementalDownload.cpp". I　will update a patch later.

Comment 11

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

Attachment: bug-927291-fix.patch

When there is error during FlushChunk return it to prevent the system get no response.

Comment 12

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

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=bb6d0be72d5d

Comment 13

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

Wayne, I think this patch could be uplift to v1.1 and v1.2. How do you think?

Comment 14

[Wayne Chang [:wchang]]

Thanks Kaizhen.

I am bumping this to leo?, apologies for contradiction from my comment 1, after re-reading the symptoms there's a possibility of device bricking as a result of this bug, so I'll put this to leo? for consideration.

Preeti, is v1.1 triage still ongoing?

Comment 15

[Wayne Chang [:wchang]]

Additional info after more investigation (and reading the bug more carefully):

I think we're not actually 'bricking' the device here, the second scenario that Lecky tested here results in device stuck at boot is likely due to /data being full, and we need some space available for it to boot.
 	
>   Second case:
> 	6、Push some file into the /data/local/  volume, let the remain space less
> the update file size(use adb push command)


This is not directly related to the FOTA process, but we need to consider if there is possibility of user actions while doing FOTA would fill up /data, for example downloading FOTA and shooting a video in parallel without an SD card inserted?

Comment 16

[Alan Huang]

(In reply to Wayne Chang [:wchang] from comment #15)
>   Second case:
> 	6、Push some file into the /data/local/  volume, let the remain space less
> the update file size(use adb push command)

I won't treat push files to /data as a normal UX.

Our GonkDiskSpaceWatcher sets "Disk Full" once free space in /data is less than our low threshold, which is 5 * 1024 * 1024 bytes or another value if you specify preference "disk_space_watcher.low_threshold".

Comment 17

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

(In reply to Wayne Chang [:wchang] from comment #15)
> >   Second case:
> > 	6、Push some file into the /data/local/  volume, let the remain space less
> > the update file size(use adb push command)
> 
> This is not directly related to the FOTA process, but we need to consider if
> there is possibility of user actions while doing FOTA would fill up /data,
> for example downloading FOTA and shooting a video in parallel without an SD
> card inserted?

Wayne, base on Comment 16, the second case does no longer exist in normal UX.

Comment 18

[Jason Duell]

Comment on attachment 820105 [details] [diff] [review]
bug-927291-fix.patch

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

::: netwerk/base/src/nsIncrementalDownload.cpp
@@ +687,5 @@
>  
> +    if (mChunkLen == mChunkSize) {
> +      rv = FlushChunk();
> +      if (NS_FAILED(rv))
> +        return rv;

OK, so this code will now cancel the channel and call the observer's OnStopRequest() with an error status.  That's good.  But do we know what the observer does at that point (I'm not sure which code is the Observer here).  Hopefully it's deleting the partially downloaded file and somehow notifying the user that there's been an error.  Kai-Zen, how do the test cases in comment 0 behave once this patch is applied?

Comment 19

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

(In reply to Jason Duell (:jduell) from comment #18)
> OK, so this code will now cancel the channel and call the observer's
> OnStopRequest() with an error status.  That's good.  But do we know what the
> observer does at that point (I'm not sure which code is the Observer here). 
> Hopefully it's deleting the partially downloaded file and somehow notifying
> the user that there's been an error.  Kai-Zen, how do the test cases in
> comment 0 behave once this patch is applied?

Once the patch is applied user will be notified, and when user click to install the update, system will reboot into recovery mode and finish the update.

I verified this patch on phone, observer(AUS) will handle the returned error and notify user there is error during downloading update.

Comment 20

[Wayne Chang [:wchang]]

hd+'ing this to get it moving.

Bhavana/Preeti, we'll need to consider this for leo as well as it potentially causes a stuck at boot logo phenomenon.

Comment 21

[lecky]

hi Kai-Zhen Li:
I apply your patch and it work fine. Now the First case in comment 0 does not exist any more.
But the Second case seem to still exist.
In that case, the update file begin to uncompress and the handset has no response.  You can not boot the handset in any way. It will get a freeze device at booting logo.
I trace into the case, the uncompress is done in nsUpdateDriver.cpp and the space get is done in DirectoryProvider.js.
Can you give some sugestion about how to modify it to avoid that case happen?

Comment 22

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

(In reply to lecky from comment #21)
> I trace into the case, the uncompress is done in nsUpdateDriver.cpp and the
> space get is done in DirectoryProvider.js.
> Can you give some sugestion about how to modify it to avoid that case happen?

Yes, my patch only solve the issue of the available space is not enough during downloading.

The available space become not enough during decompressing will cause similar issue. 
I think Geckoupdater OR update service need to handle the exception properly in order to solve the second issue.
But I don't know how to patch them right now.

Comment 23

[Jason Duell]

Comment on attachment 820105 [details] [diff] [review]
bug-927291-fix.patch

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

OK, it sounds like this patch fixes issue #1 in comment 0, so I think we should land it, close this bug, and create a new one for issue #2 (run out of space during download install).  It looks like that needs a totally different fix (probably with a reviewer who's not me).

Thanks for the patch Kai-Zhen!

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ecd3ac0f71

Comment 25

[Phil Ringnalda (:philor)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Kai-Zhen, please add your name to your hg commit information.

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdd327836fb4
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/369468c97d16

Leaving status-b2g18 set to affected pending a response to comment 20.

Comment 27

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

Ryan, Thanks!

Comment 28

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

A new Bug 931740 is created for issue #2 which is caused by a different issue and needs a totally different fix.