Closed Bug 942032 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mchen, Assigned: kikuo)

Details

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

Attachments

(1 file, 4 obsolete files)

No description provided.
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]
Component: General → AudioChannel
Assignee: nobody → kikuo
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 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+
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.
Attached patch patch (obsolete) — Splinter Review
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 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+
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #8429086 - Flags: feedback?(slin) → feedback+
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+
Comment on attachment 8429086 [details] [diff] [review]
patch

Sorry to modify the feedback+ from Shelly.
Attachment #8429086 - Flags: feedback?(slin) → feedback+
Attached patch patch.diff (obsolete) — Splinter Review
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)
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+
Attached patch patch.diffSplinter Review
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)
Attachment #8429826 - Flags: review?(mchen) → review+
carry r+ from mchen.
Thanks for slin's feedback and mchen's review.
https://hg.mozilla.org/mozilla-central/rev/f76aa5cf4e5a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.