If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

HARDWARE_UNAVAILABLE is already thrown while GetUserMedia for audio

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: schien, Assigned: slee)

Tracking

(Blocks: 1 bug, {regression})

Trunk
1.2 C6(Dec6)
ARM
Gonk (Firefox OS)
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 verified)

Details

Attachments

(4 attachments, 19 obsolete attachments)

49 bytes, text/x-github-pull-request
Details | Review | Splinter Review
8.34 KB, patch
slee
: review+
Details | Diff | Splinter Review
15.39 KB, patch
slee
: review+
Details | Diff | Splinter Review
24.60 KB, patch
slee
: review+
Details | Diff | Splinter Review
After bug 920823 landed, AudioFlinger is failed in openRecord() while checking process permission. Except for Camera app, processes have no "Groups" and will return false in checkPermission in fakeperm.cpp.

===Related logcat information===
W/AudioFlinger(  115): Thread AudioOut_1 cannot connect to the power manager service
W/AudioHardwareMSM76XXA(  115): rpc_snd_set_device(6, 1, 1)
W/AudioFlinger(  115): Thread AudioIn_5 cannot connect to the power manager service
W/AudioHardwareMSM76XXA(  115): rpc_snd_set_device(6, 1, 1)
E/fakeperm(  108): android.permission.RECORD_AUDIO for pid=423,uid=10423 denied: missing group
W/ServiceManager(  115): Permission failure: android.permission.RECORD_AUDIO from uid=10423 pid=423
E/AudioFlinger(  115): Request requires android.permission.RECORD_AUDIO
E/AudioRecord(  423): AudioFlinger could not create record track, status: -1
E/libOpenSLES(  423): android_audioRecorder_realize(0x44a11c00) error creating AudioRecord object
W/libOpenSLES(  423): Leaving Object::Realize (SL_RESULT_CONTENT_UNSUPPORTED)
Ah, ok.  I think we may need to define a new supplemental group that is associated with whatever app manifest permission is required to access the audio record API, like what we do for camera [1].   Fakeperm can then check for that group like it currently checks for AID_SDCARD_RW.   


[1] http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#l305
Does this essentially mean that gUM audio on B2G will always fail right now with HARDWARE_UNAVAILABLE? If so, this needs to block.
Blocks: 918876
Yes, this is a koi blocker because bug 920823 is also landed on v1.2 gonk-misc.
blocking-b2g: --- → koi?

Updated

4 years ago
Keywords: regression

Comment 4

4 years ago
Hi Chien,

Will you be able to help on this case? I plus it since this will block audio gUM.
Flags: needinfo?(schien)

Updated

4 years ago
blocking-b2g: koi? → koi+
My queue for koi/1.3 is quite full now. @slee can you help on this bug?
Flags: needinfo?(schien)
Flags: needinfo?(slee)
(Assignee)

Comment 6

4 years ago
sure. :)
Assignee: nobody → slee
Flags: needinfo?(slee)
(Assignee)

Comment 7

4 years ago
Hi mwu,

What do you think if we always grant the request for "android.permission.RECORD_AUDIO"? For audio recording, user must get the MediaStream through GetUserMedia API. When calling GetUSerMedia, it needs users' permission then the app can get the MediaStream for recording. 
Thanks.
Flags: needinfo?(mwu)

Comment 8

