Closed Bug 1272270 Opened 6 years ago Closed 6 years ago

Improve system app detection in the AudioChannel code to fix existing tests


(Firefox OS Graveyard :: AudioChannel, defect)

Gonk (Firefox OS)
Not set


(Not tracked)



(Reporter: gsvelto, Assigned: gsvelto)




(3 files)

+++ This bug was initially created as a clone of Bug #1254282 +++

Bug 1254282 removed the dependency on the deprecated apps API from the AudioChannel code but cut a little too deep. Identifying the system app correctly is required for correct operation. Similarly opening all channels to non-chrome code has also proven problematic and not very useful since nearly all gaia code that requires a specific audio channel is being moved into chrome anyway.
Assignee: nobody → gsvelto
Comment on attachment 8751689 [details] [diff] [review]
[PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests

This patch identifies the system app window using the 'b2g.system_startup_url' preference which I also use in the tests to get the proper behavior. Also I've removed the ability for non-chrome apps to access default channels; we originally decided for it because we thought some apps that use non-default channels would stay out of chrome code but it turns out they'll become chrome too so it's pointless.
Attachment #8751689 - Flags: review?(fabrice)
I had to add a check against uri being null while running the mochitests as chrome. Diff is trivial:
> diff --git a/dom/audiochannel/AudioChannelAgent.cpp b/dom/audiochannel/AudioChannelAgent.cpp
> index 2a94787..2919777 100644
> --- a/dom/audiochannel/AudioChannelAgent.cpp
> +++ b/dom/audiochannel/AudioChannelAgent.cpp
> @@ -121,9 +121,12 @@ AudioChannelAgent::FindCorrectWindow(nsPIDOMWindowInner* aWindow)
>    nsCOMPtr<nsIURI> uri;
>    principal->GetURI(getter_AddRefs(uri));
> +  if (!uri) {
> +    return NS_OK;
> +  }
> +
>    nsAutoCString spec;
>    uri->GetSpec(spec);
> -
>    if (spec.Equals(systemAppUrl)) {
>      return NS_OK;
>    }

I also had to fix something on the test itself (this was the reason why activestatechanged was not kicking in)
I'm also hitting a similar segfault there
Comment on attachment 8751689 [details] [diff] [review]
[PATCH] Fix detection of the system app when dealing with audio channels and fix the associated tests

Review of attachment 8751689 [details] [diff] [review]:

Makes sense to me!
Attachment #8751689 - Flags: review?(fabrice) → review+
Comment on attachment 8751809 [details] [diff] [review]
Hacking AudioChannel to bypass segfault

I've adjusted my patch to detect this scenario.
Attachment #8751809 - Flags: feedback?(gsvelto) → feedback+
Pushed to pine:

The patch includes a slight change to make it forward compatible with Alexandre's patch-set in bug 1266035.
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.