[AudioManager] Add Singleton Protection and Prevent been Created from Content Process.

RESOLVED FIXED

Status

Firefox OS
AudioChannel
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mchen, Assigned: kikuo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [u=devices c=media p=0])

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
AudioManager now became a system service in chrome process for controlling audio related configuration (volume control, routing path control, mic mute). So it should be guaranteed to have only one instance in chrome process and can't be created from content process.
Whiteboard: [u=devices c=media p=0]

Updated

5 years ago
Component: General → AudioChannel
(Assignee)

Updated

4 years ago
Assignee: nobody → kikuo
(Assignee)

Comment 2

4 years ago
Created attachment 8428554 [details] [diff] [review]
Add singleton pattern to AudioManager, and avoid the access from content process.
(Assignee)

Comment 3

4 years ago
Comment on attachment 8428554 [details] [diff] [review]
Add singleton pattern to AudioManager, and avoid the access from content process.

Any feedback would be appreciated.
Attachment #8428554 - Flags: feedback?(slin)

Comment 4

4 years ago
Comment on attachment 8428554 [details] [diff] [review]
Add singleton pattern to AudioManager, and avoid the access from content process.

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

::: dom/system/gonk/AudioManager.cpp
@@ +464,5 @@
> +AudioManager*
> +AudioManager::Get()
> +{
> +    if (!sAudioManager)
> +    { // Avoid createing multiple AudioManager instance inside main process

Put the { at the end of the if statement, unless it is the coding style of this current file. Comment should be a sentence, end it with a period.

::: dom/system/gonk/AudioManager.h
@@ +43,5 @@
>                     , public nsIObserver
>  {
>  public:
> +  static already_AddRefed<AudioManager> GetInstance();
> +  static AudioManager* Get();

You can move the implementation of Get() to your GetInstance(), and remove this method.
Attachment #8428554 - Flags: feedback?(slin) → feedback+

Comment 5

4 years ago
Btw, you don't need to copy the exact bug title as your check-in comment, rephrase what your're doing here into one single line should be good enough.
(Assignee)

Comment 6

4 years ago
Created attachment 8429003 [details] [diff] [review]
patch

New patch to fix issue with consistent style and remove unnecessary function.
Attachment #8428554 - Attachment is obsolete: true
Attachment #8429003 - Flags: review?(mchen)
Attachment #8429003 - Flags: feedback?(slin)

Comment 7

4 years ago
Comment on attachment 8429003 [details] [diff] [review]
patch

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

"avoid accessing it from content processes."

::: dom/system/gonk/AudioManager.cpp
@@ +473,5 @@
> +    if (!sAudioManager) {
> +      sAudioManager = new AudioManager();
> +    }
> +
> +    nsRefPtr<AudioManager> audioMgr = (AudioManager*)sAudioManager;

Use sAudioManager.get()
Attachment #8429003 - Flags: feedback?(slin) → feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8429086 [details] [diff] [review]
patch

Instead of casting sAudioManaget to AudioManager* directly, 
using sAudioManager.get() is a better way.
Attachment #8429003 - Attachment is obsolete: true
Attachment #8429003 - Flags: review?(mchen)
Attachment #8429086 - Flags: review?(mchen)
Attachment #8429086 - Flags: feedback?(slin)

Updated

4 years ago
Attachment #8429086 - Flags: feedback?(slin) → feedback+
(Reporter)

Comment 9

4 years ago
Comment on attachment 8429086 [details] [diff] [review]
patch

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

Thanks to contribute this patch.
And there are some comments here.

::: dom/system/gonk/AudioManager.cpp
@@ +458,5 @@
>      NS_WARNING("Failed to remove mozsettings-changed observer!");
>    }
>  }
>  
> +StaticRefPtr<AudioManager> AudioManager::sAudioManager;

Could we just declare this global variable here and outside the class of AudioManager?

@@ +465,5 @@
> +AudioManager::GetInstance()
> +{
> +    // Avoid createing AudioManager from content process.
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +      return nullptr;

According to there is no use case of using AudioManager on the content process and there is no IPC client, I suggest to add MOZ_RUNTIMEABORT().

@@ +470,5 @@
> +    }
> +
> +    // Avoid createing multiple AudioManager instance inside main process.
> +    if (!sAudioManager) {
> +      sAudioManager = new AudioManager();

This global variable should join the "ClearOnShutdown()".
Attachment #8429086 - Flags: review?(mchen)
Attachment #8429086 - Flags: feedback?(slin)
Attachment #8429086 - Flags: feedback+
(Reporter)

Comment 10

4 years ago
Comment on attachment 8429086 [details] [diff] [review]
patch

Sorry to modify the feedback+ from Shelly.
Attachment #8429086 - Flags: feedback?(slin) → feedback+
(Assignee)

Comment 11

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

1) Move singleton object out of class AudioManager. Treat it as a static global variable in AudioManager.cpp scope.
2) It seems that there's no MOZ_RUNTIMEABOUT() anymore, so I choose MOZ_CRASH("Message") instead.
3) Add ClearOnShutdown(&gAudioManager) to release resources on shutdown.
Attachment #8429086 - Attachment is obsolete: true
Attachment #8429197 - Flags: review?(mchen)
Attachment #8429197 - Flags: feedback?(slin)
Comment on attachment 8429197 [details] [diff] [review]
patch.diff

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

I think the patch is good enough for mchen to review :)
Attachment #8429197 - Flags: feedback?(slin)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8429197 [details] [diff] [review]
patch.diff

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

Good work.

give review+ after fixing these nits.

::: dom/system/gonk/AudioManager.cpp
@@ +459,5 @@
>      NS_WARNING("Failed to remove mozsettings-changed observer!");
>    }
>  }
>  
> +static StaticRefPtr<AudioManager> gAudioManager;

There are a few cases used g as prefix and most cases used s (ex: sAudioManager)

@@ +469,5 @@
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    MOZ_CRASH("Non-chrome processes should not get here.");
> +  }
> +
> +    // Avoid createing multiple AudioManager instance inside main process.

nit: indent.

::: dom/system/gonk/AudioManager.h
@@ +16,5 @@
>  #ifndef mozilla_dom_system_b2g_audiomanager_h__
>  #define mozilla_dom_system_b2g_audiomanager_h__
>  
>  #include "mozilla/Observer.h"
> +#include "mozilla/StaticPtr.h"

This can be moved to AudioManager.cpp.
Attachment #8429197 - Flags: review?(mchen) → review+
(Assignee)

Comment 14

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

1) Refine variable prefix fromg 'g'(global) to 's'(static).
2) Remove nit indent.
3) Include header("mozilla/StaticPtr.h") in AudioManager.cpp directly.
Attachment #8429197 - Attachment is obsolete: true
Attachment #8429826 - Flags: review?(mchen)
Attachment #8429826 - Flags: feedback?(slin)
Comment on attachment 8429826 [details] [diff] [review]
patch.diff

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

Nice work!

Once you got an approval from a reviewer, please feel free to r+ yourself, and comment with "carry r+ from _your_reviewer_".
And don't forget to run and paste your try server result :)
Attachment #8429826 - Flags: feedback?(slin)
(Assignee)

Updated

4 years ago
Attachment #8429826 - Flags: review?(mchen) → review+
(Assignee)

Comment 16

4 years ago
carry r+ from mchen.
Thanks for slin's feedback and mchen's review.
https://hg.mozilla.org/mozilla-central/rev/f76aa5cf4e5a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.