1118177 - [FFOS7715 v2.1][bluetooth] remove SD card while share pictures by bluetooth, result in SD card cannot be used again until restart.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[ben.song]

Reproduce step:

1.Open app gallery and load pictures.
2.Select several pictures to share by bluetooth.
3.In this moment, Remove sdcard suddenly.
4.After several seconds, insert this sdcard into phone.


Ideal result:

this sdcard could be recognized.

Actual result:

this sdcard could not be recognized, and only restart phone could recovery.

Reproduce rate: 5/5

Comment 1

[ben.song]

[Blocking Requested - why for this release]:

Comment 2

[siiaceon]

this is very bad performance which make user cannot use sdcard except reboot handset

Comment 3

[ben.song]

Dear Vance,Shawn,

From the system log we could get that while remove sdcard b2g process would be killed for bluetooth is occupying sdcard source. So the moment the phone would restart,Can you find some to help me with this problem ?

Comment 4

[shawn ku [:sku]]

Hi Shawn.H:
 Could you please help check this issue first?

Thanks!!
Shawn Ku

Comment 5

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Remove my ni since Shawn will help to check this one

Comment 6

[siiaceon]

Shawn.H:
  how about the STR, this is one of the top big issue, because I believe the customer will no agree to restart B2G . thanks for your kindly help

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to ben.song from comment #0)
> Reproduce step:
> 
> 1.Open app gallery and load pictures.
> 2.Select several pictures to share by bluetooth.
> 3.In this moment, Remove sdcard suddenly.
> 4.After several seconds, insert this sdcard into phone.
> 
> 
> Ideal result:
> 
> this sdcard could be recognized.
> 
> Actual result:
> 
> this sdcard could not be recognized, and only restart phone could recovery.
> 
> Reproduce rate: 5/5

