Closed Bug 931456 Opened 6 years ago Closed 6 years ago

[fugu][USB]When slog deamon works, open "Enable USB storage" in the setting menu, but the phone does not enter the mass storage mode, and the disk doesn't appear in the PC.

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:fugu+)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g fugu+

People

(Reporter: james.zhang, Assigned: alan.yenlin.huang)

References

Details

(Whiteboard: [Fugu] [v1.2f-uplift-needed])

Attachments

(2 files, 4 obsolete files)

Attached patch automounter.patch (obsolete) — Splinter Review
This bug is the same as 908139. But we need fix it on fugu device.

Steps to reproduce:

Test 1:
1, Play music.
2, Open "Enable USB storage" in the setting menu

Test 2:
1, After the music is stopped.
2, Open or Close "Enable USB storage" in the setting menu.


Actual results:

1,  The phone does not enter the mass storage mode, and the disk doesn't appear in the PC.


Expected results:

1, The phone can enter the mass storage mode, and the disk can appear in the PC.
Comment on attachment 822868 [details] [diff] [review]
automounter.patch

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

We can confirm this bug on v1.2 with out device fugu.
Code related to the bug is here:AutoMounter::UpdateState()
AutoMounter::UpdateState will be waiting until all opened files were closed.
And we did a try to kill all the process with the opened files, Then UpdateState() can go through.
    
In contract with Android, There will be a choice tip win on screen to remind users.
We suggest that ffos would give users choices either.
Attachment #822868 - Flags: review?(dhylands)
blocking-b2g: --- → koi?
Comment on attachment 822868 [details] [diff] [review]
automounter.patch

This would basically undo all of the work done by having the automounter look for open files in the first place.

Prior to bug 876782, the automounter didn't check for open files. vold does check for open files and kills any process which has open files. This in turn causes other poblems. Adding a kill here would basically make everything go back to pre-876782 except that the AutoMounter would be doing the kill instead of vold.

Have you tested since bug 928558 landed (just recently).
Attachment #822868 - Flags: review?(dhylands) → review-
QA Wanted - Can we test to see if this works post landing of bug 928558?
Keywords: qawanted
QA,

Please check with the latest build if this works, since 928558 has landed
QA Contact: sparsons
This issue does not reproduce on Buri 1.2 Build ID: 20131030004003

Gaia   a788ea1a3e04716938bd41a559c36a831cf1190b
SourceStamp 4fbe1b01ed63
BuildID 20131030004003
Version 26.0a2

When the user enables USB storage while music is playing in the Music App, the device will enter USB Storage Mode.
Keywords: qawanted
Sounds like this is a confirmed dupe of bug 928558 then.
Status: UNCONFIRMED → RESOLVED
blocking-b2g: koi? → ---
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 928558
Maybe music is ok now.
But fugu has some android daemon process open some files, not FFOS apps process, just like 'slog' which writes the adb log and core dump to sdcard.
If you don't kill these process like android, fugu usb storage still can't work without my patch.
So I suggest you follow android way.
mchen, you can test this case on fugu device without my patch.
Flags: needinfo?(mchen)
Please see the log, if slog works, usb storage can't work.

I/AutoMounter(  118): UpdateState: umsAvail:1 umsEnabled:1 mode:1 usbCablePluggedIn:1 tryToShare:1
I/AutoMounter(  118): UpdateState: Volume sdcard is Mounted and inserted @ /mnt/external gen 1 locked 0 sharing y
W/AutoMounter(  118): The following files are open under '/mnt/external'
W/AutoMounter(  118):   PID: 258 file: '/mnt/external/slog/20131031062740/kernel/kernel.log' app: '' comm: 'slog' exe: '/system/bin/slog'
W/AutoMounter(  118):   PID: 258 file: '/mnt/external/slog/20131031062740/android/radio.log' app: '' comm: 'slog' exe: '/system/bin/slog'
W/AutoMounter(  118):   PID: 258 file: '/mnt/external/slog/20131031062740/android/system.log' app: '' comm: 'slog' exe: '/system/bin/slog'
W/AutoMounter(  118):   PID: 258 file: '/mnt/external/slog/20131031062740/android/main.log' app: '' comm: 'slog' exe: '/system/bin/slog'
W/AutoMounter(  118): UpdateState: Mounted volume sdcard has open files, not sharing
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Summary: [fugu][USB]When the music is playing or is stopped, open "Enable USB storage" in the setting menu, but the phone does not enter the mass storage mode, and the disk doesn't appear in the PC. → [fugu][USB]When slog deamon works, open "Enable USB storage" in the setting menu, but the phone does not enter the mass storage mode, and the disk doesn't appear in the PC.
Hi James,

