869259 - [Bluetooth] when doing OPP file transfering, VOLD kills b2g chrome process after plug-in USB cable with USB Mass Storage on.

Status: VERIFIED FIXED

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

Description

[leo.bugzilla.gecko]

STR>
1) Settings - Media stroage - USB mass stroage checked
2) Device A(B2G Phone) and Device B(Other`s Phone) Paired by BlueTooth
3) File(mp3 10ea) transfer B to A(B2G Phone)
4) plug in usb cable on Device A(B2G Phone)
5) restart Device A(B2G Phone)

in during file transfer, 
at this time, user plug-in usb cable. automounter sends command signal to unmount /mnt/sdcard.
and then, VOLD will try to unmount /mnt/sdcard, but currently, b2g has a opened file handle by file transfer. 

So, To unmount /mnt/sdcard, VOLD kills process which has a file handle.
at this case, process whics has a file handle is B2G process. so SYSTEM will be restarted by this operation.

05-06 14:09:29.222 I       10900    Vold            /mnt/secure/staging/.android_secure sucessfully unmounted
05-06 14:09:29.222 I       10900    Vold            /mnt/secure/asec sucessfully unmounted
05-06 14:09:29.222 W       10900    Vold            Failed to unmount /mnt/secure/staging (Device or resource busy, retries 2, action 1)
05-06 14:09:29.272 E       10900    ProcessKiller   Process /system/b2g/b2g (10881) has open file /mnt/secure/staging/downloads/bluetooth/031_Zion.T_Babay _Feat. Gaeko_-1.mp3
05-06 14:09:29.272 W       10900    ProcessKiller   Sending SIGHUP to process 10881

Comment 1

[Jason Smith [:jsmith]]

qawanted for what purpose? A reproduction?

Also - leo, can you provide a crash report URL with your bug? It will help in figuring out the problem here.

Comment 2

[jeremy.kim.leo [:jeremykimleo]]

exactly say, it is not a crash. (and one more, leo never send a crash report yet, because socoro and leo mismatchs debugging information)
to share usb mass-storage when device connect usb cable, VOLD kills process which is opened file on /mnt/sdcard. at this case, the process is b2g because of transfering by Bluetooth.

when kills b2g process, system will be restarted.

repo is very simply.

Comment 3

[Wayne Chang [:wchang]]

Triage 5/8
Hi Leo, Can you provide logs for the crash and also any support information for the behaviour mentioned in comment 2?

Comment 4

[Wayne Chang [:wchang]]

ni? reporter.

Comment 5

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

Thanks for comment on, jeremyofleo. It is clear that this problem we need to deal with. In vold Process.cpp (Process::killProcessesWithOpenFiles), sends kill signal.

Comment 6

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

Clean needinfo status, because it's nothing to do b2g crash. But VOLD sends kill signal.

Comment 7

[Walter Chen[:ypwalter][:wachen]]

I tried on this bug.
Current leo device we have here cant reproduce that.
It would show "No memory card found" before started to transfer file. (while you can still read imgs in sd card in galley app)

However, I did reproduce this with unagi. B2G would get killed and restarted.

Comment 8

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

Just quick note, maybe we need to add observer for "NS_VOLUME_STATE_CHANGED" and do error handling?

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking since this is a reproducible system reboot issue.

Comment 10

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

Set checkpoint on 14 May. This will be highest priority after 1.0.1 certification done.

Comment 11

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

Set milestone

Comment 12

[Dave Hylands [:dhylands]]

One way that this could be fixed would be for the Bluetooth file transfer code to acquire a VolumeMountLock.

This will prevent the volume from being shared with the PC while the file transfer is active. We currently use this mechanism while downloading updates.

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#805

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#834

Comment 13

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

Thanks for the hint, Dave.

Comment 14

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

This bug was differed a little bit due to recently storage related code change. It breaks OPP function. But now patch of Bug 872238 had been committed. So I can now submit my patch for mount lock.

Comment 15

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

Attachment: Patch1:V1 for b2g18

Comment 16

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

Attachment: Patch1:v1 for central

Update patch for central

Comment 17

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

How to test:
Scenario:
1. Setup OPP file transfer session
2. Switch to PC share with USB mode
3. Verify b2g process will be crashed

Comment 18

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 752113 [details] [diff] [review]
Patch1:v1 for central

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

The idea is good, although some changes would be necessary, especially we need error handling if BluetoothOppManager couldn't get the lock. I'm happy to review again when the patch is ready.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +27,5 @@
>  #include "nsIInputStream.h"
>  #include "nsIMIMEService.h"
>  #include "nsIOutputStream.h"
>  #include "nsNetUtil.h"
> +#include "nsIVolumeService.h"

super-nit: alphabetic order, please

@@ +1224,5 @@
>    name.AssignLiteral("contentType");
>    v = sContentType;
>    parameters.AppendElement(BluetoothNamedValue(name, v));
> +  //Release Mount lock if file transfer completed
> +  ReleaseSdcardMountLock();

This should be placed in another function. AfterOppDisconnected() is a good choice.

@@ +1266,5 @@
> +  // KILL signal to kill b2g process.
> +  // If OPP file transfer is ongoing, it's possible to switch
> +  // to Sdcard disk mode. After OPP transcation were done,
> +  // lock will be freed.
> +  AcquireSdcardMountLock();

This should be placed in another function. AfterOppConnected() is a good choice.

@@ +1463,5 @@
>    }
>  }
>  
> +void
> +BluetoothOppManager::AcquireSdcardMountLock()

Since there are many operations may fail in the function, we should return bool/nsreult and do error handling at the point we call this function.

Comment 19

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

Ah, I did not see AfterOppDisconnected/AfterOppDisconnected when I wrote this patch.
It just came to me a question, when shall we hold and release the mount lock.
Shall we hold the lock immediately after OPP get connected or hold the lock after starting file transfer?

> @@ +1266,5 @@
> > +  // KILL signal to kill b2g process.
> > +  // If OPP file transfer is ongoing, it's possible to switch
> > +  // to Sdcard disk mode. After OPP transcation were done,
> > +  // lock will be freed.
> > +  AcquireSdcardMountLock();
> 
> This should be placed in another function. AfterOppConnected() is a good
> choice.
> 
> @@ +1463,5 @@
> >    }
> >  }
> >  
> > +void
> > +BluetoothOppManager::AcquireSdcardMountLock()
> 
> Since there are many operations may fail in the function, we should return
> bool/nsreult and do error handling at the point we call this function.
If we fail to get mount lock, what kind of error handling we can do except for doing ASSERT or print ns_warning? It seems we can discuss more here.

Comment 20

[Dave Hylands [:dhylands]]

Comment on attachment 752113 [details] [diff] [review]
Patch1:v1 for central

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

I threw in a couple of review comments as well.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +1262,5 @@
>    name.AssignLiteral("contentType");
>    v = sContentType;
>    parameters.AppendElement(BluetoothNamedValue(name, v));
> +  // Get mount lock in case VOID (Volume Manager) sends
> +  // KILL signal to kill b2g process.

Actually, I'd also say that this comment isn't correct. vold killing us is just a side effect, not the main reason we're grabbing the lock.

It should read something like:

Get a mount lock to prevent the sdcard from being shared with the PC while we're doing a file transfer.

@@ +1477,5 @@
> +
> +void
> +BluetoothOppManager::ReleaseSdcardMountLock()
> +{
> +  MOZ_ASSERT(mMountLock);

You shouldn't assert here, since you're allowing mMountLock to be NULL after calling AcquireSdMountLock. If you're going to NOT call ReleaseSdcardMountLock if the Acquire fails, then its OK to leave the asert, but if you are going to call it, then this needs to be changed to an if check.

Comment 21

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

Attachment: Patch 1-V2 for Bug 869259

Add extra error handling for unable to get a mount lock.

Comment 22

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 754441 [details] [diff] [review]
Patch 1-V2 for Bug 869259

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

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +472,5 @@
>    AfterFirstPut();
> +  // Get a mount lock to prevent the sdcard from being shared with
> +  // the PC while we're doing a OPP file transfer.
> +  // After OPP transcation were done,
> +  // the mount lock will be freed.

super-nit: one line should be fine. No need to wrap.

@@ +477,5 @@
> +  if (!AcquireSdcardMountLock()) {
> +    // If we fail to get a mount lock, abort this transaction
> +    // Directly sending disconnect-request is better than abort-request
> +    NS_WARNING("BluetoothOPPManager couldn't get a mount lock!");
> +    SendDisconnectRequest();

This works only if we are OPP client. When our role is OPP server, if we send DISCONNECT as a server response to CONNECT req, it should be an undefined behavior. Instead of sending DISCONNECT, closing current socket directly would be cleaner. Let's do:

if (!AcquireSdcardMountLock()) {
  NS_WARNING("BluetoothOPPManager couldn't get a mount lock!");

  Disconnect();
  Listen();
}

@@ +479,5 @@
> +    // Directly sending disconnect-request is better than abort-request
> +    NS_WARNING("BluetoothOPPManager couldn't get a mount lock!");
> +    SendDisconnectRequest();
> +  }
> +

nit: extra line

@@ +512,5 @@
>      mReadFileThread = nullptr;
>    }
> +  //Release Mount lock if file transfer completed
> +  ReleaseSdcardMountLock();
> +

nit: extra line

Comment 23

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

Attachment: Patch1:V3 for Bug869259

Update and fix previous mistakes

Comment 24

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 757812 [details] [diff] [review]
Patch1:V3 for Bug869259

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

Thanks for being patient! Please have me review again once revision is done. :)

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +493,5 @@
>      mReadFileThread->Shutdown();
>      mReadFileThread = nullptr;
>    }
> +  //Release the Mount lock if file transfer completed
> +  ReleaseSdcardMountLock();

Since we don't really care about the return value of ReleaseSdcardMountLock(), let's just do

if (mMountLock) {
  mMountLock->Unlock();
}

here. This way we can get rid of ReleaseSdcardMountLock().

@@ +1509,5 @@
> +BluetoothOppManager::AcquireSdcardMountLock()
> +{
> +  nsCOMPtr<nsIVolumeService> volumeSrv =
> +    do_GetService(NS_VOLUMESERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(volumeSrv, NS_ERROR_FAILURE);

Return value should be a bool.

@@ +1521,5 @@
> +bool
> +BluetoothOppManager::ReleaseSdcardMountLock()
> +{
> +  if (mMountLock) {
> +    return mMountLock->Unlock();

Another thing you have to do is null out mMountLock, otherwise it would be a leak.

::: dom/bluetooth/BluetoothOppManager.h
@@ +13,5 @@
>  #include "DeviceStorage.h"
>  #include "mozilla/dom/ipc/Blob.h"
>  #include "mozilla/ipc/UnixSocket.h"
>  #include "nsCOMArray.h"
> +#include "nsIVolumeMountLock.h"

Forward declaration, please.

Comment 25

[Dave Hylands [:dhylands]]

(In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE) from comment #24)
> Comment on attachment 757812 [details] [diff] [review]
> Patch1:V3 for Bug869259
...snip...
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +493,5 @@
> >      mReadFileThread->Shutdown();
> >      mReadFileThread = nullptr;
> >    }
> > +  //Release the Mount lock if file transfer completed
> > +  ReleaseSdcardMountLock();
> 
> Since we don't really care about the return value of
> ReleaseSdcardMountLock(), let's just do
> 
> if (mMountLock) {
>   mMountLock->Unlock();
> }

You can just do:

mMountLock = nullptr;

That will Unlock if the pointer is non-null.

> @@ +1521,5 @@
> > +bool
> > +BluetoothOppManager::ReleaseSdcardMountLock()
> > +{
> > +  if (mMountLock) {
> > +    return mMountLock->Unlock();
> 
> Another thing you have to do is null out mMountLock, otherwise it would be a
> leak.

Just do:

mMountLock = nullptr;

Comment 26

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

Attachment: Patch1:V4 for Bug 869259

revised version 4: Update and fix errors

Comment 27

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 759013 [details] [diff] [review]
Patch1:V4 for Bug 869259

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

r=me with nits addressed. Thanks.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +492,5 @@
>    if (mReadFileThread) {
>      mReadFileThread->Shutdown();
>      mReadFileThread = nullptr;
>    }
> +  //Release the Mount lock if file transfer completed

super-nit: please insert a blank between '/' and the first letter.

@@ +494,5 @@
>      mReadFileThread = nullptr;
>    }
> +  //Release the Mount lock if file transfer completed
> +  if (mMountLock) {
> +    //implicitly this will unlock

nit: The first letter should be capitalized.
super-nit: please insert a blank between '/' and the first letter.

::: dom/bluetooth/BluetoothOppManager.h
@@ +112,5 @@
>    void ClearQueue();
>    void RetrieveSentFileName();
>    void NotifyAboutFileChange();
> +  bool AcquireSdcardMountLock();
> +  bool ReleaseSdcardMountLock();

nit: we don't need this anymore

Comment 28

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

Attachment: Bug869259: Get a mount lock to prevent the sdcard from being shared with the PC, r=echou

Update and fix nit.

Comment 29

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

Attachment: Bug869259, b2g18 version: Get a mount lock to prevent the sdcard from being shared with the PC, r=echou

Update b2g18 version.

Comment 30

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

https://tbpl.mozilla.org/?tree=Try&rev=3c1aaf750afd

Comment 31

[Eric Chou [:ericchou] [:echou]]

birch: https://hg.mozilla.org/projects/birch/rev/740e7a18cd39

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/740e7a18cd39

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b55241345c51

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/b55241345c51

Comment 35

[Parul Mathur]

Verified fixed on:
Device: Leo phone 
Build Identifier: 20130611074722
Update channel: leo/1.1.0/nightly
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8d0562d20324
Gaia: 8c64e19b82d4e0490a7780232d3d6bd07d0ba9ec1370937291
Git commit info: 2013-06-11 07:54:51 
OS version: 1.1.0.0-prerelease

Created test case in MozTrap: https://moztrap.mozilla.org/manage/case/8592/