876782 - AutoMounter shouldn't unmount so agressively

Status: RESOLVED FIXED

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

Description

[Dave Hylands [:dhylands]]

Currently when the AutoMounter tries to unmount a volume, it uses the force option. This in turn causes vold to send a SIGHUP signal to processes which have files open on the volume in question, and eventually kill them. I think that currently, processes may be getting killed when the SIGHUP is being sent.

Times when the volume gets unmounted:
1 - When USB Mass Storage is enabled and the phone is plugged into the PC
2 - When the SD card is ejected from the phone

For case 1, the AutoMounter should detect that there are still files open and send an event of some type to the app in question so that it can have a chance to do something intelligent.

For case 2, The AutoMounter should also send an event. The files will be gone since the sdcard was ejected, but the app should at least be notified that this happened before the automounter initiates the unmount.

Comment 1

[leo.bugzilla.gecko]

This issue makes so many related issues.
If unmount happens during scanning files (in music, gallery, video), it usually makes app crash, media server crash, and various abnormal running.
It's very annoying to end-user and easy to find those cases while using the phone.
Because of that, I think this issue is very critical to end user.

Comment 2

[leo.bugzilla.gecko]

Especially, if many files are stored in sdcard, it happens many times.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Is this bug a duplicate of Bug 871383?? I had reported that bug few weeks ago. 

Since this bug had became leo+, if they are duplicate, I suggest to close Bug 871383 and keep this one.

Comment 5

[Dave Hylands [:dhylands]]

OK - I went and marked bug 871383 as a duplicate. I couldn't find it when I went looking the other day.

Comment 6

[Dave Hylands [:dhylands]]

Attachment: WIP: Don't unmount if there are open files

With the patch as supplied, the automounter will check for open files, and if it finds any, it will add a log entry for each open file, and fail to unmount.

This will leave the UI in an inconsistent state, since the UI doesn't know that enabling USB Mass Storage can fail.

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 762848 [details] [diff] [review]
WIP: Don't unmount if there are open files

Since this is still a WIP, I'm just asking for feedback on what's here so far, realizing that this is probably not yet the complete solution, but at least got the core bits.

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 762848 [details] [diff] [review]
WIP: Don't unmount if there are open files

This looks pretty sane to me!

Comment 9

[Wayne Chang [:wchang]]

Resetting milestone as unrealistic with Dave on PTO.

Comment 10

[Wayne Chang [:wchang]]

Hi Dave,
Are you on top of this one still?

Thanks

Comment 11

[Dave Hylands [:dhylands]]

Yep - it's all coded. I'm doing a couple of cleanups and then I'll find somebody to review it.

Comment 12

[Dave Hylands [:dhylands]]

Attachment: Added timer to check every 5 seconds while files are preventing the unmount.

Defers unmount until no files are open on the sdcard.
when a unmount is deferred, the automounter will check every 5 seconds to see if files are still open.

Comment 13

[Justin Lebar (not reading bugmail)]

Are you planning to get a DOM peer to look at this as well?

Comment 14

[Dave Hylands [:dhylands]]

(In reply to Justin Lebar [:jlebar] from comment #13)
> Are you planning to get a DOM peer to look at this as well?

I haven't in the past.

As far I can tell, this particular patch doesn't change anything DOM related.

I'd be happy to get someone else to review. Who would you recommend?

Comment 15

[Justin Lebar (not reading bugmail)]

> As far I can tell, this particular patch doesn't change anything DOM related.

Indeed it doesn't, even though it's in the dom directory.

I guess the question I should have asked is, have we anointed people to own/review this code, or are we doing it ad-hoc?  And I can answer that question:  Looking through the blame, this looks like mainly you and qDot.

Is this part of a larger piece of code that we can make you guys official owners of?

Comment 16

[Dave Hylands [:dhylands]]

The AutoMounter and VolumeXxx files in the dom/system/gonk directory are primarily used for USB Mass Storage, although some of the VolumeXxx stuff is also used by DeviceStorage.

I am the primary author of the AutoMounter and VolumeXxx stuff. cjones did the initial review, and I've been getting qDot to do most of the reviews since then (since it's primarily gonk specific).

I'm currently the owner of DeviceStorage, and it would make sense for the AutoMounter/VolumeManager to fall under that (or perhaps under a new module - I'm ambivalent).

Comment 17

[Justin Lebar (not reading bugmail)]

Okay, thanks for clarifying!

Comment 18

[Hema Koka [:hema]]

Kyle,

Please review Dave's changes so we can wrap this up.

Thanks!
Hema

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 771508 [details] [diff] [review]
Added timer to check every 5 seconds while files are preventing the unmount.

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

r-'ing just for another go-round on the sscanf. Honestly, if it works and it comes out fine, just fix nits, resubmit and let me know.

::: dom/system/gonk/AutoMounter.cpp
@@ +411,5 @@
> +
> +          // Check to see if there are any open files on the volume and
> +          // don't initiate the unmount while there are open files.
> +          OpenFileFinder::Info fileInfo;
> +          OpenFileFinder  fileFinder(vol->MountPoint());

Nit: extra space

