Closed Bug 1067858 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
firefox34 + fixed
firefox35 --- fixed
firefox-esr31 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

|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
Blocks: 1059265
Assignee: nobody → jwwang
Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

r=me, but can you add a test?
Attachment #8490518 - Flags: review?(bzbarsky) → review+
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.
Keywords: checkin-needed
> 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.
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.
> 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?
Yes, the re-written test_reactivate.html will be in bug 1059265 which depends on this bug.
https://hg.mozilla.org/mozilla-central/rev/220f51d5b0a8
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8490518 - Flags: approval-mozilla-aurora?
(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?
Flags: needinfo?(jwwang)
Beta (33) is also affected. Where can I download the repo of 32/31 to check this issue?
Flags: needinfo?(jwwang)
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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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
Attachment #8490518 - Flags: approval-mozilla-beta?
Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

Taking it for beta 6
Attachment #8490518 - Flags: approval-mozilla-beta?
Attachment #8490518 - Flags: approval-mozilla-beta+
Attachment #8490518 - Flags: approval-mozilla-aurora?
Attachment #8490518 - Flags: approval-mozilla-aurora+
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
Attachment #8490518 - Flags: approval-mozilla-b2g32?
Comment on attachment 8490518 [details] [diff] [review]
1067858_fix_HTMLMediaElement_CanPlayChanged.patch

Approving the uplift, given the user impact..
Attachment #8490518 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.