1067858 - Check of nsContentUtils::IsCallerChrome() failed in HTMLMediaElement::CanPlayChanged()

Status: RESOLVED FIXED

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

Description

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

|mAudioChannelAgent->SetVisibilityState| [1] will call HTMLMediaElement::CanPlayChanged indirectly and hit nsContentUtils::IsCallerChrome().

We should also add |AutoNoJSAPI| as bug 1001383.


[1] http://hg.mozilla.org/mozilla-central/file/a5ece9451343/content/html/content/src/HTMLMediaElement.cpp#l3492
[2] http://hg.mozilla.org/mozilla-central/file/a5ece9451343/content/html/content/src/HTMLMediaElement.cpp#l3986

Comment 1

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

Attachment: 1067858_fix_HTMLMediaElement_CanPlayChanged.patch

See comment for the root cause.

Try desktop: https://tbpl.mozilla.org/?tree=Try&rev=76052493625a
Try mobile: https://tbpl.mozilla.org/?tree=Try&rev=355803e2538b
Most green.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

r=me, but can you add a test?

Comment 3

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

There is test_reactivate.html which creates media elements inside an iframe and then destroys the iframe and attaches the media elements to the parent document. By doing so, we are able to test how an media element responds to changes in document activity/visibilty as well as the audio channel attached to the element.

Btw, this bug prevents an media element from resuming playback when document switch from invisible to visible.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> There is test_reactivate.html

Which is not failing right now, right?  Why is it not failing?  It would be good to have a test that reliably fails without this patch and passes with it.

Comment 5

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/220f51d5b0a8

Comment 6

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

It did fail (frequent orange) on B2G desktop which however disables the whole media mochitests.

I found this bug while rewriting test_reactivate.html to reduce active cubeb streams in order not to hit bug 847903 where it causes crash on MacOSX when the number of active cubeb streams exceeds the limit.

Without this patch, the re-written test_reactivate.html will be perma-orange on B2G emulator. Btw, the re-written test_reactivate.html works well on all desktop platforms for they don't involve audio channel.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Without this patch, the re-written test_reactivate.html will be perma-orange

OK.  So the point is the test for this is still to land in some other bug?

Comment 8

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

Yes, the re-written test_reactivate.html will be in bug 1059265 which depends on this bug.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/220f51d5b0a8

Comment 10

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

Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

Approval Request Comment
[Feature/regressing bug #]:unknown
[User impact if declined]:mute/pause status of a media element couldn't be updated correctly and results in abnormal/unexpected mute/pause status.
[Describe test coverage new/current, TBPL]:
try: https://tbpl.mozilla.org/?tree=Try&rev=45680735e952
[Risks and why]: low, the patch is simple and is exercised on central for a while.
[String/UUID change made/needed]:none

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to JW Wang [:jwwang] from comment #10)
> Approval Request Comment
> [Feature/regressing bug #]:unknown

It's known that this issue impacted 34 and 35. Does it also impact 33 and 31?

Comment 12

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

Beta (33) is also affected. Where can I download the repo of 32/31 to check this issue?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-release
http://hg.mozilla.org/releases/mozilla-esr31

Comment 14

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

Btw, since this issue only affects B2G trunk where audio channel is enabled, we should be fine with uplifting to only Aurora/Beta in order to fix test failures in bug 1059265.

Comment 15

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #13)
> http://hg.mozilla.org/releases/mozilla-release
> http://hg.mozilla.org/releases/mozilla-esr31

Both are affected. But they are for desktop browser release only, right?

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well 32 is also http://hg.mozilla.org/releases/mozilla-b2g32_v2_0/.

Comment 17

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> Well 32 is also http://hg.mozilla.org/releases/mozilla-b2g32_v2_0/.

So we should uplift to Aurora, Beta, and b2g32.

Comment 18

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

Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

Approval Request Comment
[Feature/regressing bug #]:unknown
[User impact if declined]:mute/pause status of a media element couldn't be updated correctly and results in abnormal/unexpected mute/pause status.
[Describe test coverage new/current, TBPL]:
Try: https://tbpl.mozilla.org/?tree=Try&rev=128480b95e2c
[Risks and why]: low, the patch is simple and is exercised on central for a while.
[String/UUID change made/needed]:none

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

Taking it for beta 6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2676482b6ab0
https://hg.mozilla.org/releases/mozilla-beta/rev/de5e77b26504

Comment 21

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

Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: mute/pause status of a media element couldn't be updated correctly and results in abnormal/unexpected mute/pause status.
Testing completed: 
Try: https://tbpl.mozilla.org/?tree=Try&rev=e5501a3825ea
Risk to taking this patch (and alternatives if risky): Low. The patch is simple and is exercised on central for a while.
String or UUID changes made by this patch:none

Comment 22

[bhavana bajaj [:bajaj]]

Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

Approving the uplift, given the user impact..

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/26b6a3897158

Comment 24

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/26b6a3897158