Can you provide minidump for crash analysis?
I cannot test it because currently woodduck cannot remove sdcard without removing battery.
Thanks.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> (In reply to ben.song from comment #0)
> > Reproduce step:
> > 
> > 1.Open app gallery and load pictures.
> > 2.Select several pictures to share by bluetooth.
> > 3.In this moment, Remove sdcard suddenly.
> > 4.After several seconds, insert this sdcard into phone.
> > 
> > 
> > Ideal result:
> > 
> > this sdcard could be recognized.
> > 
> > Actual result:
> > 
> > this sdcard could not be recognized, and only restart phone could recovery.
> > 
> > Reproduce rate: 5/5
> 
> Can you provide minidump for crash analysis?
> I cannot test it because currently woodduck cannot remove sdcard without
> removing battery.
> Thanks.

Sorry, this is not woodduck issue, but dolphin (v2.1).

Comment 9

[ben.song]

(In reply to Shawn Huang [:shawnjohnjr] from comment #8)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #7)
> > (In reply to ben.song from comment #0)
> > > Reproduce step:
> > > 
> > > 1.Open app gallery and load pictures.
> > > 2.Select several pictures to share by bluetooth.
> > > 3.In this moment, Remove sdcard suddenly.
> > > 4.After several seconds, insert this sdcard into phone.
> > > 
> > > 
> > > Ideal result:
> > > 
> > > this sdcard could be recognized.
> > > 
> > > Actual result:
> > > 
> > > this sdcard could not be recognized, and only restart phone could recovery.
> > > 
> > > Reproduce rate: 5/5
> > 
> > Can you provide minidump for crash analysis?
> > I cannot test it because currently woodduck cannot remove sdcard without
> > removing battery.
> > Thanks.
> 
> Sorry, this is not woodduck issue, but dolphin (v2.1).

Dear Shawn,

Never mind，the reproduce of this problem is 5/5 in sprd QA，and in RD is lower than 5/5. But the influence of this problem is big.

Thanks.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi
b2g process was killed by Vold. I think under this situation, b2g process cannot know when user eject sdcard, so b2g process cannot know how to prevent Vold sent SIGHUP signal.
Can you ask for your Vold owner to comment this situation? We cannot handle it from gecko side.
Can you ask for Vold owner to not kill b2g process?

20947 12-06 15:48:47.600 I/Vold    (   94): /mnt/secure/asec sucessfully unmounted
20948 12-06 15:48:47.600 W/Vold    (   94): Failed to unmount /storage/sdcard0 (Device or resource busy, retries 2, action 1)
20949 12-06 15:48:47.690 E/ProcessKiller(   94): Process /system/b2g/b2g (108) has open file /storage/sdcard0/DCIM/100MZLLA/IMG_0005.jpg
20950 12-06 15:48:47.690 W/ProcessKiller(   94): Sending SIGHUP to process 108

Comment 11

[ben.song]

(In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> Hi
> b2g process was killed by Vold. I think under this situation, b2g process
> cannot know when user eject sdcard, so b2g process cannot know how to
> prevent Vold sent SIGHUP signal.
> Can you ask for your Vold owner to comment this situation? We cannot handle
> it from gecko side.
> Can you ask for Vold owner to not kill b2g process?
> 
> 20947 12-06 15:48:47.600 I/Vold    (   94): /mnt/secure/asec sucessfully
> unmounted
> 20948 12-06 15:48:47.600 W/Vold    (   94): Failed to unmount
> /storage/sdcard0 (Device or resource busy, retries 2, action 1)
> 20949 12-06 15:48:47.690 E/ProcessKiller(   94): Process /system/b2g/b2g
> (108) has open file /storage/sdcard0/DCIM/100MZLLA/IMG_0005.jpg
> 20950 12-06 15:48:47.690 W/ProcessKiller(   94): Sending SIGHUP to process
> 108

Dear Shawn,

We have apply a patch about it. When remove sdcard, we would check the process if or not b2g, if yes SIGHUP would not kill b2g and b2g would not restart.

But after it, when we insert sdcard, the phone would not recognize this sdcard and b2g would hold this resource to not release.

Could you help us with this problem ? For example，b2g make a delay to release the resource while remove the sdcard.

Thanks.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Ben.song
Thanks, can you share the patch?
I want to do some test and ask our storage team how to handle the problem you mentioned.

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to ben.song from comment #11)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> > Hi
> > b2g process was killed by Vold. I think under this situation, b2g process
> > cannot know when user eject sdcard, so b2g process cannot know how to
> > prevent Vold sent SIGHUP signal.
> > Can you ask for your Vold owner to comment this situation? We cannot handle
> > it from gecko side.
> > Can you ask for Vold owner to not kill b2g process?
> > 
> > 20947 12-06 15:48:47.600 I/Vold    (   94): /mnt/secure/asec sucessfully
> > unmounted
> > 20948 12-06 15:48:47.600 W/Vold    (   94): Failed to unmount
> > /storage/sdcard0 (Device or resource busy, retries 2, action 1)
> > 20949 12-06 15:48:47.690 E/ProcessKiller(   94): Process /system/b2g/b2g
> > (108) has open file /storage/sdcard0/DCIM/100MZLLA/IMG_0005.jpg
> > 20950 12-06 15:48:47.690 W/ProcessKiller(   94): Sending SIGHUP to process
> > 108
> 
> Dear Shawn,
> 
> We have apply a patch about it. When remove sdcard, we would check the
> process if or not b2g, if yes SIGHUP would not kill b2g and b2g would not
> restart.
> 
> But after it, when we insert sdcard, the phone would not recognize this
> sdcard and b2g would hold this resource to not release.
> 
> Could you help us with this problem ? For example，b2g make a delay to
> release the resource while remove the sdcard.
> 
> Thanks.

I guess Vold needs to send unmount event to b2g process in order to fake correct procedure of ejecting SD card. Is your patch including this part?

Comment 14

[ben.song]

Attachment: last.patch

Dear Shawn,

This comment is sprd patch for kill b2g process.

I think in v2.1 remove or insert sdcard may send event to gecko and gaia, because when hot plug of sdcard, statusbar would pop a message what said sdcard insert or remove.

Thanks.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to ben.song from comment #14)
> Created attachment 8546455 [details] [diff] [review]
> last.patch
> 
> Dear Shawn,
> 
> This comment is sprd patch for kill b2g process.
> 
> I think in v2.1 remove or insert sdcard may send event to gecko and gaia,
> because when hot plug of sdcard, statusbar would pop a message what said
> sdcard insert or remove.
> 
> Thanks.

Can you provide logcat with timestamp?

Comment 16

[ben.song]

(In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> (In reply to ben.song from comment #14)
> > Created attachment 8546455 [details] [diff] [review]
> > last.patch
> > 
> > Dear Shawn,
> > 
> > This comment is sprd patch for kill b2g process.
> > 
> > I think in v2.1 remove or insert sdcard may send event to gecko and gaia,
> > because when hot plug of sdcard, statusbar would pop a message what said
> > sdcard insert or remove.
> > 
> > Thanks.
> 
> Can you provide logcat with timestamp?

Dear Shawn,

Which log would you like ? for example, main log, kernel log, system log ?

Thanks.

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to ben.song from comment #16)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> > (In reply to ben.song from comment #14)
> > > Created attachment 8546455 [details] [diff] [review]
> > > last.patch
> > > 
> > > Dear Shawn,
> > > 
> > > This comment is sprd patch for kill b2g process.
> > > 
> > > I think in v2.1 remove or insert sdcard may send event to gecko and gaia,
> > > because when hot plug of sdcard, statusbar would pop a message what said
> > > sdcard insert or remove.
> > > 
> > > Thanks.
> > 
> > Can you provide logcat with timestamp?
> 
> Dear Shawn,
> 
> Which log would you like ? for example, main log, kernel log, system log ?
> 
> Thanks.

logcat log with timestamp of testing procedure (Start-End).

Comment 18

[ben.song]

Attachment: 1118177.tar.gz

Dear Shawn,

This is the log, but without corefile.

Thanks.

Comment 19

[ben.song]

Dear Shawn,

What do you think with this problem ?

Thanks.

Comment 20

[bhavana bajaj [:bajaj]]

Can QA/reporter confirm if this issue happening on Flame and check if its a regression ?

Comment 21

[Brogan Zumwalt [Inactive]]

Unable to test on Flame due to hardware limitations of SD card connector. As per https://bugzilla.mozilla.org/show_bug.cgi?id=1077642#c35 we are unable to hot-swap SD card on Flame.

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Alphan,
Do you have any suggestion?

============  14-12-30 05:47:02  ==============
12-30 05:46:40.660   114   114 I Vold    : Vold 2.1 (the revenge) firing up
12-30 05:46:40.660   114   114 D Vold    : Volume sdcard0 state changing -1 (Initializing) -> 0 (No-Media)
12-30 05:46:40.660   114   114 I Vold    : tempSD: bind /storage/temp_sd -> /storage/sdcard0
12-30 05:46:40.660   114   114 E Vold    : Failed to bind mount points /storage/temp_sd -> /storage/sdcard0 (Invalid argument)
12-30 05:46:40.770   114   147 E DirectVolume: making device node '/dev/block/vold/179:128' 
12-30 05:46:40.770   114   147 D DirectVolume: Dv::diskIns - waiting for 1 partitions (mask 0x2)
12-30 05:46:40.770   114   147 D Vold    : Volume sdcard0 state changing 0 (No-Media) -> 2 (Pending)
12-30 05:46:40.770   114   147 W Vold    : tempSD: mTempSDEnabled is equal to enable, 0
12-30 05:46:40.770   114   147 E DirectVolume: making device node '/dev/block/vold/179:129' 
12-30 05:46:40.780   114   147 D DirectVolume: Dv:partAdd: part_num = 1, minor = 129
12-30 05:46:40.780   114   147 D DirectVolume: Dv:partAdd: Got all partitions - ready to rock!
12-30 05:46:40.780   114   147 D Vold    : Volume sdcard0 state changing 2 (Pending) -> 1 (Idle-Unmounted)
12-30 05:46:45.360   114   154 I Vold    : /dev/block/vold/179:129 being considered for volume sdcard0
12-30 05:46:45.360   114   154 D Vold    : Volume sdcard0 state changing 1 (Idle-Unmounted) -> 3 (Checking)
12-30 05:46:47.100   114   154 W Vold    : Filesystem modified - rechecking (pass 2)
12-30 05:46:50.180   114   154 D Vold    : Volume sdcard0 state changing 3 (Checking) -> 4 (Mounted)

Comment 23

[bhavana bajaj [:bajaj]]

This seems like unusual use-case, I am not sure we would block on this issue. Steven can you help to get a waiver on this for 2.1 or let us know if this blocks some kind of certification ?

Comment 24

[Alphan Chen [:alchen]]

Hi Shawn,
the log you pasted show the process of Vold remount sdcard0.
However, I don't find the unmount part in this log. (1.remove sdcard physically. 2.vold should do the unmount process and report the unmount state to gecko)
I think we can try to reproduce it on a device with SD hotplug pin.

(In reply to Shawn Huang [:shawnjohnjr] from comment #22)
> Hi Alphan,
> Do you have any suggestion?
> 
> ============  14-12-30 05:47:02  ==============
> 12-30 05:46:40.660   114   114 I Vold    : Vold 2.1 (the revenge) firing up
> 12-30 05:46:40.660   114   114 D Vold    : Volume sdcard0 state changing -1
> (Initializing) -> 0 (No-Media)
> 12-30 05:46:40.660   114   114 I Vold    : tempSD: bind /storage/temp_sd ->
> /storage/sdcard0
> 12-30 05:46:40.660   114   114 E Vold    : Failed to bind mount points
> /storage/temp_sd -> /storage/sdcard0 (Invalid argument)
> 12-30 05:46:40.770   114   147 E DirectVolume: making device node
> '/dev/block/vold/179:128' 
> 12-30 05:46:40.770   114   147 D DirectVolume: Dv::diskIns - waiting for 1
> partitions (mask 0x2)
> 12-30 05:46:40.770   114   147 D Vold    : Volume sdcard0 state changing 0
> (No-Media) -> 2 (Pending)
> 12-30 05:46:40.770   114   147 W Vold    : tempSD: mTempSDEnabled is equal
> to enable, 0
> 12-30 05:46:40.770   114   147 E DirectVolume: making device node
> '/dev/block/vold/179:129' 
> 12-30 05:46:40.780   114   147 D DirectVolume: Dv:partAdd: part_num = 1,
> minor = 129
> 12-30 05:46:40.780   114   147 D DirectVolume: Dv:partAdd: Got all
> partitions - ready to rock!
> 12-30 05:46:40.780   114   147 D Vold    : Volume sdcard0 state changing 2
> (Pending) -> 1 (Idle-Unmounted)
> 12-30 05:46:45.360   114   154 I Vold    : /dev/block/vold/179:129 being
> considered for volume sdcard0
> 12-30 05:46:45.360   114   154 D Vold    : Volume sdcard0 state changing 1
> (Idle-Unmounted) -> 3 (Checking)
> 12-30 05:46:47.100   114   154 W Vold    : Filesystem modified - rechecking
> (pass 2)
> 12-30 05:46:50.180   114   154 D Vold    : Volume sdcard0 state changing 3
> (Checking) -> 4 (Mounted)

Comment 25

[howie [:howie]]

move it to 2.1S? since it's not happening on Flame

Comment 26

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Alphan Chen[:Alphan] from comment #24)
> Hi Shawn,
> the log you pasted show the process of Vold remount sdcard0.
> However, I don't find the unmount part in this log. (1.remove sdcard
> physically. 2.vold should do the unmount process and report the unmount
> state to gecko)
> I think we can try to reproduce it on a device with SD hotplug pin.
> 
> (In reply to Shawn Huang [:shawnjohnjr] from comment #22)
> > Hi Alphan,
> > Do you have any suggestion?
> > 
> > ============  14-12-30 05:47:02  ==============
> > 12-30 05:46:40.660   114   114 I Vold    : Vold 2.1 (the revenge) firing up
> > 12-30 05:46:40.660   114   114 D Vold    : Volume sdcard0 state changing -1
> > (Initializing) -> 0 (No-Media)
> > 12-30 05:46:40.660   114   114 I Vold    : tempSD: bind /storage/temp_sd ->
> > /storage/sdcard0
> > 12-30 05:46:40.660   114   114 E Vold    : Failed to bind mount points
> > /storage/temp_sd -> /storage/sdcard0 (Invalid argument)
> > 12-30 05:46:40.770   114   147 E DirectVolume: making device node
> > '/dev/block/vold/179:128' 
> > 12-30 05:46:40.770   114   147 D DirectVolume: Dv::diskIns - waiting for 1
> > partitions (mask 0x2)
> > 12-30 05:46:40.770   114   147 D Vold    : Volume sdcard0 state changing 0
> > (No-Media) -> 2 (Pending)
> > 12-30 05:46:40.770   114   147 W Vold    : tempSD: mTempSDEnabled is equal
> > to enable, 0
> > 12-30 05:46:40.770   114   147 E DirectVolume: making device node
> > '/dev/block/vold/179:129' 
> > 12-30 05:46:40.780   114   147 D DirectVolume: Dv:partAdd: part_num = 1,
> > minor = 129
> > 12-30 05:46:40.780   114   147 D DirectVolume: Dv:partAdd: Got all
> > partitions - ready to rock!
> > 12-30 05:46:40.780   114   147 D Vold    : Volume sdcard0 state changing 2
> > (Pending) -> 1 (Idle-Unmounted)
> > 12-30 05:46:45.360   114   154 I Vold    : /dev/block/vold/179:129 being
> > considered for volume sdcard0
> > 12-30 05:46:45.360   114   154 D Vold    : Volume sdcard0 state changing 1
> > (Idle-Unmounted) -> 3 (Checking)
> > 12-30 05:46:47.100   114   154 W Vold    : Filesystem modified - rechecking
> > (pass 2)
> > 12-30 05:46:50.180   114   154 D Vold    : Volume sdcard0 state changing 3
> > (Checking) -> 4 (Mounted)

Hi Alphan, i have a Dolphin device with SD hotswap detection pin, you can have it if you want to reproduce this issue yourself

Comment 27

[Alphan Chen [:alchen]]

Shawn also have one on his hand.
Thanks anyway.

Comment 28

[Alphan Chen [:alchen]]

Here is my local try.
From log, we can see the state of sdcard0 is changed from "Mounted" to "Unmounting" after removing sdcard physically.
Then "Unmounting" to "Idle".
In the end "Idle" to "No-Media".
So we can add a listener for this state change and do some corresponding behavior.

(logcat)
01-15 18:01:46.234   108   360 I VolumeManager: Volume sdcard0 (1): changing state from Mounted to Unmounting (3 observers)
01-15 18:01:46.234   108   360 I AutoMounter: AutoMounter state changed from IDLE to UMS_CONFIGURED
01-15 18:01:46.234   108   360 I AutoMounter: UpdateState: ums:A1C1E0 mtp:A1C0E0 mode:0 usb:1 tryToShare:0 state:UMS_CONFIGURED
01-15 18:01:46.234   108   360 I AutoMounter: UpdateState: Volume sdcard0 is Unmounting and missing
01-15 18:01:46.234   108   360 I AutoMounter: AutoMounter state changed from UMS_CONFIGURED to IDLE
01-15 18:01:46.234   108   360 I AutoMounter: UpdateState: ums:A1C1E0 mtp:A1C0E0 mode:0 usb:1 tryToShare:0 state:IDLE
01-15 18:01:46.234   108   360 I AutoMounter: UpdateState: Volume sdcard0 is Unmounting and missing
01-15 18:01:46.234    94   193 W Vold    : tempSD: service fuse_sdcard0 is running, stop it first!
01-15 18:01:46.234    94   193 D DirectVolume: Volume sdcard0 /mnt/media_rw/sdcard0 partition 179:1 removed
01-15 18:01:46.234    94   193 D Vold    : Volume sdcard0 state changing 4 (Mounted) -> 5 (Unmounting)
...
01-15 18:01:51.434   108   360 I VolumeManager: Volume sdcard0 (1): changing state from Unmounting to Idle (3 observers)
01-15 18:01:51.434    94   193 I Vold    : /storage/sdcard0 sucessfully unmounted
01-15 18:01:51.434    94   193 I Vold    : tempSD: bind /storage/temp_sd -> /storage/sdcard0
01-15 18:01:51.434    94   193 E Vold    : Failed to bind mount points /storage/temp_sd -> /storage/sdcard0 (Invalid argument)
01-15 18:01:51.434    94   193 I Vold    : /mnt/media_rw/sdcard0 sucessfully unmounted
01-15 18:01:51.434    94   193 I Vold    : /mnt/media_rw/sdcard0 unmounted successfully
01-15 18:01:51.434    94   193 D Vold    : Volume sdcard0 state changing 5 (Unmounting) -> 1 (Idle-Unmounted)
...
01-15 18:01:51.464   108   360 I VolumeManager: Volume sdcard0 (1): changing state from Idle to NoMedia (3 observers)
01-15 18:01:51.464    94   193 D Vold    : Volume sdcard0 state changing 1 (Idle-Unmounted) -> 0 (No-Media)

Comment 29

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Thanks, Alphan. I think we can add AddObserver and check NS_VOLUME_STATE_CHANGED to release locked resource.

Comment 30

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Shawn, Alphan, appreciate your help, if you have WIP patch available, please help to attach it here. SPRD can help us test and verify 

Thanks

Vance

Comment 31

[ben.song]

Dear Shawn, Alphan,

Thanks for your help, i would wait for your patch to test and verify it.

Comment 32

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Comment 33

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1118177 - Release file resources if users unexpectedly remove SD card (v2)

code update (v2)-
* remove observer in destructor

Comment 34

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Ben Song, patch is ready, could you give it a try?

Thanks

Vance

Comment 35

[ben.song]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #34)
> Hi Ben Song, patch is ready, could you give it a try?
> 
> Thanks
> 
> Vance

Dear Vance,Shawn,

I would try this patch, and give you feedback as soon as possible.

Thanks a lot.

Comment 36

[siiaceon]

Thanks so much Vance & Shawn.H.

Ben , pls ask our QA to do the official try, then feedback, thx.

Comment 37

[ben.song]

Comment on attachment 8552254 [details] [diff] [review]
Bug 1118177 - Release file resources if users unexpectedly remove SD card (v2)

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

We have verified this patch, and it work well.This problem has been revolved.

Comment 38

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to ben.song from comment #37)
> Comment on attachment 8552254 [details] [diff] [review]
> Bug 1118177 - Release file resources if users unexpectedly remove SD card
> (v2)
> 
> Review of attachment 8552254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We have verified this patch, and it work well.This problem has been revolved.
Ben.song,
Thanks for testing.

Comment 39

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Will, can you take up this bug?

Comment 40

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Dear Ben

I applied this patch and examined it, the original reproduce steps will not cause issue anymore.

Moreover, I performed following tests and these are OK as well:
1. (change the initiator) android phone(Nexus 4) initiate the file transfer and remove SD card in the firefox phone during the transfer = OK
2. (use another file format) transfer a bigger video clip = OK

Please help to review this patch,
Thanks!

Comment 41

[Ben Tian (inactive)]

Comment on attachment 8553648 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +76,5 @@
>  {
>    MOZ_ASSERT(sBluetoothOppManager);
>  
> +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject);

Do we care change of all volumes, the volume we are using, or external storage only? Can we create DeviceStorageFile during initialization to save repetitive volume name queries?

@@ +77,5 @@
>    MOZ_ASSERT(sBluetoothOppManager);
>  
> +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject);
> +

nit: remove newline here.

@@ +90,5 @@
> +    nsString status;
> +    // Get Status (one of "available, unavailable, shared")
> +    dsf->GetStatus(status);
> +    BT_LOGR("Storage status: %s",
> +      NS_ConvertUTF16toUTF8(status).get());

nit: suggest to make code clearer as following:

    nsString volName;
    vol->GetName(volName);

    // Get device storage status ("available"/"unavailable"/"shared")
    nsRefPtr<DeviceStorageFile> dsf =
      new DeviceStorageFile(NS_LITERAL_STRING(DEVICESTORAGE_SDCARD), volName));
    nsString status;
    dsf->GetStatus(status);

    BT_LOGR("Storage status: %s", NS_ConvertUTF16toUTF8(status).get());

@@ +94,5 @@
> +      NS_ConvertUTF16toUTF8(status).get());
> +
> +    // Having MountLock can only handle device storage shared mode.
> +    // It is required to handle a case, the external storage is unexpectedly
> +    // been removed.

Suggest to simplify comment as following:

    // Handle unexpected removal of external storage here since
    // sdcard MountLock only handles device storage shared mode.

Comment 42

[Will Wang [:WillWang]]

Hi Ben

thanks for your feedback!

I had wrote a new patch based on your feedback,
the new patch can:
1. only care about the volume which OPP is using, this can avoid further potential bugs.
2. avoid using of DeviceStorageFile(including its object creation) by directly using GetState method provided by Volume class

I am testing this patch and will update it soon
thanks!!

Comment 43

[ben.song]

(In reply to Will Wang [:WillWang] from comment #42)
> Hi Ben
> 
> thanks for your feedback!
> 
> I had wrote a new patch based on your feedback,
> the new patch can:
> 1. only care about the volume which OPP is using, this can avoid further
> potential bugs.
> 2. avoid using of DeviceStorageFile(including its object creation) by
> directly using GetState method provided by Volume class
> 
> I am testing this patch and will update it soon
> thanks!!

Dear Will,

OK, thanks for your patch.

Comment 44

[Will Wang [:WillWang]]

After testing patch, I found that:

Since files selected in transfer may involve multiple volume, so observing only one volume is not enough for some cases

For now, the stable solution is: abort connection if any volume state is changed to un-mounted

The incoming patch will be implemented in this way

Comment 45

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Dear Ben Tian,

This is the patch as described above,
I had tested in several ways and didn't find any issue,
May you help to review,
thanks!

Dear Shawn and Alphan,
if you have any concern, please also kindly provide suggestion :)

thanks!

Comment 46

[Ben Tian (inactive)]

Comment on attachment 8558519 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +78,5 @@
>  
> +  // if state of any volume was changed
> +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject);
> +

nit: remove newline here.

@@ +86,5 @@
> +
> +    // Handle unexpected removal of any storage here since
> +    // sdcard MountLock only handles device storage shared mode.
> +    int32_t state;
> +    nsresult rv = vol->GetState(&state);

Remove |rv| since it's useless.

@@ +87,5 @@
> +    // Handle unexpected removal of any storage here since
> +    // sdcard MountLock only handles device storage shared mode.
> +    int32_t state;
> +    nsresult rv = vol->GetState(&state);
> +    if (state == nsIVolume::STATE_MOUNTED) {

Why monitor STATE_MOUNTED here?

@@ +90,5 @@
> +    nsresult rv = vol->GetState(&state);
> +    if (state == nsIVolume::STATE_MOUNTED) {
> +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> +    }
> +    else{

nit: } else {

@@ +91,5 @@
> +    if (state == nsIVolume::STATE_MOUNTED) {
> +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> +    }
> +    else{
> +      BT_LOGR("State of a volume is not STATE_MOUNTED, abort OPP connection if any.");

nit: newline here. Also revise log message as following:

  "Volume state is not STATE_MOUNTED. Abort any ongoing OPP connection."

@@ +94,5 @@
> +    else{
> +      BT_LOGR("State of a volume is not STATE_MOUNTED, abort OPP connection if any.");
> +      Disconnect(nullptr);
> +      DeleteReceivedFile();
> +      FileTransferComplete();

Does this patch monitor volume state for client, server, or both? Action here differs according to the role.

Comment 47

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #44)
> After testing patch, I found that:
> 
> Since files selected in transfer may involve multiple volume, so observing
> only one volume is not enough for some cases

Please detail the insufficient cases. Also is the fix for client only? How to handle sdcard removal for OPP server that stores file into sdcard?

Comment 48

[Ben Tian (inactive)]

Clear ni? for Shawn and Alphan.

Will, you don't have to ni? us additionally. We get informed with f? / r? flag set.

Comment 49

[Alphan Chen [:alchen]]

Comment on attachment 8558519 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

Please see my comment.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +87,5 @@
> +    // Handle unexpected removal of any storage here since
> +    // sdcard MountLock only handles device storage shared mode.
> +    int32_t state;
> +    nsresult rv = vol->GetState(&state);
> +    if (state == nsIVolume::STATE_MOUNTED) {

I also think we don't need to monitor mounted state here.

@@ +90,5 @@
> +    nsresult rv = vol->GetState(&state);
> +    if (state == nsIVolume::STATE_MOUNTED) {
> +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> +    }
> +    else{

I will use 
if (state == nsIVolume::STATE_MOUNTED) && mConnected)
{
   //disconnect the OOP connection
   ...
}

Comment 50

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8558519 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +90,5 @@
> +    nsresult rv = vol->GetState(&state);
> +    if (state == nsIVolume::STATE_MOUNTED) {
> +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> +    }
> +    else{

"I will use
if (state == nsIVolume::STATE_MOUNTED) && mConnected)
{
//disconnect the OOP connection
...
}"

Hmm...We shall check "NOT mounted" condition, right?

@@ +94,5 @@
> +    else{
> +      BT_LOGR("State of a volume is not STATE_MOUNTED, abort OPP connection if any.");
> +      Disconnect(nullptr);
> +      DeleteReceivedFile();
> +      FileTransferComplete();

I though it should be the same, but it's worthy confirming again.

Comment 51

[Will Wang [:WillWang]]

(In reply to Ben Tian [:btian] from comment #46)

Thanks for your review !
Please see my reply:

> Comment on attachment 8558519 [details] [diff] [review]
> 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch
> 
> Review of attachment 8558519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment below.
> 
> ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> @@ +78,5 @@
> >  
> > +  // if state of any volume was changed
> > +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> > +    nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject);
> > +
> 
> nit: remove newline here.
OK

> 
> @@ +86,5 @@
> > +
> > +    // Handle unexpected removal of any storage here since
> > +    // sdcard MountLock only handles device storage shared mode.
> > +    int32_t state;
> > +    nsresult rv = vol->GetState(&state);
> 
> Remove |rv| since it's useless.
OK!
My original idea was keeping this for possible further debugging usage,
however it may be better using it only if we need it.
I will remove this.

> 
> @@ +87,5 @@
> > +    // Handle unexpected removal of any storage here since
> > +    // sdcard MountLock only handles device storage shared mode.
> > +    int32_t state;
> > +    nsresult rv = vol->GetState(&state);
> > +    if (state == nsIVolume::STATE_MOUNTED) {
> 
> Why monitor STATE_MOUNTED here?
The idea I additional added this is to let log indicate whether the SD card is actually mounted,
otherwise we would see lots of fail log and cannot identify the current state is already correct or not.

And since vol->GetState() will be invoked regardless of state, so I suppose this would not have performance issue and can provide clear info to developer

> 
> @@ +90,5 @@
> > +    nsresult rv = vol->GetState(&state);
> > +    if (state == nsIVolume::STATE_MOUNTED) {
> > +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> > +    }
> > +    else{
> 
> nit: } else {
OK

> 
> @@ +91,5 @@
> > +    if (state == nsIVolume::STATE_MOUNTED) {
> > +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> > +    }
> > +    else{
> > +      BT_LOGR("State of a volume is not STATE_MOUNTED, abort OPP connection if any.");
> 
> nit: newline here. Also revise log message as following:
> 
>   "Volume state is not STATE_MOUNTED. Abort any ongoing OPP connection."
OK
Original idea is to address that non-mounted state of "any" volume will be handled,
however my wording may not be smooth

> 
> @@ +94,5 @@
> > +    else{
> > +      BT_LOGR("State of a volume is not STATE_MOUNTED, abort OPP connection if any.");
> > +      Disconnect(nullptr);
> > +      DeleteReceivedFile();
> > +      FileTransferComplete();
> 
> Does this patch monitor volume state for client, server, or both? Action
> here differs according to the role.
Thanks! I will confirm this and feedback,
besides, as my previous comment (https://bugzilla.mozilla.org/show_bug.cgi?id=1118177#c40), testing show that this patch is also work for the server case.

Thanks!

Comment 52

[Will Wang [:WillWang]]

(In reply to Ben Tian [:btian] from comment #47)
> (In reply to Will Wang [:WillWang] from comment #44)
> > After testing patch, I found that:
> > 
> > Since files selected in transfer may involve multiple volume, so observing
> > only one volume is not enough for some cases
> 
> Please detail the insufficient cases. Also is the fix for client only? How
> to handle sdcard removal for OPP server that stores file into sdcard?

insufficient cases:
Since user may select files which are scattered in multiple volume in a single transfer, (e.g., user select 2 files to transfer, one is in internal volume and another is in external volume),
so non-mounted state in any involved volume should be handled. 

my experiment show that:
if we only observe one volume, e.g., the preferred storage is set to internal one in setting UI, 
then once the file in SD card is selected for transfer and SD card is removed during the transfer, 
this volume state will not be handled, hence the issue still occurs.

therefore,
two possible strategy to solve this issue:
1. elaborately handle each file within the transfer: check only if any file is located in the unmounted volume, then disconnect this OPP connection (or more complex, just discard these inaccessible files, but let unaffected files go on). this way should be evaluated whether it is attainable and worth implementing or not.
2. disconnect if any volume is not mounted: less risk, might be more stable.

for now, I suppose 2nd strategy is suitable for this issue.

Comment 53

[Will Wang [:WillWang]]

(In reply to Alphan Chen[:Alphan] from comment #49)

Thanks for your comment!
please see my reply: 

> Comment on attachment 8558519 [details] [diff] [review]
> 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch
> 
> Review of attachment 8558519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment.
> 
> ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> @@ +87,5 @@
> > +    // Handle unexpected removal of any storage here since
> > +    // sdcard MountLock only handles device storage shared mode.
> > +    int32_t state;
> > +    nsresult rv = vol->GetState(&state);
> > +    if (state == nsIVolume::STATE_MOUNTED) {
> 
> I also think we don't need to monitor mounted state here.
I just left a reason for this,
may you help to feedback if any?
thanks!

> 
> @@ +90,5 @@
> > +    nsresult rv = vol->GetState(&state);
> > +    if (state == nsIVolume::STATE_MOUNTED) {
> > +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> > +    }
> > +    else{
> 
> I will use 
> if (state == nsIVolume::STATE_MOUNTED) && mConnected)
> {
>    //disconnect the OOP connection
>    ...
> }

OK!
I will use this improvement in the incoming patch,

thanks!!

Comment 54

[Will Wang [:WillWang]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #50)

thanks!
please see my reply:

> Comment on attachment 8558519 [details] [diff] [review]
> 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch
> 
> Review of attachment 8558519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> @@ +90,5 @@
> > +    nsresult rv = vol->GetState(&state);
> > +    if (state == nsIVolume::STATE_MOUNTED) {
> > +      BT_LOGR("State of a volume is changed to STATE_MOUNTED now.");
> > +    }
> > +    else{
> 
> "I will use
> if (state == nsIVolume::STATE_MOUNTED) && mConnected)
> {
> //disconnect the OOP connection
> ...
> }"
> 
> Hmm...We shall check "NOT mounted" condition, right?
Yes, you are right!

> 
> @@ +94,5 @@
> > +    else{
> > +      BT_LOGR("State of a volume is not STATE_MOUNTED, abort OPP connection if any.");
> > +      Disconnect(nullptr);
> > +      DeleteReceivedFile();
> > +      FileTransferComplete();
> 
> I though it should be the same, but it's worthy confirming again.
OK,
I will confirm whether the two functions (DeleteReceivedFile(), FileTransferComplete() ) can be actually covered by function Disconnect() in both code logic and experiment.

thanks!

Comment 55

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Hi Ben Tian

This is the enhanced patch, which 
1. simplify logic, remove unnecessary function calls
2. rewrite comment
3. add checking connection status
4. was tested for both server and client cases.

may you help to review?

thanks! :)

Comment 56

[Alphan Chen [:alchen]]

Comment on attachment 8559689 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +83,5 @@
> +      return NS_OK;
> +    }
> +
> +    // Handle unexpected removal of any storage here since
> +    // sdcard MountLock only handles device storage shared mode.

This is not that accurate.
MountLock not just handle shared mode.
It can handle all the case in gecko.
However, in this case, it is an physical removal.
That's why we need to handle in this way.

@@ +89,5 @@
> +    vol->GetState(&state);
> +    if (nsIVolume::STATE_MOUNTED == state) {
> +      BT_LOGR("Volume state is STATE_MOUNTED.");
> +      return NS_OK;
> +    }

I still don't know why we need to keep this.

Comment 57

[Ben Tian (inactive)]

Comment on attachment 8559689 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

Almost there. Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +75,5 @@
>                               const char16_t* aData)
>  {
>    MOZ_ASSERT(sBluetoothOppManager);
>  
> +  // if state of any volume was changed

Explain why we disconnect OPP connection no matter which volume becomes non STATE_MOUNTED.

@@ +77,5 @@
>    MOZ_ASSERT(sBluetoothOppManager);
>  
> +  // if state of any volume was changed
> +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject);

Suggest to wrap this section as a function |HandleVolumeStateChanged|.

@@ +87,5 @@
> +    // sdcard MountLock only handles device storage shared mode.
> +    int32_t state;
> +    vol->GetState(&state);
> +    if (nsIVolume::STATE_MOUNTED == state) {
> +      BT_LOGR("Volume state is STATE_MOUNTED.");

I prefer BT_LOGD to avoid excessive logs for normal cases.

@@ +92,5 @@
> +      return NS_OK;
> +    }
> +
> +    // as above, state is now changed to non STATE_MOUNTED,
> +    // let's check we are connected or not, then disconnect if any.

Condense the comment as following:

  // Disconnect any connected OPP connection since volume state becomes non STATE_MOUNTED

@@ +95,5 @@
> +    // as above, state is now changed to non STATE_MOUNTED,
> +    // let's check we are connected or not, then disconnect if any.
> +    if (mConnected) {
> +      BT_LOGR("Volume state is not STATE_MOUNTED. Abort any ongoing OPP connection.");
> +      Disconnect(nullptr);

Leave comment to brief that |OnSocketDisconnect| would handle remaining files to send/receive.

Comment 58

[Ben Tian (inactive)]

(In reply to Alphan Chen[:Alphan] from comment #56)
> > +    // Handle unexpected removal of any storage here since
> > +    // sdcard MountLock only handles device storage shared mode.
> 
> This is not that accurate.
> MountLock not just handle shared mode.
> It can handle all the case in gecko.
> However, in this case, it is an physical removal.
> That's why we need to handle in this way.

Alphan, can you suggest more accurate comment here? Thanks.

> @@ +89,5 @@
> > +    vol->GetState(&state);
> > +    if (nsIVolume::STATE_MOUNTED == state) {
> > +      BT_LOGR("Volume state is STATE_MOUNTED.");
> > +      return NS_OK;
> > +    }
> 
> I still don't know why we need to keep this.

I think it's guardian clause here to filter out normal case.

Comment 59

[Alphan Chen [:alchen]]

(In reply to Ben Tian [:btian] from comment #58)
> (In reply to Alphan Chen[:Alphan] from comment #56)
> > > +    // Handle unexpected removal of any storage here since
> > > +    // sdcard MountLock only handles device storage shared mode.
> > 
> > This is not that accurate.
> > MountLock not just handle shared mode.
> > It can handle all the case in gecko.
> > However, in this case, it is an physical removal.
> > That's why we need to handle in this way.
> 
> Alphan, can you suggest more accurate comment here? Thanks.

I will write something like below.

"Here is for unexpected physical removal of any storage.
By checking the volume state change, we should release the storage which is unavailable to prevent being killed by vold."
> 
> > @@ +89,5 @@
> > > +    vol->GetState(&state);
> > > +    if (nsIVolume::STATE_MOUNTED == state) {
> > > +      BT_LOGR("Volume state is STATE_MOUNTED.");
> > > +      return NS_OK;
> > > +    }
> > 
> > I still don't know why we need to keep this.
> 
> I think it's guardian clause here to filter out normal case.

Comment 60

[Alphan Chen [:alchen]]

(In reply to Ben Tian [:btian] from comment #58)
> Alphan, can you suggest more accurate comment here? Thanks.
> 
> > @@ +89,5 @@
> > > +    vol->GetState(&state);
> > > +    if (nsIVolume::STATE_MOUNTED == state) {
> > > +      BT_LOGR("Volume state is STATE_MOUNTED.");
> > > +      return NS_OK;
> > > +    }
> > 
> > I still don't know why we need to keep this.
> 
> I think it's guardian clause here to filter out normal case.

I think here we should care the volume state only when there is a connection.
So I will prefer only check the state when there is a connection.
I also think Will should use "BT_LOGD" for STATE_MOUNTED case.

if (mConnected)
{
   if (nsIVolume::STATE_MOUNTED == state)
   {
       BT_LOGD(...)
       return;
   } else {
       //disconnect the connection
   }
}

Comment 61

[Ben Tian (inactive)]

You're right. |mConnected| appears more selective to me so we can revise as below to save volume interface & state query. How do you guys think?

  if (!mConnected) {
    return;
  }

  // get volume interface and state
  ...
  
  if (state is not MOUNTED) {
    Disconnect();
  }

(In reply to Alphan Chen[:Alphan] from comment #60)
> if (mConnected)
> {
>    if (nsIVolume::STATE_MOUNTED == state)
>    {
>        BT_LOGD(...)
>        return;
>    } else {
>        //disconnect the connection
>    }
> }

Comment 62

[Alphan Chen [:alchen]]

I'm fine with that.

(In reply to Ben Tian [:btian] from comment #61)
> You're right. |mConnected| appears more selective to me so we can revise as
> below to save volume interface & state query. How do you guys think?
> 
>   if (!mConnected) {
>     return;
>   }
> 
>   // get volume interface and state
>   ...
>   
>   if (state is not MOUNTED) {
>     Disconnect();
>   }
> 
> (In reply to Alphan Chen[:Alphan] from comment #60)
> > if (mConnected)
> > {
> >    if (nsIVolume::STATE_MOUNTED == state)
> >    {
> >        BT_LOGD(...)
> >        return;
> >    } else {
> >        //disconnect the connection
> >    }
> > }

Comment 63

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Dear Ben Tian

I had already made a new patch based on your feedback,
please help to review :)

thanks!

Comment 64

[Ben Tian (inactive)]

Comment on attachment 8562729 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

r=me with comment addressed. Thanks.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +76,5 @@
>  {
>    MOZ_ASSERT(sBluetoothOppManager);
>  
> +  // We will disconnect OPP connection no matter which volume
> +  // becomes non STATE_MOUNTED, since user may select files which are

Simplify by removing 'which are'

@@ +77,5 @@
>    MOZ_ASSERT(sBluetoothOppManager);
>  
> +  // We will disconnect OPP connection no matter which volume
> +  // becomes non STATE_MOUNTED, since user may select files which are
> +  // scattered in multiple volume in a single transfer, (e.g., user select 2

nit: multiple volume's' ... user select's'

@@ +78,5 @@
>  
> +  // We will disconnect OPP connection no matter which volume
> +  // becomes non STATE_MOUNTED, since user may select files which are
> +  // scattered in multiple volume in a single transfer, (e.g., user select 2
> +  // files to transfer, one is in internal volume and another is in external

It seems more conventional to use 'the other' rather than 'another' here since only 2 files are mentioned.

@@ +82,5 @@
> +  // files to transfer, one is in internal volume and another is in external
> +  // volume).
> +
> +  // if state of any volume was changed
> +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {

Wrap this section and comment above into a function |HandleVolumeStateChanged|, like |HandleShutdown|.

  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED) {
    HandleVolumeStateChanged();
    return NS_OK;
  }

===

BluetootOppManager::HandleVolumeStateChanged()
{
  // We will disconnect OPP connection no matter which volume
  ...
}

@@ +98,5 @@
> +    vol->GetState(&state);
> +
> +    if (nsIVolume::STATE_MOUNTED != state) {
> +      // Here is for unexpected physical removal of any storage.
> +      // By checking the volume state change, we should release the storage 

nit: remove trailing space.

Also simplify 'the storage which is unavailable' to 'the unavailable storage.'

@@ +102,5 @@
> +      // By checking the volume state change, we should release the storage 
> +      // which is unavailable to prevent being killed by vold.
> +
> +      // Disconnect any connected OPP connection since volume state becomes
> +      // non STATE_MOUNTED. Then |OnSocketDisconnect| would also be called to 

nit: remove trailing space.

@@ +103,5 @@
> +      // which is unavailable to prevent being killed by vold.
> +
> +      // Disconnect any connected OPP connection since volume state becomes
> +      // non STATE_MOUNTED. Then |OnSocketDisconnect| would also be called to 
> +      // handle remaining files during send(DiscardBlobsToSend) or 

nit: remove trailing space.

@@ +109,5 @@
> +      BT_LOGR("Volume state is not STATE_MOUNTED. Abort any ongoing OPP connection.");
> +      Disconnect(nullptr);
> +    }
> +
> +    return NS_OK; 

nit: remove trailing space.

Comment 65

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Dear Ben Tian

Here is the enhanced patch based on your feedback,
mainly it is for method extraction,
please help to review,
thanks! :)

Comment 66

[Ben Tian (inactive)]

Comment on attachment 8564058 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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

r=me with nits addressed.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +77,5 @@
>    MOZ_ASSERT(sBluetoothOppManager);
> +  // if state of any volume was changed
> +  if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    HandleVolumeStateChanged(aSubject);
> +    return NS_OK; 

nit: remove trailing space.

@@ +312,5 @@
> +  // get volume interface and state
> +  nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject);
> +  if (!vol) {
> +    return;
> +  }

nit: newline here.

Comment 67

[Will Wang [:WillWang]]

remove keyword:checkin-needed since I want to perform more test on try server.

Comment 68

[Will Wang [:WillWang]]

Add keyword:checkin-needed since tests on try server were done:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5081ee549369

thanks :)

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/759cfab73c49

Comment 70

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/759cfab73c49

Comment 71

[Will Wang [:WillWang]]

Comment on attachment 8564058 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

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 #): none
User impact if declined: SD card cannot be used anymore after unexpectedly ejected.

Testing completed: 
Yes, tested in 2.1s as above comments.
(However, under 2.1(b2g34), currently this issue can not be reproduced and tested,
since there is no corresponding phone which have necessary hardware component(detect pin for SD card ejection/plug)).

Risk to taking this patch (and alternatives if risky): no

String or UUID changes made by this patch: none

Comment 72

[howie [:howie]]

This is general issue should be 2.1+

Comment 73

[Will Wang [:WillWang]]

Comment on attachment 8564058 [details] [diff] [review]
0001-Bug-1118177-Release-file-resources-if-users-unexpect.patch

Note: FYI, Following [Approval Request Comment] is identical to comment 71.

----

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 #): none
User impact if declined: SD card cannot be used anymore after unexpectedly ejected.

Testing completed: 
Yes, tested in 2.1s as above comments.
(However, under 2.1(b2g34), currently this issue can not be reproduced and tested,
since there is no corresponding phone which have necessary hardware component(detect pin for SD card ejection/plug)).

Risk to taking this patch (and alternatives if risky): no

String or UUID changes made by this patch: none

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a6cd1ca5ebbc
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/832fd2e07aeb

Comment 75

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/832fd2e07aeb

Comment 76

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on latest Flame v3.0.
See attachment: verified_v3.0.mp4.
Reproduce rate: 0/5.

STR:
1.Open Gallery app.
2.Select several pictures to share by bluetooth.
3.In this moment, remove SD card suddenly.
4.After several seconds, insert this SD card into phone.
**The SD card is recognized and user can continue to browse pictures(or songs in Music app).

Flame 3.0 build:
Build ID               20150305072141
Gaia Revision          0017f2bbc63781a5409644b664d80ebaa1543653
Gaia Date              2015-03-05 00:46:59
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/993eb76a8bd6
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150305.160341
Firmware Date          Thu Mar  5 16:03:51 EST 2015
Bootloader             L1TC000118D0

Comment 77

[Shally Li（leave from Mozilla）]

Attachment: verified_v3.0.MP4

Comment 78

[Will Wang [:WillWang]]

Hi Shally

Thanks for your verify and video!
Please let me confirm:

(In reply to Shally from comment #76)

> STR:
> 3.In this moment, remove SD card suddenly.
> 4.After several seconds, insert this SD card into phone.
> **The SD card is recognized and user can continue to browse pictures(or
> songs in Music app).

For the step 3,4 above, it seems that Flame didn't show notification for SD card removal and insertion, right?
did you see transfer fail(or success) in receiving phone?

besides, 
are those selected pictures located in SD card?

thanks!

Comment 79

[Shally Li（leave from Mozilla）]

Hi Will,

    I am sorry to provide the steps incompletely.

Update STR:
(I have deleted all pictures in Phone storage)
1.Open Gallery app.
2.Select several pictures in SD card and share by bluetooth.
3.In this moment, remove SD card suddenly.
**Before pictures share completely,the sharing pictures will stop sending (show "Sending Bluetooth transfer..." on notification bar) after I remove SD card.

4.After several seconds, insert this SD card into phone.
**The sharing pictures can't continue to send.
**If you tap Cancel and share new pictures from SD card,you can find SD card is recognized and share pictures successfully.
-----------------------------------------------------------------------------
(In reply to Will Wang [:WillWang] from comment #78)
> For the step 3,4 above, it seems that Flame didn't show notification for SD
> card removal and insertion, right?

Yes,Flame didn't show notification about SD card removal and insertion. 


> did you see transfer fail(or success) in receiving phone?

Yes,other device will show fail(or success) prompt messages.


> are those selected pictures located in SD card?

Yes.


Thank you for your reply.

Comment 80

[Shally Li（leave from Mozilla）]

Attachment: update_verified_v3.0.MP4

Comment 81

[Shally Li（leave from Mozilla）]

Hi Will & Reporter,

    Could you help confirm whether the steps and results(SD card can be used again after removing and inserting SD card without reboot) of Comment 79 are correct? 
    I think device shows the notifications about SD card removal and insertion is another bug,is right?

Thank you very much.

Comment 82

[ben.song]

clear ni for me

Comment 83

[SandKing]

Attachment: Verified video: verified video.3GP

This bug has been verified on latest nightly build of Flame v2.1 and Flame v2.2.
STR: 
Same STR as comment79,
Actually result:
**The sharing pictures can't continue to send.
**If you tap Cancel and share new pictures from SD card,you can find SD card is recognized and share pictures successfully.

See video:"verified video.3GP"
Repro rate: 0/10

Device info:
Flame 2.1(Pass):
Build ID               20150614001205
Gaia Revision          f8b848c82d1ed589f7a1eb5cc099830c867ff1d4
Gaia Date              2015-06-08 09:48:23
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/7d767fc15126
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150614.033810
Firmware Date          Sun Jun 14 03:38:22 EDT 2015
Bootloader             L1TC000118D0

Flame 2.2(pass):
Build ID               20150614002504
Gaia Revision          cfceba16e48ede3defee24be93637a0fa291c494
Gaia Date              2015-06-11 22:10:18
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2cfd86c2ba1b
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150614.040237
Firmware Date          Sun Jun 14 04:02:49 EDT 2015
Bootloader             L1TC000118D0