1214148 - AudioChannel API design doesn't fit into nested mozbrowser iframe case.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Evelyn Hung]

It's common in FxOS to have nested (3-layer or more) mozbrowser-iframe to load a video content, but we can't reply on a 3rd party app correctly managing audio channel policy. It looks like we need to tweak the whole mechanism a bit.

Here are two use cases we want to fulfill:

1. On TV, Browser is an app (different from phone which is part of System app), using mozbrowsr-iframe embedded by System app. Browser app uses mozbrowsr-iframe embeds other web pages. We want System app being able to control the whole audio status of the browser app. If browser app is allowed to play, then it should correctly manage which tab is playing; if browser app is NOT allowed to play, no matter what browser app says, all tabs should be muted.

2. There are some cases like Youtube app embeds youtube site, Marketplace app embeds Marketplaces site, ... etc. In these cases, the app is more like a relay in order to launch a website in a proper context. We can't rely on its effort to manage audio channels. Instead, System app should be able to directly control the status of video content.

Comment 1

[Evelyn Hung]

@Luke, could you write down the idea we came out in the offline discussion? Thanks!

Comment 2

[Andrea Marchesini [:baku]]

Can you tell me more about why the current setup doesn't work?
To me it seems that system app has access to the audio channel policy for each moz-iframe (top level ones). That means that it can mute the browser app. If the browser app has a nested moz-iframe and that is not muted following that the system app decides for the browser app, this seems a bug to me.

Any nested iframe should follow the top iframe policy. Is it enough?

Comment 3

[Andrea Marchesini [:baku]]

I spoke with Alastor on IRC and I see the reason why we want this issue fixed.
Wondering if we can implement this:

1. nested moz-iframe should follow the parent iframe policy
2. activity events should dispatched to the parent and to the child iframes.

Comment 4

[Evelyn Hung]

Yes, it's basically what we think of. We also want an attribute to indicate the parent iframe wanted/unwanted to be involved in the whole process. Luke can explain more.

Comment 5

