926746 - HARDWARE_UNAVAILABLE is already thrown while GetUserMedia for audio

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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)

Comment 1

[Michael Vines [:m1] [:evilmachines]]

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

Comment 2

[Jason Smith [:jsmith]]

Does this essentially mean that gUM audio on B2G will always fail right now with HARDWARE_UNAVAILABLE? If so, this needs to block.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Yes, this is a koi blocker because bug 920823 is also landed on v1.2 gonk-misc.

Comment 4

[Ivan Tsay (:ITsay)]

Hi Chien,

Will you be able to help on this case? I plus it since this will block audio gUM.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

My queue for koi/1.3 is quite full now. @slee can you help on this bug?

Comment 6

[StevenLee[:slee]]

sure. :)

Comment 7

[StevenLee[:slee]]

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.

Comment 8

[Michael Wu [:mwu]]

(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

[Michael Wu [:mwu]]

(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.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

[Michael Wu [:mwu]]

(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.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@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.

Comment 13

[Jason Smith [:jsmith]]

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?

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 15

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 16

[StevenLee[:slee]]

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?

Comment 17

[StevenLee[:slee]]

Attachment: 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.

Comment 18

[StevenLee[:slee]]

(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 19

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 20

[bhavana bajaj [:bajaj]]

NI on :slee to help with next steps here. Steven, given we have feedback+ here is this ready to land ?

Comment 21

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g

Comment 22

[StevenLee[:slee]]

Attachment: part2: mozFakePerm

Comment 23

[StevenLee[:slee]]

Attachment: part3: Check whether mic initialization is done

Comment 24

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g

change the place of initializing FakePermissionService

Comment 25

[StevenLee[:slee]]

Attachment: part2: mozFakePerm

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 28

[Kai-Chih Hu [:khu]]

Since 1.2 C4 was passed, could you assign a new target milestone that you're trying to hit?

Comment 29

[StevenLee[:slee]]

Attachment: part 0: remove fakeperm from gonk

Comment 30

[StevenLee[:slee]]

Attachment: part2: mozFakePerm

Hi Fabrice,

Would you please help review this patch?
Thanks.

Comment 31

[StevenLee[:slee]]

Attachment: part3: Check whether mic initialization is done¡

address comment 27

Comment 32

[[:fabrice] Fabrice Desré]

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 ?

Comment 33

[StevenLee[:slee]]

(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.

Comment 34

[StevenLee[:slee]]

Attachment: part2: mozFakePerm V2

Address comment 32

Comment 35

[Michael Wu [:mwu]]

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.

Comment 36

[Michael Wu [:mwu]]

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

[Michael Wu [:mwu]]

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

(per agreement on IRC, switching review to myself)

Comment 38

[Michael Wu [:mwu]]

Nevermind about addGrantInfo - I found the caller in the last patch of the series.

Comment 39

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g - V2

Address comment 35

Comment 40

[Michael Wu [:mwu]]

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

[Michael Wu [:mwu]]

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.

Comment 42

[StevenLee[:slee]]

Attachment: 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

Comment 43

[StevenLee[:slee]]

Attachment: part2 : Check whether mic initialization is done - V3

rebase with the latest patch

Comment 44

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g - V3

Comment 45

[StevenLee[:slee]]

Attachment: part2 : Check whether mic initialization is done - V3

Comment 46

[Michael Wu [:mwu]]

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.

Comment 47

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g - V4

Hi mwu,

I update a new patch. Please help review it.
Thanks.

Comment 48

[Michael Wu [:mwu]]

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.

Comment 49

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g - V5

address comment 48

Comment 50

[StevenLee[:slee]]

Attachment: part2 : Check whether mic initialization is done - V4

rebase and fix cross compile error

Comment 51

[StevenLee[:slee]]

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

Comment 52

[StevenLee[:slee]]

Attachment: part 0: remove fakeperm from gonk

This patch should be landed after patch part 1 and part 2 or b2g will be blocked.

Comment 53

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 54

[StevenLee[:slee]]

Attachment: part 1: move fakeperm into b2g - V6

Hi m1,

Sorry for that I forgot updating the header of cpp. Here is the correct one.

Comment 55

[Michael Vines [:m1] [:evilmachines]]

Ta!

Comment 56

[Michael Wu [:mwu]]

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.

Comment 57

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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?

Comment 58

[Michael Wu [:mwu]]

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.

Comment 59

[StevenLee[:slee]]

(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.

Comment 60

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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?

Comment 61

[StevenLee[:slee]]

(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.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c24da899172a
https://hg.mozilla.org/integration/b2g-inbound/rev/720a36d92d37

Comment 63

[Michael Wu [:mwu]]

(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.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

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

Comment 65

[Sotaro Ikeda [:sotaro]]

MediaManger seems to have a bug about releasing JS object in non-main Thread. It is Bug 945334.

Comment 66

[Jason Smith [:jsmith]]

(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?

Comment 67

[StevenLee[:slee]]

Hi Ryan,

Could we land this first then I fix all the threading issues in bug 945334? Because it blocks bug 853356.
Thanks.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Wasn't bug 945334 filed for the reason this was backed out? We were having frequent mochitest-3 hangs with this landed.

Comment 69

[Sotaro Ikeda [:sotaro]]

(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.

Comment 70

[Jason Smith [:jsmith]]

(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.

Comment 71

[StevenLee[:slee]]

Attachment: 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.

Comment 72

[Sotaro Ikeda [:sotaro]]

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.

Comment 73

[StevenLee[:slee]]

(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.

Comment 74

[StevenLee[:slee]]

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.

Comment 75

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 76

[StevenLee[:slee]]

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

Comment 77

[StevenLee[:slee]]

Attachment: part2 : Check whether mic initialization is done - V5

rebase

Comment 78

[StevenLee[:slee]]

Attachment: part 3: Fix MediaManager incorrectly handling JS objects - v2

address comment 75 and rebase

Comment 79

[StevenLee[:slee]]

* Here is the try server log 
** https://tbpl.mozilla.org/?tree=Try&rev=a9746b99d38c

Comment 80

[Ryan VanderMeulen [:RyanVM]]

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

Comment 81

[Ryan VanderMeulen [:RyanVM]]

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

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f2cb80854f3c
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4a503cfb0ebc

Comment 83

[Jason Smith [:jsmith]]

Sanity tests on a 1.2 base image with gaia/gecko on top worked fine for a 12/12 1.2 buri build.