4 years ago
(In reply to Michael Vines [:m1] [:evilmachines] from comment #1)
> Ah, ok.  I think we may need to define a new supplemental group that is
> associated with whatever app manifest permission is required to access the
> audio record API, like what we do for camera [1].   Fakeperm can then check
> for that group like it currently checks for AID_SDCARD_RW.   
> 

The problem is nearly any app can get camera/mic access after starting by prompting the user. Most importantly, webpages can ask for camera/mic access, and there's no useful way (AFAIK) to grant other processes new groups after the fact.

Comment 9

4 years ago
(In reply to StevenLee from comment #7)
> What do you think if we always grant the request for
> "android.permission.RECORD_AUDIO"? For audio recording, user must get the
> MediaStream through GetUserMedia API. When calling GetUSerMedia, it needs
> users' permission then the app can get the MediaStream for recording. 
> Thanks.

We would like to grant the request for RECORD_AUDIO only *after* the permission is granted, in case the child process is compromised and tries to record without asking for the recording permission.
Flags: needinfo?(mwu)
Accessing microphone and camera doesn't mean a SD card read/writing is going to happen, e.g., WebRTC is transmitting video/audio frame to the remote peer. It's a bit weird to me that we are binding the SD card read/write permission to whom want to access microphone and camera.

Comment 11

4 years ago
(In reply to Shih-Chiang Chien [:schien] from comment #10)
> Accessing microphone and camera doesn't mean a SD card read/writing is going
> to happen, e.g., WebRTC is transmitting video/audio frame to the remote
> peer. It's a bit weird to me that we are binding the SD card read/write
> permission to whom want to access microphone and camera.

Agreed. It was a convenient thing under previous assumptions that only the camera app would access the camera, but it's clearly wrong when we want to support more general uses of camera on the web.

Updated

4 years ago
Blocks: 917367

Updated

4 years ago
Target Milestone: --- → 1.2 C4(Nov8)
@mwu, can we backout the patch for checking Groups in fakeperm.cpp? since it already affect the media recording feature severely for our 1.2 road map.
Flags: needinfo?(mwu)
Shih-Chiang - So this works for me on my Buri 1.2 build where I flash gaia/gecko bits on top of a 1.1 Buri base image. So how do you reproduce this bug then?
Flags: needinfo?(schien)
You need to use latest gonk for 1.2 or nightly because the code causing error is in gonk. Using 1.1 base image means you're using 1.1 gonk.
Flags: needinfo?(schien)
Specifically /system/bin/fakeperm needs to be updated.

Looks like this bug has stalled so lemme try to help.  We can't back out that change, see bug 920823 for details.  Since the decision to grant mic/camera access dynamic at runtime, we need a way for the b2g process and fakeperm to coordinate.  Merging fakeperm into Gecko might be one option, which is nice because then we don't need to come up with an IPC protocol between those two processes.
(Assignee)

Comment 16

4 years ago
Hi m1,

bug 920823 is a confidential bug. We cannot see it. Can you cc SC and me so that we can know the details?
(Assignee)

Comment 17

4 years ago
Created attachment 827793 [details] [diff] [review]
testFakePerm.patch

Hi m1,

On comment 15, you mentioned merging fakeperm into gecko. Is it like this? If it is, I will continue the following works.

Thanks.
Attachment #827793 - Flags: feedback?(mvines)
Flags: needinfo?(mwu)
(Assignee)

Comment 18

4 years ago
(In reply to StevenLee from comment #17)
> Created attachment 827793 [details] [diff] [review]
> testFakePerm.patch
BTW, I also remove the origin fakeperm from init.b2g.rc.
Comment on attachment 827793 [details] [diff] [review]
testFakePerm.patch

Yep, that's pretty much what I was thinking.  

Note that fakePermission.h was derived from fakeperm.cpp, which was licensed Apache2, and it looks like that license header was stripped.
Attachment #827793 - Flags: feedback?(mvines) → feedback+
NI on :slee to help with next steps here. Steven, given we have feedback+ here is this ready to land ?
Flags: needinfo?(slee)
(Assignee)

Comment 21

4 years ago
Created attachment 831310 [details] [diff] [review]
part 1: move fakeperm into b2g
Attachment #827793 - Attachment is obsolete: true
Flags: needinfo?(slee)
(Assignee)

Comment 22

4 years ago
Created attachment 831312 [details] [diff] [review]
part2: mozFakePerm
Attachment #831312 - Flags: feedback?(rjesup)
(Assignee)

Comment 23

4 years ago
Created attachment 831313 [details] [diff] [review]
part3: Check whether mic initialization is done
Attachment #831313 - Flags: feedback?(rjesup)
(Assignee)

Comment 24

4 years ago
Created attachment 831436 [details] [diff] [review]
part 1: move fakeperm into b2g

change the place of initializing FakePermissionService
(Assignee)

Updated

4 years ago
Attachment #831436 - Attachment description: fakeperm.patch part 1: move fakeperm into b2g → part 1: move fakeperm into b2g
(Assignee)

Comment 25

4 years ago
Created attachment 831437 [details] [diff] [review]
part2: mozFakePerm
Attachment #831310 - Attachment is obsolete: true
Attachment #831312 - Attachment is obsolete: true
Attachment #831312 - Flags: feedback?(rjesup)
Attachment #831437 - Flags: feedback?(rjesup)
Comment on attachment 831437 [details] [diff] [review]
part2: mozFakePerm

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

f+, but really this is all in areas outside of my knowledge.  You'll need another reviewer
Attachment #831437 - Flags: feedback?(rjesup) → feedback+
Comment on attachment 831313 [details] [diff] [review]
part3: Check whether mic initialization is done

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

::: dom/media/MediaManager.h
@@ +245,5 @@
> +class MyErrorCallbackRunnable : public nsRunnable
> +{
> +public:
> +  MyErrorCallbackRunnable(
> +    already_AddRefed<nsIDOMGetUserMediaErrorCallback> aError, 

space at end of line

@@ +249,5 @@
> +    already_AddRefed<nsIDOMGetUserMediaErrorCallback> aError, 
> +    const nsAString& aErrorMsg, uint64_t aWindowID)
> +    : mError(aError)
> +    , mErrorMsg(aErrorMsg)
> +    , mWindowID(aWindowID) {}

The order here must match the order they're declared in (mErrorMsg before mError).

@@ +260,5 @@
> +
> +    nsCOMPtr<nsIDOMGetUserMediaErrorCallback> error(mError);
> +
> +    // This is safe since we're on main-thread, and the windowlist can only
> +    // be invalidated from the main-thread (see OnNavigation)

Were the comments here copied from other code? They don't seem to match what you're doing (this and the previous comment at the start of Run()).

@@ +295,5 @@
>      , mVideoSource(aVideoSource)
>      , mListener(aListener)
>      , mFinish(aNeedsFinish)
>      , mWindowID(aWindowID)
> +    , mError(aError)

This will cause problems on some compilers because the order of entries was changed below; the two must match.  (mFinish is now last in your patch.)

@@ +330,5 @@
>                MM_LOG(("Starting audio failed, rv=%d",rv));
> +              NS_DispatchToMainThread(new MyErrorCallbackRunnable(
> +               mError.forget(), NS_LITERAL_STRING("start audio failed"), mWindowID));
> +              mOnTracksAvailableCallback.forget();
> +              mStream.forget();

This will cause mStream and mOnTracksAvailableCallback to leak; rarely if ever is a stand-alone foo.forget() correct.  As an argument (to something that takes already_AddRefed) sure, as foo = bar.forget() sure (avoids an AddRef/Release pair, especially if you're on the "wrong" thread), as return bar.forget() sure (if the return type is already_AddRefed).

@@ +358,5 @@
>                                                mAudioSource != nullptr,
>                                                mVideoSource != nullptr,
>                                                mWindowID);
>            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +          mError.forget();

This will cause mError to leak

@@ +409,1 @@
>  };

Don't delete the blank line here
Attachment #831313 - Flags: feedback?(rjesup) → feedback-

Comment 28

4 years ago
Since 1.2 C4 was passed, could you assign a new target milestone that you're trying to hit?
(Assignee)

Updated

4 years ago
Target Milestone: 1.2 C4(Nov8) → 1.2 C6(Dec6)
(Assignee)

Comment 29

4 years ago
Created attachment 8333683 [details] [diff] [review]
part 0: remove fakeperm from gonk
Attachment #8333683 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #8333683 - Flags: review? → review?(mwu)
(Assignee)

Updated

4 years ago
Attachment #831436 - Flags: review?(mwu)
(Assignee)

Comment 30

4 years ago
Created attachment 8333688 [details] [diff] [review]
part2: mozFakePerm

Hi Fabrice,

Would you please help review this patch?
Thanks.
Attachment #831437 - Attachment is obsolete: true
Attachment #8333688 - Flags: review?(fabrice)
(Assignee)

Comment 31

4 years ago
Created attachment 8333748 [details] [diff] [review]
part3: Check whether mic initialization is done¡

address comment 27
Attachment #831313 - Attachment is obsolete: true
Attachment #8333748 - Flags: review?(rjesup)

Updated

4 years ago
Attachment #8333748 - Flags: review?(rjesup) → review+

Updated

4 years ago
Attachment #8333683 - Flags: review?(mwu) → review+
Comment on attachment 8333688 [details] [diff] [review]
part2: mozFakePerm

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

I'm not super familiar with what we're doing there but:
- why is this code in dom/base ? It seems to me that it should be in hal/gonk instead.
- wouldn't it be better to use an enum for permissions instead of strings? Is that performance sensitive?

::: dom/base/mozFakePermission.cpp
@@ +10,5 @@
> +  int32_t pid;
> +  permissionObject(const char* perm, int32_t p) : pid(p) {
> +    strcpy(permission, perm);
> +  }
> +};

please make permission and pid private. (and consider using nsACString instead of char*)

@@ +21,5 @@
> +    } else {
> +      return false;
> +    }
> +  }
> +};