[Luke Chang [:lchang] (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #3)
> I spoke with Alastor on IRC and I see the reason why we want this issue
> fixed.
> Wondering if we can implement this:
> 
> 1. nested moz-iframe should follow the parent iframe policy
> 2. activity events should dispatched to the parent and to the child iframes.

Yes, it's exactly what we intend to propose except that

3. every mozbrowser-iframes should set a specific attribute to inform Gecko that it wants to control audio policy. If there's any iframe that doesn't set this attribute, Gecko is unnecessary to wait for its response and just let the events to be propagated to its parents.


That's because under current design, the audio channel playback will be pending until "setMuted" is called. If some embedders don't handle these events, the audio will never play.

For example, a third-party app contains only one mozbrowser-iframe and loads some web pages in it. The app may not need to care about the audio policy. What it needs is to let its parent to decide whether the whole app (it and its web page) can play the audio or not.

Is that feasible?

Comment 6

[Alastor Wu [:alwu]]

After offline discussion with Baku, he would help with this issue.

Comment 7

[Andrea Marchesini [:baku]]

Yes, I'm working on this bug. I just need to understand a bit more how nested mozbrowser iframes are executed and in which processes they run.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: nested.patch

This first patch covers the signaling of nested iframes to top-level moz-browser iframes. Still the volume is disconnected from the parent one and I'm planning to do it in a separate patch.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8676811 [details] [diff] [review]
nested.patch

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

LGTM. So the system app has to have all audio-channel permissions. I guess it already has?

::: dom/browser-element/BrowserElementAudioChannel.cpp
@@ +479,5 @@
> +    if (!tabParent) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    RefPtr<TabParent> tp = static_cast<TabParent*>(tabParent.get());

Use TabParent::GetFrom

::: dom/browser-element/mochitest/browserElement_AudioChannel_nested.js
@@ +11,5 @@
> +
> +function runTests() {
> +  var iframe = document.createElement('iframe');
> +  iframe.setAttribute('mozbrowser', 'true');
> +  iframe.setAttribute('mozapp', 'http://example.org/manifest.webapp');

Note to set mozapp properly you might want to add 'embed-apps' permission to this document too.

@@ +21,5 @@
> +    } else if (/^KO/.exec(message)) {
> +      ok(false, "Message from app: " + message);
> +    } else if (/DONE/.exec(message)) {
> +      ok(true, "Messaging from app complete");
> +      iframe.removeEventListener('mozbrowsershowmodalprompt', listener);

Ensure we reach here before we call SimpleTest.finish()

@@ +72,5 @@
> +}
> +
> +addEventListener('load', function() {
> +  SimpleTest.executeSoon(runTests);
> +});

Should listen the 'testready' event.

::: dom/browser-element/mochitest/file_browserElement_AudioChannel_nested.html
@@ +16,5 @@
> +
> +  addEventListener('load', function(e) {
> +    var iframe = document.createElement('iframe');
> +    iframe.setAttribute('mozbrowser', 'true');
> +    iframe.setAttribute('remote', 'true');

Add a comment saying that set 'remote' to true here will make the the iframe remote in _inproc_ test and in-process in _oop_ test. It's a little bit confusing like this because we do not allow nested-oop iframe.

Does the test work in both cases?

::: dom/html/nsGenericHTMLFrameElement.cpp
@@ +191,5 @@
>    return loader.forget();
>  }
>  
>  NS_IMETHODIMP
> +nsGenericHTMLFrameElement::GetParentApplication(mozIApplication **aApplication)

nit: ** following the type. This file needs some formating but let's stick with this style.

::: dom/interfaces/base/nsITabParent.idl
@@ +4,5 @@
>  
>  
>  #include "domstubs.idl"
>  
> +[builtinclass, scriptable, uuid(8e49f7b0-1f98-4939-bf91-e9c39cd56434)]

Why is this uuid changed?

Comment 10

[Andrea Marchesini [:baku]]

> > +[builtinclass, scriptable, uuid(8e49f7b0-1f98-4939-bf91-e9c39cd56434)]
> 
> Why is this uuid changed?

because I made the class 'builtinclass'. Is it ok?

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> > > +[builtinclass, scriptable, uuid(8e49f7b0-1f98-4939-bf91-e9c39cd56434)]
> > 
> > Why is this uuid changed?
> 
> because I made the class 'builtinclass'. Is it ok?

OK! I missed that change in splinter.

Comment 13

[Andrea Marchesini [:baku]]

Can you change if this patch is enough to fix this issue?

Comment 14

[Andrea Marchesini [:baku]]

I meant: can you _check_ if this patch is enough?

Comment 15

[Mahendra Potharaju [:mahe]]

Kanru, Seems like baku has missed NI flag for you.

Comment 16

[Alastor Wu [:alwu]]

This patch looks good for me!
But I have some little questions about this patch, we can discuss offline.

Comment 17

[Shian-Yow Wu [:swu]]

An easy way to test is to run TV simulator and reproduce by steps in bug 1199985.

Comment 18

[Andrea Marchesini [:baku]]

swu, I need to know if for 2.5 I need to write the 'second' part of this patch of this is OK and we can work on the settings of values as follow up.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: nested.patch

patch with comments fixed.

Comment 20

[Shian-Yow Wu [:swu]]

Hi baku, could you explain more about what the second part patch will do?

For 2.5 we must fix the blocker bug 1199985, which was marked as duplicated to this bug.

I tested the latest nested.patch by B2G desktop + TV gaia (a.k.a. TV simulator[1]), but the issue still exists.

My test scenario is:
1. Run TV simulator
2. Launch Browser App
3. Go to YouTube and watch a video

The video stucks at the 1st frame.  The reason why video stuck is related to this bug can be referred in bug 1199985.

[1] https://developer.mozilla.org/en-US/Firefox_OS/TVs_connected_devices

Comment 21

[Andrea Marchesini [:baku]]

(In reply to Shian-Yow Wu [:swu] from comment #20)
> Hi baku, could you explain more about what the second part patch will do?

Exactly what we need to fix this STR. This second patch will propagate what the system does to the nested iframe.
Currently from the system app we control just the first iframe. So, if the second one is muted, it will not be unmuted when the system app unmutes the first one.

Working on the second part of this bug.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 2 - changes propagation

This is the second patch. Alwu can you test it?
There is a mochitest but I'm not able to test it with a real device.

Comment 23

[Alastor Wu [:alwu]]

After applying these two patches, the video in the youtube stll can't be playback in the TV simulator.
I'm still finding the reason.

Comment 24

[Alastor Wu [:alwu]]

Hi, Baku,
Our situation is the system app in the b2g process, and Browser and Youtube apps are in the content process.
After discuss with Kanru, it seems that your patch didn't include this case.
In addition, we still need to propagate the event to the system app.

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8680678 [details] [diff] [review]
nested.patch

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

::: dom/browser-element/BrowserElementAudioChannel.cpp
@@ +477,5 @@
> +  if (!wrapper) {
> +    nsCOMPtr<nsITabParent> iTabParent = do_QueryInterface(aSubject);
> +    if (!iTabParent) {
> +      return NS_ERROR_FAILURE;
> +    }

When both frames are in the same process there is no TabParent so this would fail to handle the case.

Comment 26

[Alastor Wu [:alwu]]

In addition, since you change the GenerateAllowedAudioChannels(), you should also modify the AudioChannelManager [1].
 
[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioChannelManager.cpp#211

Comment 27

[Andrea Marchesini [:baku]]

> Our situation is the system app in the b2g process, and Browser and Youtube
> apps are in the content process.

I do cover this case. I also have a mochitest for that. Can we talk tomorrow morning on IRC?

Comment 28

[Alastor Wu [:alwu]]

The nested mozbrowser frame case is like that,
> System app -> Outside app -> inside app
The nested apps might be INPROC or OOP, so we need to consider all possible situations.

Here are two important thing we need to handle.

> (P1) Propagate the event "activestatechanged" from the inside app to the outside app.
> (P2) Propagate the mute/volume changing from outside app to the inside app.

---

Before we discuss the Gecko solution, there still one thing to be notice.

To reduce the developing difficulty, we don't hope that the outside app have to write some logic to manage the audio channel of the inside nested app. If the outside app have the "audio focus", it should also be in the inside app.

Therefore, since no one would call "insideApp.allowedaudiochannel", the BrowserElementAudioChannel of the inside app wouldn't be created in the inside app. 

So, here are other two important things, 

> (P3) The BrowserElementAudioChannel doesn't be created in the inside app. 
> (P4) The mute/volume control would only happen on the outside app.

Comment 29

[Alastor Wu [:alwu]]

Let's start to see your patch whether can work well under these limitations.

CASE 1, nested frame happens in OOP

>    b2g process          content process
> [ (system app) ]  [ (outside app) (inside app) ]

---

Let's see whether (P1) can work successfully.

When the AudioChannelService sends the "audiochannel-activity" event, the window ID of the wrapper is the ID of the "inside app".
Since the (P3) limitation, we would send this event to outside app's BrowserElementAudioChannel, then we get fail because the window ID is not the same.

Therefore, the (P1) fails.

---

Let's see whether (P2) can work successfully.

Because of (P4), we would only call "outsideApp.allowedaudiochannels[idx].setMute(false)". When we do that, we would use the BrowserElemenetAPI to notify the remote AudioChannelService.

Then, we call RefreshAgentsVolume(), using the outside app's window ID to get agents which we want to notify. 

Since the MediaElements in the inside app register with the inside app's window ID, we can't find them by outside app's window ID.

Therefore, the (P2) fails.

---

The INPROC case is the same, we would still get fail because of the different windowID.


CASE 2, nested frame happens in INPROC

>                b2g process          
> [ (system app) (outside app) (inside app) ]

---

Here I have a rough idea to solve this problem.
If we always use the outside frame as the value of the |mWindow| in the AudioChannelAgent, I think the problem can be solved.

But for the outside app, the drawback is that it can't implement "insideApp.allowedaudiochannel" anymore, because all elements would be regards as the part of the outside frame.

Comment 30

[Alastor Wu [:alwu]]

Hi, Baku,
If there is anything wrong, please correct me!
Thanks!

Comment 32

[Josh Cheng [:josh]]

Hi Baku,
Do you have any feedback for the patch Alastor provided?
Thanks!

Comment 33

[Alastor Wu [:alwu]]

After offline sync with Baku, now he is trying to reproduce this issue.
I think we would get some progress few days later.
Thanks Baku :)

Comment 34

[Josh Cheng [:josh]]

Hi Baku,
Partner TV porting FxOS 2.5 is pending on this issue and expect to start by Dec 1st. 
Really appreciate if you could put this issue as high priority in your bucket. 
Thank you very much!

Comment 35

[Alastor Wu [:alwu]]

After offline discussion, we already have the solution.
Baku would update the patch soon \o/

Comment 36

[Andrea Marchesini [:baku]]

Attachment: patch 1 - propagation from the nested iframe back to the toplevel iframe

Comment 37

[Andrea Marchesini [:baku]]

Attachment: patch 2 - from toplevel iframe to the nested iframe

This patch works and fixes the problem. Alwu, can you review it?
I don't used a PassiveAgent but TabParent directly in ACS.

Comment 38

[Alastor Wu [:alwu]]

Here we have three different cases need to handle with.
Note, app2 is the nested mozbrowser frame.

Case 1 : all in content process       , [systemApp] [app1 + app2]
Case 2 : all in b2g process           , [systemApp + app1 + app2]
Case 3 : one in b2g & one in content  , [systemApp + app1] [app2]


In addition, we have two behaviors need to be implemented.
Behavior 1 : propagation from the "nested iframe" to the "topLevel iframe"
Behavior 2 : propagation from the "topLevel iframe" to the "nested iframe"

---

I test the latest patches, and they still can't solve this issue.
Error results are describe in following table.

------------------------------------------------------------------
|        |  from "nested iframe"    |  from "topLevel iframe"    |
|        |  to   "topLevel iframe"  |  to   "nested iframe"      |
------------------------------------------------------------------
|        | checking failed cause of | get wrong winData in ACS   |
| case 1 | wrong windowID in the    | cause we use the app1's    |
|        | TabChild [1]             | window, instead app2's [2] |
------------------------------------------------------------------
|        | checking failed cause of | get wrong winData in ACS   |
| case 2 | wrong windowID in the    | cause we use the app1's    |
|        | BEAC [3]                 | window, instead app2's [2] |
------------------------------------------------------------------
|        | checking failed cause of | get wrong winData in ACS   |
| case 3 | wrong window in the BEAC | cause we use the app1's    |
|        | [4]                      | window, instead app2's [2] |
------------------------------------------------------------------

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#695
[2] https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#815
[3] https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementAudioChannel.cpp#606
[4] In baku's patch1, BrowserElementAudioChannel.cpp, line#488, |if (window == mFrameWindow)|

Comment 39

[Alastor Wu [:alwu]]

Because we got fail in the behavior 1, I can't check whether the behavior 2 can work successfully.
Therefore, I check the code-flow, and made these assumption for the error situations of behavior 2.

---

Here are my testing methods.
Case 1 : reproduce using [1]
Case 2 : reproduce using [2]
Case 3 : now we don't have the such situation, so I didn't test it.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1213666#c0
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1214148#c20

Comment 40

[Andrea Marchesini [:baku]]

You are totally right. I was running all my test with dom.ipc.tabs.disabled=false so that my setup was: [systemApp - app1] [app2].
Now I can reproduce this setup [systemApp - app1 - app2].

Comment 41

[Andrea Marchesini [:baku]]

Attachment: patch 3 - correct window for nested iframes

This is an hack, but it works locally. If you like it, I want a feedback from fabrice as well.

Comment 42

[Gabriele Svelto [:gsvelto]]

(In reply to Alastor Wu [:alwu] from comment #38)
> Case 3 : one in b2g & one in content  , [systemApp + app1] [app2]

I think this is the callscreen case as I'm not aware of other apps living in the main process. If that's the case then it won't last for long; I'm working on removing the callscreen from the system app process and moving it back to the dialer as a regular app in bug 1226195.

Comment 43

[Alastor Wu [:alwu]]

Comment on attachment 8691500 [details] [diff] [review]
patch 1 - propagation from the nested iframe back to the toplevel iframe

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

::: dom/browser-element/BrowserElementAudioChannel.cpp
@@ +466,5 @@
>      return NS_OK;
>    }
>  
>    nsCOMPtr<nsISupportsPRUint64> wrapper = do_QueryInterface(aSubject);
> +  // This can be a nested iframe.

nit : modify comment, to describe this case is about [systemApp - app1] [app2]

Comment 44

[Alastor Wu [:alwu]]

Comment on attachment 8691501 [details] [diff] [review]
patch 2 - from toplevel iframe to the nested iframe

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

::: dom/ipc/TabParent.cpp
@@ +3403,5 @@
> +  if (!mFrameElement || !mFrameElement->OwnerDoc()) {
> +    return;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindow> window = mFrameElement->OwnerDoc()->GetWindow();

Just curious, if the app is OOP, the window should be null, because the window is in another process so that we can't get it, right?

----

Sometime, we would get the run-time crash on above two conditions, the mFrameElement seems would become a invalid pointer for unknown reason.

I don't have the specific reproduce steps now, but it's easy to reproduce when you try to play audio from different apps.

here are the crash stacks.

(1)
Program received signal SIGSEGV, Segmentation fault.
mozilla::dom::TabParent::AudioChannelChangeNotification (this=0xa820a800, aWindow=aWindow@entry=0xb06cee90,
    aAudioChannel=aAudioChannel@entry=mozilla::dom::Normal, aVolume=1, aMuted=true)
    at /Users/Alastor/mozilla-central/dom/ipc/TabParent.cpp:3413
3413	  if (!mFrameElement || !mFrameElement->OwnerDoc()) {
(gdb) bt
#0  mozilla::dom::TabParent::AudioChannelChangeNotification (this=0xa820a800, aWindow=aWindow@entry=0xb06cee90,
    aAudioChannel=aAudioChannel@entry=mozilla::dom::Normal, aVolume=1, aMuted=true)
    at /Users/Alastor/mozilla-central/dom/ipc/TabParent.cpp:3413
#1  0xb4a390fc in mozilla::dom::AudioChannelService::RefreshAgentsVolumeAndPropagate (this=0xac2c1700, this@entry=0xbee86688,
    aAudioChannel=mozilla::dom::Normal, aAudioChannel@entry=2842790400, aWindow=0xb06cee90, aWindow@entry=0xbee866c4)
    at /Users/Alastor/mozilla-central/dom/audiochannel/AudioChannelService.cpp:599
#2  0xb4a3952e in mozilla::dom::AudioChannelService::SetAudioChannelMuted (this=0xbee86688, aWindow=0xbee866c4, aAudioChannel=2842790400,
    aMuted=aMuted@entry=true) at /Users/Alastor/mozilla-central/dom/audiochannel/AudioChannelService.cpp:859
#3  0xb47829de in mozilla::dom::BrowserElementAudioChannel::SetMuted (this=this@entry=0xa9718a00, aMuted=<optimized out>, aRv=...)
    at /Users/Alastor/mozilla-central/dom/browser-element/BrowserElementAudioChannel.cpp:460

(2)
Program received signal SIGSEGV, Segmentation fault.
nsIDocument::GetWindow (this=0x1fed) at ../../../../../../Users/Alastor/mozilla-central/dom/base/nsIDocument.h:1073
1073	    return mWindow ? mWindow->GetOuterWindow() : GetWindowInternal();
(gdb) bt
#0  nsIDocument::GetWindow (this=0x1fed) at ../../../../../../Users/Alastor/mozilla-central/dom/base/nsIDocument.h:1073
#1  0xb49f72f8 in mozilla::dom::TabParent::AudioChannelChangeNotification (this=0xa99b3800, aWindow=aWindow@entry=0xb05b8250,
    aAudioChannel=aAudioChannel@entry=mozilla::dom::Ringer, aVolume=1, aMuted=false)
    at /Users/Alastor/mozilla-central/dom/ipc/TabParent.cpp:3417
#2  0xb4a390fc in mozilla::dom::AudioChannelService::RefreshAgentsVolumeAndPropagate (this=0xac2caac0, this@entry=0xbed46688,
    aAudioChannel=mozilla::dom::Ringer, aAudioChannel@entry=2891731872, aWindow=0xb05b8250, aWindow@entry=0xbed466c4)
    at /Users/Alastor/mozilla-central/dom/audiochannel/AudioChannelService.cpp:599
#3  0xb4a3952e in mozilla::dom::AudioChannelService::SetAudioChannelMuted (this=0xbed46688, aWindow=0xbed466c4, aAudioChannel=2891731872,
    aMuted=aMuted@entry=false) at /Users/Alastor/mozilla-central/dom/audiochannel/AudioChannelService.cpp:859
#4  0xb47829de in mozilla::dom::BrowserElementAudioChannel::SetMuted (this=this@entry=0xac5c53a0, aMuted=<optimized out>, aRv=...)
    at /Users/Alastor/mozilla-central/dom/browser-element/BrowserElementAudioChannel.cpp:460
#5  0xb46afa7c in mozilla::dom::BrowserElementAudioChannelBinding::setMuted (cx=0xad803e00, obj=..., self=0xac5c53a0, args=...)
    at BrowserElementAudioChannelBinding.cpp:242

@@ +3404,5 @@
> +    return;
> +  }
> +
> +  nsCOMPtr<nsPIDOMWindow> window = mFrameElement->OwnerDoc()->GetWindow();
> +  while (window) {

Why use while-loop here? you didn't change the window within the while-loop.
You can only keep the line#3409~3413.

Comment 45

[Alastor Wu [:alwu]]

Comment on attachment 8691930 [details] [diff] [review]
patch 3 - correct window for nested iframes

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +89,5 @@
> +
> +  mWindow = window->GetScriptableTop();
> +  if (NS_WARN_IF(!mWindow)) {
> +    return NS_OK;
> +  }

why NS_OK?

@@ +106,5 @@
> +  // find apps that are not the system one.
> +  window = mWindow->GetParent();
> +  if (!window || window == mWindow) {
> +    return NS_OK;
> +  }

nit : we can add the MOZ_LOG here to print out the windowID and parentWindowID.

@@ +116,5 @@
> +
> +  nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> +  if (!doc) {
> +    return NS_OK;
> +  }

ditto, why above three conditions return NS_OK, instead of NS_ERROR?
Is ok if we don't get the result of the parentWindow/innerWindow/doc?

@@ +129,5 @@
> +
> +  if (appId == nsIScriptSecurityManager::NO_APP_ID ||
> +      appId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> +    return NS_OK;
> +  }

It might be wrong when the nested mozframe is not an app.

If we have three layer nested mozframes
- [frame1, frame2, frame3], frame1 is an app, but frame2 & 3 are not.

In this case, we can't get the window of the frame1. 
Because the frame2 is not an app, we stop to traverse the parent tree.

This case might be exist, since the bug1213666 is already using the non-app nested mozframe. (but it's only two layer, so it doesn't hit this case.) [1] is the app-mozframe, when you click the video, it would change to non-app-mozframe [2]

[1] https://goo.gl/16arRf
[2] https://goo.gl/xbA9vJ

::: dom/audiochannel/AudioChannelAgent.h
@@ +55,5 @@
>                          bool aUseWeakRef);
>  
>    void Shutdown();
>  
> +  nsresult FindCorrectWindow(nsIDOMWindow* aWindow);