Is slog daemon alive on the user build?
I think it should be enabled on eng build only or protected by some way like engineering mode.

If so, it didn't impact the end user.
Flags: needinfo?(mchen)
(In reply to Marco Chen [:mchen] from comment #10)
> Hi James,
> 
> Is slog daemon alive on the user build?
> I think it should be enabled on eng build only or protected by some way like
> engineering mode.
> 
> If so, it didn't impact the end user.
Slog alive on eng and user debug build.
But don't close this bug, I'll find other test case to prove,  killing the process with opened file is necessary.
Let me find a test case, need info.
Flags: needinfo?(james.zhang)
What is this slog daemon?
Why does it only exist on fugu?
Why isn't it using device storage properly?

The slog daemon should listen for state changes from device storage and close its logs.
We cannot change the code back to kill processes with open files.
Lianxiang will provide patch again, kill the processes which are not created by b2g process.
Flags: needinfo?(james.zhang)
blocking-b2g: --- → fugu?
Flags: needinfo?(lianxiang.zhou)
Please reivew the new patch. In the patch, we check the progress before we try to kill it. If the progress is b2g or child of b2g, we would not try to kill it.
Attachment #8340303 - Flags: review?(fabrice)
Flags: needinfo?(lianxiang.zhou)
Comment on attachment 8340303 [details] [diff] [review]
gecko_dom_system_gonk_AutoMounter.cpp.patch

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

::: dom/system/gonk/AutoMounter.cpp
@@ +462,5 @@
>                     fileInfo.mComm.get(),
>                     fileInfo.mExe.get());
> +
> +              // Check the progress, if it is not b2g or child of b2g, then try
> +              // to kill it.

The AutoMounter shouldn't be killing processes.

I'd be ok with having the AutoMounter ignore the fact that non-b2g apps have open files.

Then when you perform the unmount it will kill any apps which have files open.