Instead of a new class, why not have a Equals() method on the permissionObject itself?

@@ +25,5 @@
> +};
> +
> +static mozFakePermission* gfakePerm = NULL;
> +
> +mozFakePermission* mozFakePermission::GetInstance() {

nit: Here and in all other methods: move the { to its own line.

::: dom/base/mozFakePermission.h
@@ +15,5 @@
> +{
> +public:
> +  static mozFakePermission* GetInstance();
> +
> +  void addGrantInfo(const char* permission, int32_t pid);

any reason why these are char* and not nsACString ?
Attachment #8333688 - Flags: review?(fabrice)
(Assignee)

Comment 33

4 years ago
(In reply to Fabrice Desré [:fabrice] from comment #32)
> I'm not super familiar with what we're doing there but:
What I want to solve is that we fail to call GetUserMedia to get mic resource because of permission not granted. There are 2 permission checks. One is the dialog prompt by Gecko and the other goes through audioflinger, from android, to fakeperm in gonk. The first one is done. I want to solve the latter. So I need to pass the grant information from MediaPermissionGonk(from gecko) to fakeperm. As comment 15, we want to merge fakeperm into b2g that make the things easier.

> - why is this code in dom/base ? It seems to me that it should be in
> hal/gonk instead.
> - wouldn't it be better to use an enum for permissions instead of strings?
> Is that performance sensitive?
No, because of fakeperm uses strings to check. I think we may not need a table to map the strings. 
> 
> ::: dom/base/mozFakePermission.cpp
> @@ +10,5 @@
> > +  int32_t pid;
> > +  permissionObject(const char* perm, int32_t p) : pid(p) {
> > +    strcpy(permission, perm);
> > +  }
> > +};
> 
> please make permission and pid private. (and consider using nsACString
> instead of char*)
OK. 
> 
> @@ +21,5 @@
> > +    } else {
> > +      return false;
> > +    }
> > +  }
> > +};
> 
> Instead of a new class, why not have a Equals() method on the
> permissionObject itself?
Sure.
> 
> @@ +25,5 @@
> > +};
> > +
> > +static mozFakePermission* gfakePerm = NULL;
> > +
> > +mozFakePermission* mozFakePermission::GetInstance() {
> 
> nit: Here and in all other methods: move the { to its own line.
I will update it in the next version.
> 
> ::: dom/base/mozFakePermission.h
> @@ +15,5 @@
> > +{
> > +public:
> > +  static mozFakePermission* GetInstance();
> > +
> > +  void addGrantInfo(const char* permission, int32_t pid);
> 
> any reason why these are char* and not nsACString ?
No, I will change to nsACString.
(Assignee)

Comment 34

4 years ago
Created attachment 8334406 [details] [diff] [review]
part2: mozFakePerm V2

Address comment 32
Attachment #8333688 - Attachment is obsolete: true
Attachment #8334406 - Flags: review?(fabrice)

Comment 35

4 years ago
Comment on attachment 831436 [details] [diff] [review]
part 1: move fakeperm into b2g

dom/base doesn't seem to be the appropriate place for this. Also implementing everything inside a header file wrong - please put the implementation in a cpp file.

We've been putting service initialization into http://hg.mozilla.org/mozilla-central/file/c335eaa4494a/widget/gonk/nsAppShell.cpp#l747 FWIW. I'd like to keep all the android service initialization in one place.

BTW, I think this deserves to be called GonkPerm rather than FakePerm once you've hooked the checks in. The service will actually be doing something now, so it doesn't seem right to put fake in its name.
Attachment #831436 - Flags: review?(mwu)

Comment 36

4 years ago
Comment on attachment 8334406 [details] [diff] [review]
part2: mozFakePerm V2

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

::: hal/gonk/mozFakePermission.cpp
@@ +11,5 @@
> +  }
> +  return gfakePerm;
> +}
> +
> +void mozFakePermission::addGrantInfo(const permissionObject& permObj)

I don't see any users of this. What code uses this?

Comment 37

4 years ago
Comment on attachment 8334406 [details] [diff] [review]
part2: mozFakePerm V2

(per agreement on IRC, switching review to myself)
Attachment #8334406 - Flags: review?(fabrice) → review?(mwu)

Comment 38

4 years ago
Nevermind about addGrantInfo - I found the caller in the last patch of the series.
(Assignee)

Comment 39

4 years ago
Created attachment 8335126 [details] [diff] [review]
part 1: move fakeperm into b2g - V2

Address comment 35
Attachment #831436 - Attachment is obsolete: true
Attachment #8335126 - Flags: review?(mwu)

Comment 40

4 years ago
Comment on attachment 8335126 [details] [diff] [review]
part 1: move fakeperm into b2g - V2

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

::: widget/gonk/nsAppShell.cpp
@@ +730,5 @@
>  
>  #ifdef MOZ_OMX_DECODER
>      if (XRE_GetProcessType() == GeckoProcessType_Default) {
>        android::MediaResourceManagerService::instantiate();
> +      android::FakePermissionService::instantiate();

This shouldn't depend on MOZ_OMX_DECODER. Fortunately, this will be easy to do once bug 929973 lands, which cleans this part of the code up a bit. I recommend rebasing on top of the patch there.

Comment 41

4 years ago
Comment on attachment 8334406 [details] [diff] [review]
part2: mozFakePerm V2

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

I think we should just integrate mozFakePermission into GonkPermission. There's no reason to keep the permission information in separate class. There's also no need to expose permissionObject. It only holds two fields which can be specified directly when granting a permission.
Attachment #8334406 - Flags: review?(mwu)
(Assignee)

Comment 42

4 years ago
Created attachment 8336598 [details] [diff] [review]
part 1: move fakeperm into b2g - V3

1. rebase patch base on bug 929973
2. merge with original part2 patch
3. merge mozFakePermission into GonkPermissionService
Attachment #8334406 - Attachment is obsolete: true
Attachment #8335126 - Attachment is obsolete: true
Attachment #8335126 - Flags: review?(mwu)
Attachment #8336598 - Flags: review?(mwu)
(Assignee)

Comment 43

4 years ago
Created attachment 8336600 [details] [diff] [review]
part2 : Check whether mic initialization is done - V3

rebase with the latest patch
Attachment #8333748 - Attachment is obsolete: true
Attachment #8336600 - Flags: review+
(Assignee)

Comment 44

4 years ago
Created attachment 8336690 [details] [diff] [review]
part 1: move fakeperm into b2g - V3
Attachment #8336598 - Attachment is obsolete: true
Attachment #8336598 - Flags: review?(mwu)
Attachment #8336690 - Flags: review?(mwu)
(Assignee)

Comment 45

4 years ago
Created attachment 8336692 [details] [diff] [review]
part2 : Check whether mic initialization is done - V3
Attachment #8336600 - Attachment is obsolete: true
Attachment #8336692 - Flags: review+

Comment 46

4 years ago
Comment on attachment 8336690 [details] [diff] [review]
part 1: move fakeperm into b2g - V3

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

Things look a bit better and more straightforward here. The indentation in GonkPermission.* is inconsistent though - pick either two or four space and stick with it. widget/gonk is generally 4 space indent, but I don't care too much as long as you pick one and stick with it.

::: widget/gonk/GonkPermission.cpp
@@ +103,5 @@
> +  }
> +  return gGonkPermissionService;
> +}
> +
> +void GonkPermissionService::addGrantInfo(const permissionObject& permObj)

