1044773 - Assertion failure: aWindow || XRE_GetProcessType() == GeckoProcessType_Default, at ../../../gecko/dom/audiochannel/AudioChannelAgent.cpp:84

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=44593135&tree=Try&full=1#error3

Ran into this assertion while running media mochitests on B2G debug.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

It looks like it is possible for window to be null. Should we just remove the assertion at [1]

[1] http://hg.mozilla.org/mozilla-central/file/de8c0f0e74a2/dom/audiochannel/AudioChannelAgent.cpp#l84

Comment 2

[Andrea Marchesini [:baku]]

> http://hg.mozilla.org/mozilla-central/file/de8c0f0e74a2/dom/audiochannel/
> AudioChannelAgent.cpp#l84

Is it 100% reproducible? Can you tell me which mochitest is failing? I want to understand why this happens before removing the assertion. Thanks.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

No particular test and it is not always repdocucible. I tried to run mochitest-3 about 10 times on B2G opt build (https://tbpl.mozilla.org/?tree=Try&rev=28d4bf99574f) and observed it once. However, it didn't crash for it was an opt build.

logcat output:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/cd409ea44cae06c55f061fa375ed5d79bf7df6e4ceff361f7a15f904384392131e1d80cd25a517b3910c2f9b8ff63406b8bc6077bd21c9a8557b21d78d850ba5
Search for the keyword |UpdateAudioChannelPlayingState window=0|.

HTMLMediaElement.cpp with log messages:
https://hg.mozilla.org/try/diff/1b21c4f99b15/content/html/content/src/HTMLMediaElement.cpp

Comment 4

[Andrea Marchesini [:baku]]

Attachment: assert.patch

Talking with bz it seems that it's possible to have a HTMLMediaElement in a document without a window: for instance when the element is from an XHR response.

The window is used to change the volume of any inner audio element, so this assert can be removed.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

The log message is curious:
2014-07-28 13:31:31.520039 UTC - 1074443400[40245080]: 45222bc0 UpdateAudioChannelPlayingState window=0 play=0

It doesn't seem to make sense to create an nsIAudioChannelAgent object in order to call StopPlaying(). I wonder if it has something to do with GC/CC. If so, we should return from UpdateAudioChannelPlayingState() immeidately when |OwnerDoc()->GetWindow()| is null for the media element object is CCed and in an unstable state.

I will try to add more logs to verify it.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

logcat output:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/f02903dc5f103c8f0ae7dc0cc1eb629cfec6d0e4dea526561fe63c93ea4af6afefae3d3ab1d575d07b5853bad4699dd8329e7c8b4b11f9118bd584acbfa5f13f

add logs to trace CC:
https://hg.mozilla.org/try/diff/33a9de92ae7e/content/html/content/src/HTMLMediaElement.cpp


07-29 04:36:45.369 I/PRLog   (  769): 2014-07-29 04:36:45.376087 UTC - 1074443400[40245080]: 432dfd60 cycle collection, unlink mAudioChannelAgent
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.522642 UTC - 1074443400[40245080]: 4435c3e0 cycle collection, unlink mAudioChannelAgent
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.524135 UTC - 1074443400[40245080]: 432ddde0 cycle collection, unlink mAudioChannelAgent
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.524386 UTC - 1074443400[40245080]: 4435d3a0 cycle collection, unlink mAudioChannelAgent
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.524619 UTC - 1074443400[40245080]: 432de5c0 cycle collection, unlink mAudioChannelAgent
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.524880 UTC - 1074443400[40245080]: 4435d8e0 cycle collection, unlink mAudioChannelAgent
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.525508 UTC - 1074443400[40245080]: Decoder=43d53f80 ChangeState PAUSED => PAUSED
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.526277 UTC - 1074443400[40245080]: 432dfd60 UpdateAudioChannelPlayingState window=0 play=0
07-29 04:36:45.519 I/PRLog   (  769): 2014-07-29 04:36:45.526749 UTC - 1074443400[40245080]: 432dfd60 UpdateAudioChannelPlayingState StopPlaying

As we can see from the logs, 432dfd60 is called on UpdateAudioChannelPlayingState() after CC which unlinks |mAudioChannelAgent|.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

> I will try to add more logs to verify it.

Can you please verify it on top of this patch? Here I check if mHasSelfReference is set to true. In theory, mHasSelfReference should be true if the object is kept alive, and it should be false after the unlink.

Thanks!

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8463991 [details] [diff] [review]
patch

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ -80,5 @@
>                                  nsIAudioChannelAgentCallback *aCallback,
>                                  bool aUseWeakRef, bool aWithVideo)
>  {
> -  // We need the window only for IPC.
> -  MOZ_ASSERT(aWindow || XRE_GetProcessType() == GeckoProcessType_Default);

Running the tests. I think you have to restore the assertion here to know whether the change in HTMLMediaElement.cpp works.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

with the patch: https://tbpl.mozilla.org/?tree=Try&rev=875cd5db8370
without the patch: https://tbpl.mozilla.org/?tree=Try&rev=d836f616fc68

The patch seems to cause perma-failure of test_audioChannelChange.html.

Comment 10

[Andrea Marchesini [:baku]]

> The patch seems to cause perma-failure of test_audioChannelChange.html.

Thanks for the debugging. I suspect that in any case we should land the first version of the patch that removes the assertion because of comment 4.

I would like to ask roc 2 things:

1. a nice way to know if the HTMLMediaElement is already GCed/CCed and, in this case, don't call UpdateAudioChannelPlayingState(). I don't want to add an additional boolean if we already have something.

2. Just to confirm, is it correct that we can have a HTMLMediaElement where OwnerDoc()->GetWindow() is null ?

Thanks!

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> 1. a nice way to know if the HTMLMediaElement is already GCed/CCed and, in
> this case, don't call UpdateAudioChannelPlayingState(). I don't want to add
> an additional boolean if we already have something.

I don't know of any. A boolean flag which you set during Unlink() makes sense.

> 2. Just to confirm, is it correct that we can have a HTMLMediaElement where
> OwnerDoc()->GetWindow() is null ?

Yes.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although, why are we calling UpdateAudioChannelPlayingState() after CC? Seems to me Unlink() should remove enough state that nothing can call UpdateAudioChannelPlayingState() on that HTMLMediaElement afterward. Do we need to tell the decoder to shutdown in Unlink(), maybe?

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch

This patch should fix the problem, at least the AudioChannelAgent part:

1. We don't need to create a new AudioChannelAgent when we are not playing, just to call StopPlaying().

2. The assertion about the window is wrong and has to be removed.

Comment 14

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e55d691f0a2

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e55d691f0a2