@@ +463,5 @@
>                     fileInfo.mExe.get());
> +
> +              // Check the progress, if it is not b2g or child of b2g, then try
> +              // to kill it.
> +              if (strcmp(fileInfo.mComm.get(), "b2g") == 0) {

You shouldn't rbe relying on mComm.

Instead we should look at the mPid. If it matches ours, then those are files we have open. We should also consider any process whose parent process is our pid to also be a process whose open files we should not ignore.
Attachment #8340303 - Flags: review?(fabrice) → review-
I think this might be a better approach.

Considering performance team will enable Nuwa soon, not all apps' ppid will equal chrome's pid. I think we could try out a process is b2g or its descendant by this way:

If it has b2g's pid, then it is b2g. For the rest, we recursively check the ppid. if one of its ancestor has b2g's pid, then we stopped and define it is a descendant of b2g. If we finally reach Init (pid = 1), then it is not a descendant of b2g.
Attachment #8340991 - Flags: review?(dhylands)
Assignee: nobody → ahuang
Comment on attachment 8340991 [details] [diff] [review]
Check whether b2g and its descendant have opened files only, v1

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

I'm going to give this a minus since I think hard-coding this class only return open files for b2g processes or its descendants is wrong.

Otherwise I think it looks good.

::: dom/system/gonk/OpenFileFinder.cpp
@@ +85,5 @@
>            if (ReadSymLink(fdSymLink, resolvedPath) && PathMatches(resolvedPath)) {
>              // We found an open file contained within the directory tree passed
>              // into the constructor.
>              FillInfo(aInfo, resolvedPath);
> +            if (aInfo->mIsB2gOrDescendant) {

Shouldn't this decision really be up to the caller?

By putting this check here, we can only use the OpenFileFinder for determining whether b2g or descendents have open files, which doesn't seem right to me.

I'd be happier if either

1 - The caller passes in an option when constructing the OpenFileFinder that determines whether the filtering takes place or not.
2 - Or we let the caller filter the files returned based on aInfo->mIsB2gOrDescendant is true or not.

@@ +166,2 @@
>      return;
> +  } else {

nit: no else after return

@@ +166,3 @@
>      return;
> +  } else {
> +    while (ppid != getpid() && ppid != 1) {

Don't call getpid() over and over again (it's a system call and therefore takes a non-trivial amount of time).
Cache the result of getpid() when the constructor is called, perhaps using a member variable like mMyPid

You can then use mMyPid elsewhere and shouldn't have to call getpid() anywhere except in the constructor.

@@ +174,5 @@
> +      if (!closeParen) {
> +        return;
> +      }
> +      ppid = atoi(&closeParen[4]);
> +      continue;

Why have a continue at the end of the loop?

@@ +182,5 @@
> +    // This is a not a b2g process.
> +    DBG("Non-b2g process has open file(s)");
> +    aInfo->mIsB2gOrDescendant = false;
> +    return;
> +  } else if (ppid == getpid()) {

nit: no else after return
Attachment #8340991 - Flags: review?(dhylands) → review-
I'd like to pass in an option when constructing the OpenFileFinder :)
Attachment #8340991 - Attachment is obsolete: true
Attachment #8341546 - Flags: review?(dhylands)
Comment on attachment 8341546 [details] [diff] [review]
Check whether b2g and its descendant have opened files only, v2

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

Just a few cleanups

::: dom/system/gonk/OpenFileFinder.cpp
@@ +23,5 @@
>      mFdDir(nullptr),
>      mPid(0)
>  {
> +  sCheckIsB2gOrDescendant = aCheckIsB2gOrDescendant;
> +  sMyPid = getpid();

If you're going to store this every time the constructor is called, then it really doesn't make sense to make sMyPid be static.

You could initialize mMyPid to -1 and only call getpid() if mMyPid was -1

@@ +91,5 @@
>            if (ReadSymLink(fdSymLink, resolvedPath) && PathMatches(resolvedPath)) {
>              // We found an open file contained within the directory tree passed
>              // into the constructor.
>              FillInfo(aInfo, resolvedPath);
> +            // If sCheckIsB2gOrDescendant is set false, caller do care about

nit: s^caller do care^the caller cares^

@@ +93,5 @@
>              // into the constructor.
>              FillInfo(aInfo, resolvedPath);
> +            // If sCheckIsB2gOrDescendant is set false, caller do care about
> +            // all processes which have open files. If sCheckIsB2gOrDescendant
> +            // is set false, we only care about b2g process or its descendant.

nit: s^about b2g process^about the b2g proccess^

and s^descendant^descendants^

::: dom/system/gonk/OpenFileFinder.h
@@ +66,5 @@
>    DIR*      mProcDir; // Used for scanning /proc
>    DIR*      mFdDir;   // Used for scanning /proc/PID/fd
>    int       mPid;     // PID currently being processed
> +
> +  static int    sMyPid; // PID of parent process, we assume we're running on it.

getpid returns pid_t not int, so this variable should be of type pid_t

@@ +67,5 @@
>    DIR*      mFdDir;   // Used for scanning /proc/PID/fd
>    int       mPid;     // PID currently being processed
> +
> +  static int    sMyPid; // PID of parent process, we assume we're running on it.
> +  static bool   sCheckIsB2gOrDescendant; // Do we care about non-b2g process?

This shouldn't be static
Attachment #8341546 - Flags: review?(dhylands) → review+
Revise some comments and change two static attributes to object members.

All looks good, except B2G JB Emulator Opt. But it doesn't look like my patch cause that.
https://tbpl.mozilla.org/?tree=Try&rev=4ba9123d494b
Attachment #8341546 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #822868 - Attachment is obsolete: true
Attachment #8340303 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/68a34c25a2ab
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Comment on attachment 8341612 [details] [diff] [review]
Check whether b2g and its descendant have opened files only, v3

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

::: dom/system/gonk/OpenFileFinder.cpp
@@ +162,5 @@
>    // ) X ppid
>    // 01234
>    int ppid = atoi(&closeParen[4]);
>    // We assume that we're running in the parent process
> +  if (mMyPid == -1) {

So I would have initialized mMyPid to getpid() in the constructor since you made it a non-static member.

And I would only have done the -1 check if it were static (and then I probably would have put it in the constructor as well).

Probably not worth changing at this point, but I thought I would mention it.
(In reply to Dave Hylands [:dhylands] from comment #24)
> So I would have initialized mMyPid to getpid() in the constructor since you
> made it a non-static member.
> 
> And I would only have done the -1 check if it were static (and then I
> probably would have put it in the constructor as well).
> 
> Probably not worth changing at this point, but I thought I would mention it.

Thanks, Dave :)

Apparently I misunderstood something here. Even though there's no bug without applying the change, I think I should still apply it to make it looks better.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
blocking-b2g: fugu? → fugu+
Duplicate of this bug: 933700
Attachment #8342099 - Flags: review?(dhylands) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb4d1db9477c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Fugu] [v1.2f-uplift-needed]
You need to log in before you can comment on or make changes to this bug.