nit : add short comment to describe the purpose.

Comment 46

[Andrea Marchesini [:baku]]

> Just curious, if the app is OOP, the window should be null, because the
> window is in another process so that we can't get it, right?

Correct.

> Sometime, we would get the run-time crash on above two conditions, the
> mFrameElement seems would become a invalid pointer for unknown reason.

It seems that a TabParent is not correctly unregistered. I'll debug it.

Comment 47

[Andrea Marchesini [:baku]]

> > +  mWindow = window->GetScriptableTop();
> > +  if (NS_WARN_IF(!mWindow)) {
> > +    return NS_OK;
> > +  }

We should support Agents when the device or the app are shutting down. In this case the window can be null.
Without window, we mute them but it's better to not return error because this was the previous behaviour.
In case we want to change it, we should file a separate bug.

> It might be wrong when the nested mozframe is not an app.

> If we have three layer nested mozframes
> - [frame1, frame2, frame3], frame1 is an app, but frame2 & 3 are not.

A mozframe without app? For instance? If that is a browser, it inherits the appId for the parent app.

Comment 48

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #47) 
> 
> A mozframe without app? For instance? If that is a browser, it inherits the
> appId for the parent app.

You can ignore this question if the app ID would be inherited :)
Thanks!

Comment 49

[Alastor Wu [:alwu]]