I think it would be better to specify the pid and permission separately rather than use permissionObject.

@@ +108,5 @@
> +{
> +  mGrantArray.AppendElement(permObj);
> +}
> +
> +bool GonkPermissionService::checkPermission(const permissionObject& permObj)

We should be able to integrate this directly into the other checkPermission function. This isn't a function that needs to be exposed outside the class.

@@ +119,5 @@
> +      permObj.permission().Equals("android.permission.RECORD_AUDIO")) {
> +    deleteEntry = true;
> +  }
> +
> +  for (uint32_t i = 0; i < mGrantArray.Length(); i++) {

You should be able to use IndexOf to find the entry instead of manually looping through the array.

::: widget/gonk/GonkPermission.h
@@ +21,5 @@
> +#include "nsTArray.h"
> +
> +namespace mozilla {
> +
> +class permissionObject

PermissionGrant might be a better name. Class names generally have the first letter capitalized, and Object is redundant.

@@ +36,5 @@
> +  }
> +
> +  bool operator==(const permissionObject& other) const
> +  {
> +    if (mPid == other.pid() && mPermission.Equals(other.permission())) {

return this directly rather than using if/else.

@@ +81,5 @@
> +  void addGrantInfo(const mozilla::permissionObject& permObj);
> +  bool checkPermission(const mozilla::permissionObject& permObj);
> +private:
> +  GonkPermissionService(): android::BnPermissionController() {}
> +  nsTArray<mozilla::permissionObject> mGrantArray;

mozilla:: shouldn't be necessary here since you're already in the mozilla namespace.
Attachment #8336690 - Flags: review?(mwu)
(Assignee)

Comment 47

4 years ago
Created attachment 8337209 [details] [diff] [review]
part 1: move fakeperm into b2g - V4

Hi mwu,

I update a new patch. Please help review it.
Thanks.
Attachment #8336690 - Attachment is obsolete: true
Attachment #8337209 - Flags: review?(mwu)

Comment 48

4 years ago
Comment on attachment 8337209 [details] [diff] [review]
part 1: move fakeperm into b2g - V4

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

Looks pretty good - thanks.

I think my only concern now is a potential leak of PermissionGrant objects if a child process gets a grant but doesn't use it, or crashes before being able to use it. It would be nice to remove all PermissionGrants associated with a pid when a child quits. Might need to file a followup for that.

::: widget/gonk/GonkPermission.h
@@ +21,5 @@
> +#include "nsTArray.h"
> +
> +namespace mozilla {
> +class PermissionGrant
> +

remove this empty line

@@ +36,5 @@
> +  }
> +
> +  bool operator==(const PermissionGrant& other) const
> +  {
> +    return (mPid == other.pid() && mPermission.Equals(other.permission()));  }

Curly brace needs to be on a new line.
Attachment #8337209 - Flags: review?(mwu) → review+
(Assignee)

Comment 49

4 years ago
Created attachment 8338227 [details] [diff] [review]
part 1: move fakeperm into b2g - V5

address comment 48
Attachment #8337209 - Attachment is obsolete: true
Attachment #8338227 - Flags: review+
(Assignee)

Comment 50

4 years ago
Created attachment 8338228 [details] [diff] [review]
part2 : Check whether mic initialization is done - V4

rebase and fix cross compile error
Attachment #8336692 - Attachment is obsolete: true
Attachment #8338228 - Flags: review+
(Assignee)

Comment 51

4 years ago
Here is the log of try server.
* try: -b do -p linux64,android-x86,emulator,unagi,linux64_gecko,macosx64_gecko -u all -t none
** https://tbpl.mozilla.org/?tree=Try&rev=7d7992ca60dd
(Assignee)

Comment 52

4 years ago
Created attachment 8338291 [details] [review]
part 0: remove fakeperm from gonk

This patch should be landed after patch part 1 and part 2 or b2g will be blocked.
Attachment #8333683 - Attachment is obsolete: true
Comment on attachment 8338227 [details] [diff] [review]
part 1: move fakeperm into b2g - V5

I still don't think you should re-license widget/gonk/GonkPermission.cpp to MPL2, like I mentioned in comment 19.  This file is clearly derived from https://github.com/mozilla-b2g/gonk-misc/blob/master/fakeperm.cpp which is A2.
Attachment #8338227 - Flags: feedback-
(Assignee)

Comment 54

4 years ago
Created attachment 8338302 [details] [diff] [review]
part 1: move fakeperm into b2g - V6

Hi m1,

Sorry for that I forgot updating the header of cpp. Here is the correct one.
Attachment #8338227 - Attachment is obsolete: true
Attachment #8338302 - Flags: review+
Ta!

Comment 56

4 years ago
Hey Naoki, just wanted to give you a heads up on this. This may break our current partial flash testing until a new 1.2 base lands. We can delete /system/bin/fakeperm in the partial flash scripts to ensure things work after this lands. Let us know how you want to proceed on this.
Flags: needinfo?(nhirata.bugzilla)
Thanks for the heads up.

Nuts.  We could go with deleting the /system/bin/fakeperm ; having said that getting everyone in sync with that is going to be rough.  Taipei script will have to temporarily change + the fullflash script.

How soon will a new 1.2 base build be out that has the fix for this?  Would we survive not making this change?
Flags: needinfo?(mwu)

Comment 58

4 years ago
Well, we can't get a base build that removes fakeperm without landing this first, so it might be a bit difficult to avoid this unless we have codeaurora take the change temporarily.

Things also might work without any deleting of fakeperm - I have no idea what happens when a second process attempts to provide the same service.

The change to delete fakeperm doesn't have to be temporary - fakeperm is unlikely to come back.
Flags: needinfo?(mwu)
(Assignee)

Comment 59

4 years ago
(In reply to Michael Wu [:mwu] from comment #58)
> Things also might work without any deleting of fakeperm - I have no idea
> what happens when a second process attempts to provide the same service.
I did the same test on unagi and it works. I also see the log from logcat.
"E/ServiceManager(  102): add_service('permission',0x8) uid=0 - ALREADY REGISTERED, OVERRIDE"
At this time, the permission request goes to b2g::GonkPermission.

Then I restart fakeperm and I got the same log. After restarting, the permission request goes back to fakeperm again.
If restarting will cause the file to come back, it would have to be deleted every reboot... this doesn't seem like something a flash script would cover easily.

I guess we should just land this and then file a follow up bug to get the other part fixed ASAP?
(Assignee)

Comment 61

4 years ago
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #60)
> I guess we should just land this and then file a follow up bug to get the
> other part fixed ASAP?
Agree, request to check-in since it breaks the feature of audio recording.
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Depends on: 944595
https://hg.mozilla.org/integration/b2g-inbound/rev/c24da899172a
https://hg.mozilla.org/integration/b2g-inbound/rev/720a36d92d37
Keywords: checkin-needed

Comment 63

4 years ago
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #60)
> If restarting will cause the file to come back, it would have to be deleted
> every reboot... this doesn't seem like something a flash script would cover
> easily.
> 
> I guess we should just land this and then file a follow up bug to get the
> other part fixed ASAP?

It shouldn't come back if it's deleted. StevenLee did not delete fakeperm.
Backed out for causing frequent hangs in test_dataChannel_basicAudio.html.
https://hg.mozilla.org/integration/b2g-inbound/rev/d4577f8235d9

https://tbpl.mozilla.org/php/getParsedLog.php?id=31321907&tree=B2g-Inbound
MediaManger seems to have a bug about releasing JS object in non-main Thread. It is Bug 945334.
(In reply to Sotaro Ikeda [:sotaro] from comment #65)
> MediaManger seems to have a bug about releasing JS object in non-main
> Thread. It is Bug 945334.

Do we need bug 945334 fixed to fix this bug?
Depends on: 945334
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 67

4 years ago
Hi Ryan,

Could we land this first then I fix all the threading issues in bug 945334? Because it blocks bug 853356.
Thanks.
Blocks: 853356
Flags: needinfo?(ryanvm)
Wasn't bug 945334 filed for the reason this was backed out? We were having frequent mochitest-3 hangs with this landed.
Flags: needinfo?(ryanvm)
(In reply to Jason Smith [:jsmith] from comment #66)
> (In reply to Sotaro Ikeda [:sotaro] from comment #65)
> > MediaManger seems to have a bug about releasing JS object in non-main
> > Thread. It is Bug 945334.
> 
> Do we need bug 945334 fixed to fix this bug?

Yes, I think it is necessary. But bug 945334 fixed is necessary in general. Current MediaManager incorrectly handling JS object, this causes intermittent crash at MediaManager. We experience this at dom camera.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #68)
> Wasn't bug 945334 filed for the reason this was backed out? We were having
> frequent mochitest-3 hangs with this landed.

That's correct. That bug needs to be fixed in order to land this to keep the tree green. We aren't turning off tests here to resolve this.
(Assignee)

Comment 71

4 years ago
Created attachment 8341726 [details] [diff] [review]
part 3: Fix MediaManager incorrectly handling JS objects

Hi Sotaro,

Could I fix the problem in this bug? 

Here is the results of try server, https://tbpl.mozilla.org/?tree=Try&rev=aa00b60382c6

Thanks.
Attachment #8341726 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8341726 [details] [diff] [review]
part 3: Fix MediaManager incorrectly handling JS objects

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

Sorry, I am not a correct person to review this patch. It might be better to be reviewed by MediaManaer owner or Bobby Holley (:bholley), who wrote nsMainThreadPtr{Handle,Holder} API in Bug 771074.
Attachment #8341726 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 73

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #72)
> Sorry, I am not a correct person to review this patch. It might be better to
> be reviewed by MediaManaer owner or Bobby Holley (:bholley), who wrote
> nsMainThreadPtr{Handle,Holder} API in Bug 771074.
OK, I will change the reviewer.
Thanks.
(Assignee)

Comment 74

4 years ago
Comment on attachment 8341726 [details] [diff] [review]
part 3: Fix MediaManager incorrectly handling JS objects

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

Hi Randell,

Could you help review my patch?
Thanks.
Attachment #8341726 - Flags: review?(rjesup)
Comment on attachment 8341726 [details] [diff] [review]
part 3: Fix MediaManager incorrectly handling JS objects

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

So here are the comments - however, before we go and land something to add proxy levels to all this I'd like to know why this suddenly was needed - i.e. is bug 945334 a problem we need to have, and secondly, even if it is (and this change is therefore needed), that doesn't explain why desktop builds were failing to exit on b-i after this landed before.  So I'm not sure this is going to go any better on landing.  I suggest a *targeted* try for the known-failing platforms and tests (m3 IIRC) and then retrigger it a bunch.

I'm going to r+ it assuming the types are modified (and that works correctly), but I want those confirmations and explanation why this is needed before we land.

::: dom/media/MediaManager.cpp
@@ +183,5 @@
>  }
>  
>  ErrorCallbackRunnable::ErrorCallbackRunnable(
> +  nsMainThreadPtrHandle<nsIDOMGetUserMediaSuccessCallback> aSuccess,
> +  nsMainThreadPtrHandle<nsIDOMGetUserMediaErrorCallback> aError,

See below about types

@@ +217,5 @@
>  class SuccessCallbackRunnable : public nsRunnable
>  {
>  public:
>    SuccessCallbackRunnable(
> +    nsMainThreadPtrHandle<nsIDOMGetUserMediaSuccessCallback> aSuccess,

Wouldn't the arguments be nsIDOMGetUserMediaSuccessCallback's, and do mSuccess(new nsMainThreadPtrHolder<...>(aSuccess))?

Or an &aSuccess.  But I think this might create useless temp references or objects.

@@ +233,5 @@
>      // Only run if the window is still active.
>      NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
>  
> +    //nsCOMPtr<nsIDOMGetUserMediaSuccessCallback> success(mSuccess);
> +    //nsCOMPtr<nsIDOMGetUserMediaErrorCallback> error(mError);

Don't leave commented-out code here or elsewhere, unless there's a reason (and if so include a comment as to why)

@@ +497,5 @@
>  {
>  public:
>    GetUserMediaStreamRunnable(
> +    nsMainThreadPtrHandle<nsIDOMGetUserMediaSuccessCallback> aSuccess,
> +    nsMainThreadPtrHandle<nsIDOMGetUserMediaErrorCallback> aError,

Ditto types

@@ +516,5 @@
>    class TracksAvailableCallback : public DOMMediaStream::OnTracksAvailableCallback
>    {
>    public:
>      TracksAvailableCallback(MediaManager* aManager,
> +                            nsMainThreadPtrHandle<nsIDOMGetUserMediaSuccessCallback> aSuccess,

Ditto types

Also indent to match; you can break the 80-char limit

@@ +782,5 @@
>  public:
>    GetUserMediaRunnable(
>      const MediaStreamConstraintsInternal& aConstraints,
> +    nsIDOMGetUserMediaSuccessCallback* aSuccess,
> +    nsIDOMGetUserMediaErrorCallback* aError,

Right - this is better
Attachment #8341726 - Flags: review?(rjesup) → review+
(Assignee)

Comment 76

4 years ago
I found there are 2 places that may cause the crash problem.
1. If main thread executes and release the event, [1], before media thread exits the scope [2], the event will be released on media thread. That can also explain why part-3 patch can pass the tests.
2. The window, [3], could be null in the test case. It causes IPC failed and makes child process be killed. I ever encountered once on my local test. I am not sure if it's related to bug 940045. I will update my repo and try again.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.h#337
[2] http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.h#344
[3] http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1844
(Assignee)

Comment 77

4 years ago
Created attachment 8342831 [details] [diff] [review]
part2 : Check whether mic initialization is done - V5

rebase
Attachment #8338228 - Attachment is obsolete: true
Attachment #8342831 - Flags: review+
(Assignee)

Comment 78

4 years ago
Created attachment 8342832 [details] [diff] [review]
part 3: Fix MediaManager incorrectly handling JS objects - v2

address comment 75 and rebase
Attachment #8341726 - Attachment is obsolete: true
Attachment #8342832 - Flags: review+
(Assignee)

Comment 79

4 years ago
* Here is the try server log 
** https://tbpl.mozilla.org/?tree=Try&rev=a9746b99d38c
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/a481c9adc411
https://hg.mozilla.org/integration/b2g-inbound/rev/6a829a0de92d
https://hg.mozilla.org/integration/b2g-inbound/rev/e88d34a3c8d5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a481c9adc411
https://hg.mozilla.org/mozilla-central/rev/6a829a0de92d
https://hg.mozilla.org/mozilla-central/rev/e88d34a3c8d5

And Part 3 was later backed out by Randell.
https://hg.mozilla.org/mozilla-central/rev/c4b843dfa959
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox28: --- → fixed
Flags: needinfo?(nhirata.bugzilla)
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f2cb80854f3c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4a503cfb0ebc
status-b2g-v1.2: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
Sanity tests on a 1.2 base image with gaia/gecko on top worked fine for a 12/12 1.2 buri build.
status-b2g-v1.2: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.