::: dom/system/gonk/OpenFileFinder.cpp
@@ +126,5 @@
> +  char *stat = statString.BeginWriting();
> +  if (stat) {
> +    ReadSysFile(statPath.get(), stat, statString.Length());
> +    int ppid;
> +    if (sscanf(stat, "%*d (%16[^)]) %*c %d ", comm, &ppid) == 2) {

Hmm. Wondering if there's a better way we can do this. There's tons of sscanf/PR_sscanf's around the code, but lots of grumbling on #developers about them? Not r-'ing on this, just curious.

::: dom/system/gonk/OpenFileFinder.h
@@ +20,5 @@
> +    NEXT_PID,
> +    CHECK_FDS,
> +    DONE
> +  };
> +  class Info

Nit?: struct? (I don't know if we have standards on this)

Comment 20

[Dave Hylands [:dhylands]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #19)
> Comment on attachment 771508 [details] [diff] [review]
> Added timer to check every 5 seconds while files are preventing the unmount.
> 
> Review of attachment 771508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-'ing just for another go-round on the sscanf. Honestly, if it works and it
> comes out fine, just fix nits, resubmit and let me know.
> 
> ::: dom/system/gonk/OpenFileFinder.cpp
> @@ +126,5 @@
> > +  char *stat = statString.BeginWriting();
> > +  if (stat) {
> > +    ReadSysFile(statPath.get(), stat, statString.Length());
> > +    int ppid;
> > +    if (sscanf(stat, "%*d (%16[^)]) %*c %d ", comm, &ppid) == 2) {
> 
> Hmm. Wondering if there's a better way we can do this. There's tons of
> sscanf/PR_sscanf's around the code, but lots of grumbling on #developers
> about them? Not r-'ing on this, just curious.

So your nit made me think about this some more, and I realized that it won't work properly if the comm field contains a ) character in it. This would happen if a process with a ) in the name was launched (extremely unlikely) or if an app with a ) in the name (less likely, but still a possibility).

There actually is a process called "(Preallocated process)" but the ) is beyond character 16 so it doesn't get included.

I'll fix this deal with arbitrary comm, and by then it won't be worth using the sscanf anyore.

> 
> ::: dom/system/gonk/OpenFileFinder.h
> @@ +20,5 @@
> > +    NEXT_PID,
> > +    CHECK_FDS,
> > +    DONE
> > +  };
> > +  class Info
> 
> Nit?: struct? (I don't know if we have standards on this)

In previous reviews, I've coded struct and gotten nits to change it to class, so I think its reviewer specific.

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Dave Hylands [:dhylands] from comment #20)

> > ::: dom/system/gonk/OpenFileFinder.h
> > @@ +20,5 @@
> > > +    NEXT_PID,
> > > +    CHECK_FDS,
> > > +    DONE
> > > +  };
> > > +  class Info
> > 
> > Nit?: struct? (I don't know if we have standards on this)
> 
> In previous reviews, I've coded struct and gotten nits to change it to
> class, so I think its reviewer specific.

Ok, yeah, that's why I nit?'d. I'll go ahead and change my opinion to class on that too, at least for moz code. I can understand the want for consistency there.

Comment 22

[Dave Hylands [:dhylands]]

Attachment: Reworked sscanf to work, even when

Comment 23

[Dave Hylands [:dhylands]]

Attachment: Reworked sscanf to work, even when comm has ). Fixed nit

Bah - forgot to fix the extra space nit.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 776593 [details] [diff] [review]
Reworked sscanf to work, even when comm has ). Fixed nit

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

::: dom/system/gonk/OpenFileFinder.cpp
@@ +45,5 @@
> +  while (mState != DONE) {
> +    switch (mState) {
> +      case NEXT_PID: {
> +        struct dirent *pidEntry;
> +        pidEntry = readdir(mProcDir);

nit: Should probably comment somewhere that we're guaranteed to not block since we're hitting the proc filesystem

Comment 25

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/projects/birch/rev/d556762ff012

Comment 26

[Ed Morley [:emorley]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/38a66e499ca8

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage. Looks like this'll need a branch-specific patch for uplift.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8826d586d644

https://tbpl.mozilla.org/php/getParsedLog.php?id=25391760&tree=Mozilla-B2g18

Comment 29

[Dave Hylands [:dhylands]]

Attachment: Additional patch to apply to b2g18

ReadSysFile was factored into FileUtils.h on m-c, and that change wasn't backported to b2g18. Since the ReadSysFile variant used by OpenFileFinder.cpp is available in AutoMounter.cpp, we just expose that function.

Comment 30

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4de723654987
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1be4281f3d27

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/4de723654987
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1be4281f3d27

Comment 32

[Alex Keybl [:akeybl]]

I don't think we need the keyword on here anymore if resolved on branches

Comment 33

[leo.bugzilla.gecko]

storage rescan error after unplug UMS 

Precondition :  Have songs and videos stored on both memories (internal and external). Enable 
sharing all memories.

Test Action : 1) Open music & video. and check file list.
              2) Go to home. and plug USB.
              3) After a while (30 sec) unplug the cable
              4) Go to Music and check 'All Songs' tab view.

Detailed Symptom : only internal SD file list show.

Expected : music show internal and external file list
Reproducibility: Y (Frequency Rate : 100%)

Comment 34

[Jason Smith [:jsmith]]

Please file a followup bug for the issue you've hit.

Comment 35

[sbogati]

Issue does not repros on Leo 
Build ID: 20130729070226
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8135299f3efd
Gaia: 7aaffc8ccb6cf7ddd1e97943c108f1cb9eae5de0
Platform Version: 18.1
Result as expected