After discuss with baku, we have already found the crash reason.
Baku would update new patch later.

---

[Crash reason] 
We didn't unregister the TabParent even if it have already been destroyed.

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4865d79416
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f693237c8c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e716d9ac93d7

Comment 51

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/87e625f2b847 for https://treeherder.mozilla.org/logviewer.html#?job_id=18427813&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=18427819&repo=mozilla-inbound

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afe3d65b74b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/debd1ce280de
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3d458624eb

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/afe3d65b74b6
https://hg.mozilla.org/mozilla-central/rev/debd1ce280de
https://hg.mozilla.org/mozilla-central/rev/eb3d458624eb

Comment 54

[Blake Wu [:bwu][:blakewu]]

baku and alwu,
This bug is fixed thanks to your great teamwork!

Comment 55

[Shian-Yow Wu [:swu]]

So great we have it landed, thank you Alaster and Baku!  Could you create 2.5 uplift request?

Comment 56

[Andrea Marchesini [:baku]]

(In reply to Shian-Yow Wu [:swu] from comment #55)
> So great we have it landed, thank you Alaster and Baku!  Could you create
> 2.5 uplift request?

Is it b2g 37?

Comment 57

[Andrea Marchesini [:baku]]

Comment on attachment 8691500 [details] [diff] [review]
patch 1 - propagation from the nested iframe back to the toplevel iframe

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1113086 + nested mozbrowser iframes
User impact if declined: nested mozbrowser iframes will not able to produce audio
Testing completed: tests included in the patches
Risk to taking this patch (and alternatives if risky): this is a lot of code, but we need it.
String or UUID changes made by this patch: none

Comment 58

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8691500 [details] [diff] [review]
patch 1 - propagation from the nested iframe back to the toplevel iframe

(In reply to Andrea Marchesini (:baku) from comment #57)
Based on bug 1232359, bug 1232363, and bug 1232399, no sound can be heard on Firefox OS. Uplifting this patch queue in the current state will highly injure the 2.5 branch. I'd recommend to release management to not uplift this patch, before it's fixed and QA does extensive testing around it.

Comment 59

[Carsten Book [:Tomcat]]

backed this out from mozilla-central in 
   https://hg.mozilla.org/mozilla-central/rev/a9085a77cbd4
   https://hg.mozilla.org/mozilla-central/rev/4f75683d679e
   https://hg.mozilla.org/mozilla-central/rev/cb66ffeb6725

Comment 60

[Carsten Book [:Tomcat]]

also backed out from aurora sicne this made the merge in 
https://hg.mozilla.org/releases/mozilla-aurora/rev/98dc080d53ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/39f2a4d3cdb6
https://hg.mozilla.org/releases/mozilla-aurora/rev/306790649f34

Comment 61

[Alastor Wu [:alwu]]

Sorry for the delay, I'll check these regression issues.

Comment 62

[Alastor Wu [:alwu]]

I'll reopen all dependency issues and present the solutions for them.

Comment 63

[Alastor Wu [:alwu]]

First, we need to land the bug1232363, and then we re-land this bug.
Then landing bug1232348, we fix all these issues, and hope no other new issues :P

Comment 64

[Alastor Wu [:alwu]]

Hi, Baku,
Now we can land this bug.

Comment 65

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c216d2e13acd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3aa28049b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/abfc65af5919

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c216d2e13acd
https://hg.mozilla.org/mozilla-central/rev/fb3aa28049b5
https://hg.mozilla.org/mozilla-central/rev/abfc65af5919

Comment 67

[Josh Cheng [:josh]]

Comment on attachment 8691500 [details] [diff] [review]
patch 1 - propagation from the nested iframe back to the toplevel iframe

Approved for TV 2.5

Comment 68

[Josh Cheng [:josh]]

Comment on attachment 8691501 [details] [diff] [review]
patch 2 - from toplevel iframe to the nested iframe

Approved for TV 2.5

Comment 69

[Josh Cheng [:josh]]

Comment on attachment 8691930 [details] [diff] [review]
patch 3 - correct window for nested iframes

Approved for TV 2.5

Comment 70

[Shian-Yow Wu [:swu]]

Hi Tomcat, would you help to uplift patch to b2g44_v2_5 branch?  Thanks!

Comment 71

[Carsten Book [:Tomcat]]

Hi, this failed to uplift:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r c216d2e13acd
grafting 320302:c216d2e13acd "Bug 1214148 - patch 1 - propagation from the nested iframe back to the toplevel iframe, r=alwu"
merging dom/base/nsIFrameLoader.idl
merging dom/base/nsObjectLoadingContent.cpp
merging dom/browser-element/BrowserElementAudioChannel.cpp
merging dom/browser-element/mochitest/mochitest-oop.ini
merging dom/browser-element/mochitest/mochitest.ini
merging dom/html/nsBrowserElement.cpp
merging dom/html/nsBrowserElement.h
merging dom/html/nsGenericHTMLFrameElement.cpp
merging dom/interfaces/base/nsITabParent.idl
merging dom/system/gonk/AudioChannelManager.cpp
merging dom/xul/nsXULElement.cpp
merging dom/xul/nsXULElement.h
warning: conflicts while merging dom/browser-element/BrowserElementAudioChannel.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/html/nsBrowserElement.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/interfaces/base/nsITabParent.idl! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 72

[Shian-Yow Wu [:swu]]

Baku or Alaster, would you please take a look at this uplift conflict?  Thanks!

Comment 73

[Shian-Yow Wu [:swu]]

s/Alaster/Alastor/  :)

Comment 74

[Andrea Marchesini [:baku]]

Tomcat, do you want to have this patch for which tree?

Comment 75

[Carsten Book [:Tomcat]]

(In reply to Andrea Marchesini (:baku) from comment #74)
> Tomcat, do you want to have this patch for which tree?

hi Baku, its 

http://hg.mozilla.org/releases/mozilla-b2g44_v2_5

Comment 76

[Josh Cheng [:josh]]

Hi Baku,
Could you help check the conflict when merging on http://hg.mozilla.org/releases/mozilla-b2g44_v2_5 branch?
Thanks!

Comment 77

[Andrea Marchesini [:baku]]

Attachment: patch for b2g 44 - part 1

Comment 78

[Andrea Marchesini [:baku]]

Attachment: patch for b2g 44 - part 2

Comment 79

[Andrea Marchesini [:baku]]

Attachment: patch for b2g 44 - part 3

Here the patches for b2g 44.

Comment 80

[Josh Cheng [:josh]]

Thanks Baku,


Hi tomcat,

Could you try merge again with the new patches?

Comment 81

[Carsten Book [:Tomcat]]

done!

remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/234cf6a92b08
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b820b559cd77
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e36827603e01

Comment 82

[Carsten Book [:Tomcat]]

backed this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=24539&repo=mozilla-b2g44_v2_5

Comment 83

[Andrea Marchesini [:baku]]

Attachment: patch for b2g 44 - part 2

Comment 84

[Carsten Book [:Tomcat]]

landed again

remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d3c70ef89cbf
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/092f928aac5e
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3ed5eec0b2db

Comment 85

[Carsten Book [:Tomcat]]

backed out again - for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=24646&repo=mozilla-b2g44_v2_5

Andrea, since this is bouncing back and forth could you do the checkin next time ? :)

Comment 86

[Andrea Marchesini [:baku]]

Attachment: patch for b2g 44 - part 2

Comment 87

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d38330c362a4
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5f42d937988e
remote:   https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2b6f8826861a

Comment 88

[u535950]

Hello !

These commits break alarm and timer sounds in v2.5 (vibrator only).
Moreover, it's impossible to listen sounds when I go to Settings > Sound > Ringtones. Only a 'bip' is listened.

Is it possible to back out these commits ?

My device : ZTE Open C FR
Gecko/Gaia version: v2.5

Comment 89

[u535950]

Precision: When I said vibrator only in my previous comment, it means that when the timer/alarm start, no sound is audible.

Comment 90

[Andrea Marchesini [:baku]]

> Is it possible to back out these commits ?

Instead reverting this commits, can we debug why we have this issue and fix it?
Tell me more how we can reproduce this issue. Thanks.

Comment 91

[u535950]

I have this issue on my ZTE Open C FR on B2G v2.5.
Git Informations :
2016-02-08 15:34:34
28880004

The STR are :
- Open Clock application
- Set an alarm or a timer on 1 minute, and wait

Happened : The popup is shown and device vibrate, but no sound is heareable from the device.

Can you verify on your side that alarm/timer work on v2.5 ?

Comment 92

[Alastor Wu [:alwu]]

You also need to merge the changeset of bug1232363 into your gaia.

Comment 93

[u535950]

(In reply to Alastor Wu [:alwu] from comment #92)
> You also need to merge the changeset of bug1232363 into your gaia.

Yes, indeed. I already tried the patch of bug 12322363 on my build (apply patch + flash gaia), but it I had not resetted my device, so it didn't work.

After new try with reset, the issue for alarm/timer has gone.