Closed Bug 1214148 Opened 4 years ago Closed 4 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla45
blocking-b2g 2.5+
Tracking Status
firefox44 --- affected
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed
b2g-master --- fixed

People

(Reporter: jj.evelyn, Assigned: baku)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [ft:conndevices][partner-blocker][platform][backout-asap])

Attachments

(6 files, 5 obsolete files)

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.
@Luke, could you write down the idea we came out in the offline discussion? Thanks!
Flags: needinfo?(lchang)
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?
Flags: needinfo?(ehung)
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 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.
Flags: needinfo?(ehung)
(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?
Flags: needinfo?(lchang)
After offline discussion with Baku, he would help with this issue.
blocking-b2g: --- → 2.5+
Priority: -- → P1
Whiteboard: [ft:conndevices][partner-blocker]
Blocks: TV_P1
Blocks: 1206621
Blocks: 1206805
Whiteboard: [ft:conndevices][partner-blocker] → [ft:conndevices][partner-blocker][platform]
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
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.
Blocks: 1199985
Attached patch nested.patch (obsolete) — Splinter Review
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.
Attachment #8676811 - Flags: review?(kchen)
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?
Attachment #8676811 - Flags: review?(kchen) → review+
> > +[builtinclass, scriptable, uuid(8e49f7b0-1f98-4939-bf91-e9c39cd56434)]
> 
> Why is this uuid changed?

because I made the class 'builtinclass'. Is it ok?
(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.
Duplicate of this bug: 1199985
Can you change if this patch is enough to fix this issue?
Flags: needinfo?(alwu)
I meant: can you _check_ if this patch is enough?
Kanru, Seems like baku has missed NI flag for you.
Flags: needinfo?(kchen)
This patch looks good for me!
But I have some little questions about this patch, we can discuss offline.
Flags: needinfo?(alwu)
An easy way to test is to run TV simulator and reproduce by steps in bug 1199985.
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.
Flags: needinfo?(swu)
Attached patch nested.patch (obsolete) — Splinter Review
patch with comments fixed.
Attachment #8676811 - Attachment is obsolete: true
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
Flags: needinfo?(swu)
(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.
Attached patch part 2 - changes propagation (obsolete) — Splinter Review
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.
Attachment #8681304 - Flags: review?(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.
Flags: needinfo?(kchen)
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.
Flags: needinfo?(amarchesini)
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.
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
> 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?
Flags: needinfo?(amarchesini)
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.
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.
Hi, Baku,
If there is anything wrong, please correct me!
Thanks!
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1213666
Blocks: 1223298
QA Whiteboard: [COM=TV Firefox Account]
QA Whiteboard: [COM=TV Firefox Account] → [COM=TV Browser]
Hi Baku,
Do you have any feedback for the patch Alastor provided?
Thanks!
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 :)
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!
After offline discussion, we already have the solution.
Baku would update the patch soon \o/
Attachment #8680678 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
This patch works and fixes the problem. Alwu, can you review it?
I don't used a PassiveAgent but TabParent directly in ACS.
Attachment #8681304 - Attachment is obsolete: true
Attachment #8681304 - Flags: review?(alwu)
Attachment #8691501 - Flags: review?(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)|
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
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].
This is an hack, but it works locally. If you like it, I want a feedback from fabrice as well.
Attachment #8691930 - Flags: review?(alwu)
(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 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 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.
Attachment #8691501 - Flags: review?(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.
Attachment #8691930 - Flags: review?(alwu)
> 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.
> > +  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.
(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!
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.
baku and alwu,
This bug is fixed thanks to your great teamwork!
So great we have it landed, thank you Alaster and Baku!  Could you create 2.5 uplift request?
Flags: needinfo?(amarchesini)
Flags: needinfo?(alwu)
(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?
Flags: needinfo?(amarchesini)
Flags: needinfo?(swu)
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
Attachment #8691500 - Flags: approval‑mozilla‑b2g44?
Flags: needinfo?(swu)
Attachment #8691501 - Flags: approval‑mozilla‑b2g44?
Attachment #8691930 - Flags: approval‑mozilla‑b2g44?
Flags: needinfo?(alwu)
Whiteboard: [ft:conndevices][partner-blocker][platform] → [ft:conndevices][partner-blocker][platform][backout-asap]
Depends on: 1232359
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.
Attachment #8691500 - Flags: qa-approval-
Sorry for the delay, I'll check these regression issues.
I'll reopen all dependency issues and present the solutions for them.
Blocks: 1232348
No longer depends on: 1232348
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
Hi, Baku,
Now we can land this bug.
Flags: needinfo?(amarchesini)
Comment on attachment 8691500 [details] [diff] [review]
patch 1 - propagation from the nested iframe back to the toplevel iframe

Approved for TV 2.5
Attachment #8691500 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8691501 [details] [diff] [review]
patch 2 - from toplevel iframe to the nested iframe

Approved for TV 2.5
Attachment #8691501 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Comment on attachment 8691930 [details] [diff] [review]
patch 3 - correct window for nested iframes

Approved for TV 2.5
Attachment #8691930 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Hi Tomcat, would you help to uplift patch to b2g44_v2_5 branch?  Thanks!
Flags: needinfo?(cbook)
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)
Flags: needinfo?(cbook) → needinfo?(swu)
Baku or Alaster, would you please take a look at this uplift conflict?  Thanks!
Flags: needinfo?(swu)
Flags: needinfo?(amarchesini)
Flags: needinfo?(alwu)
s/Alaster/Alastor/  :)
Tomcat, do you want to have this patch for which tree?
Flags: needinfo?(cbook)
Flags: needinfo?(amarchesini)
Flags: needinfo?(alwu)
(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
Flags: needinfo?(cbook) → needinfo?(amarchesini)
Hi Baku,
Could you help check the conflict when merging on http://hg.mozilla.org/releases/mozilla-b2g44_v2_5 branch?
Thanks!
Flags: needinfo?(amarchesini)
Attached patch patch for b2g 44 - part 2 (obsolete) — Splinter Review
Here the patches for b2g 44.
Flags: needinfo?(jocheng)
Thanks Baku,


Hi tomcat,

Could you try merge again with the new patches?
Flags: needinfo?(jocheng) → needinfo?(cbook)
Attached patch patch for b2g 44 - part 2 (obsolete) — Splinter Review
Attachment #8711094 - Attachment is obsolete: true
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 ? :)
Flags: needinfo?(amarchesini)
Attachment #8712083 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Flags: needinfo?(cbook)
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
Flags: needinfo?(amarchesini)
Precision: When I said vibrator only in my previous comment, it means that when the timer/alarm start, no sound is audible.
> 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.
Flags: needinfo?(amarchesini)
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 ?
You also need to merge the changeset of bug1232363 into your gaia.
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.