Closed Bug 1316758 Opened 8 years ago Closed 8 years ago

Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: cpearce, Assigned: kikuo)

References

Details

Attachments

(2 files)

Bug 1316758-Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
[FF52] Bug 1316758 - Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement
2.83 KB, patch
cpearce
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
This is a follow up from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1277813#c23

I'm pretty sure the calls to nsContentUtils::*IsCallerChrome* are leftovers from the days when the video controls had chrome privileges so that they can start video playback when autoplay is disabled on Android. The video controls no longer have chrome privileges, and given that we have a call in HTMLMediaElement::DoLoad() which gets called from the DOM parser, this means <video src=url> will pass the test and be blessed to playback even when autoplay is disabled.

So we need to remove the nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement and test that setting media.autoplay.enabled=false only plays back when the user interacts with the video.
Note: we especially need to ensure that playback on android works as expected when autoplay is disabled on Android.
Blake: Is this someone on your team can look at? The patches would need to be tested on Fennec.
Flags: needinfo?(bwu)
See Also: → 1316271
Kilik, 
Can you help on this?
Flags: needinfo?(bwu) → needinfo?(kikuo)
Change P3 to P1 since this affects bug 1317167 and bug 1316271.
Priority: P3 → P1
Assignee: nobody → kikuo
Flags: needinfo?(kikuo)
Comment on attachment 8812040 [details]
Bug 1316758-Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement.

https://reviewboard.mozilla.org/r/93950/#review94088
Comment on attachment 8812040 [details]
Bug 1316758-Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement.

https://reviewboard.mozilla.org/r/93950/#review94090

I've removed these calls from HTMLMediaElement and did some tests on desktop and fennec. The behaviors (w/ or w/o the patch) are consistent when "media.autoplay.enabled=false".

[Desktop browser]
On YouTube, consistent.
On VIMEO, consistent. 
  - NOTE : VIMEO's video won't start playback automatically even "media.autoplay.enabled = true".  (Maybe that's an issue.)

[Fennec]
On Youtube, consistent.
 - NOTE : W/O the patch, video starts playback automatically when "media.autoplay.enabled = false" ... (This should be a regression, because it behaves correcly on released Fennec(49), will file a bug for that)
(In reply to Kilik Kuo [:kikuo] from comment #8)
> Comment on attachment 8812040 [details]
> Bug 1316758-Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and
> IsCallerChrome calls from HTMLMediaElement.
> 
> https://reviewboard.mozilla.org/r/93950/#review94090
> 
> I've removed these calls from HTMLMediaElement and did some tests on desktop
> and fennec. The behaviors (w/ or w/o the patch) are consistent when
> "media.autoplay.enabled=false".
> 
> [Desktop browser]
> On YouTube, consistent.
> On VIMEO, consistent. 
>   - NOTE : VIMEO's video won't start playback automatically even
> "media.autoplay.enabled = true".  (Maybe that's an issue.)

I see this behaviour in Chrome, so I expect it's intended behaviour as designed by Vimeo.

> 
> [Fennec]
> On Youtube, consistent.
>  - NOTE : W/O the patch, video starts playback automatically when
> "media.autoplay.enabled = false" ... (This should be a regression, because
> it behaves correcly on released Fennec(49), will file a bug for that)

On Fennec YouTube is supposed to work when autoplay is disabled if the user clicks through from the main YouTube.com page; when the user clicks on YouTube's video speed-dial to select a video to play, the click handler creates a video element and loads an empty src="" in it in order to bless the video so that it can play later outside of a user-generated event handler. We don't want to break YouTube here, as we interpret the user clicking on the YouTube page as intent to play.

So please can you ensure that if YouTube is calling HTMLMediaElement.load() in a user generated event handler, that the video element is blessed to ensure the video can be played later, even if when autoplay is disabled?

If YouTube are not calling load() inside a user generated event handler, please let me know and I'll reach out to YouTube.
Flags: needinfo?(kikuo)
Comment on attachment 8812040 [details]
Bug 1316758-Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement.

https://reviewboard.mozilla.org/r/93952/#review94448
Attachment #8812040 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #9)
> So please can you ensure that if YouTube is calling HTMLMediaElement.load()
> in a user generated event handler, that the video element is blessed to
> ensure the video can be played later, even if when autoplay is disabled?
> 
> If YouTube are not calling load() inside a user generated event handler,
> please let me know and I'll reach out to YouTube.

As Chris said, HTMLMediaElement.load() is called in a user generated event handler ("click").
So should be no problem.

...
11-22 14:33:09.743 30049 30072 I Gecko   : [Main Thread]: D/nsMediaElement 8c280000 SuspendOrResumeElement(pause=0, suspendEvents=0) hidden=0
11-22 14:33:09.745 30049 30072 I Gecko   : Call to virtual nsresult mozilla::dom::HTMLMediaElement::Load(). The JS stack is:
11-22 14:33:09.745 30049 30072 I Gecko   : 0 yi(a = [object Object], b = [object HTMLVideoElement]) ["https://s.ytimg.com/yts/jsbin/mobile-blazer-nirvan
a-phone-vfl4SxMvo/core.js":136]
11-22 14:33:09.745 30049 30072 I Gecko   :     this = [object Window]
11-22 14:33:09.745 30049 30072 I Gecko   : 1 zi(a = [object Object]) ["https://s.ytimg.com/yts/jsbin/mobile-blazer-nirvana-phone-vfl4SxMvo/core.js":136]
11-22 14:33:09.745 30049 30072 I Gecko   :     this = [object Window]
11-22 14:33:09.745 30049 30072 I Gecko   : 2 g.h.FA(a = 4) ["https://s.ytimg.com/yts/jsbin/mobile-blazer-nirvana-phone-vfl4SxMvo/core.js":269]
11-22 14:33:09.745 30049 30072 I Gecko   :     this = [object Object]
11-22 14:33:09.745 30049 30072 I Gecko   : 3 g.h.hK() ["https://s.ytimg.com/yts/jsbin/mobile-blazer-nirvana-phone-vfl4SxMvo/core.js":270]
11-22 14:33:09.745 30049 30072 I Gecko   :     this = [object Object]
11-22 14:33:09.745 30049 30072 I Gecko   : 4 tca([object MouseEvent], [object HTMLBodyElement], "click", "") ["https://s.ytimg.com/yts/jsbin/mobile-blaz
er-nirvana-phone-vfl4SxMvo/core.js":217]
11-22 14:33:09.745 30049 30072 I Gecko   :     this = [object HTMLBodyElement]
11-22 14:33:09.745 30049 30072 I Gecko   : 5 eh/<([object MouseEvent]) ["https://s.ytimg.com/yts/jsbin/mobile-blazer-nirvana-phone-vfl4SxMvo/core.js":11
5]
11-22 14:33:09.745 30049 30072 I Gecko   :     this = [object HTMLBodyElement]
11-22 14:33:09.745 30049 30072 I Gecko   : 
11-22 14:33:09.745 30049 30072 I Gecko   : [Main Thread]: D/nsMediaElement 8c280000 Load() hasSrcAttrStream=0 hasSrcAttr=0 hasSourceChildren=0 handlingI
nput=1 isCallerChromeOrNative=0
11-22 14:33:09.745 30049 30072 I Gecko   :  >>>>>>>>>>>>>>>>>>>>>>>>> DoLoad (set mHasUserInteraction(0) -> True)
...

and the src attribute is set with actual blob uri later.
Flags: needinfo?(kikuo)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6b94b6c5fb0
Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement. r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6b94b6c5fb0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Is the AutoNoJSAPI in HTMLMediaElement::NotifyAudioChannelAgent still needed?
Flags: needinfo?(kikuo)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15)
> Is the AutoNoJSAPI in HTMLMediaElement::NotifyAudioChannelAgent still needed?

Good catch, thanks !
Thought I don't quite get into how AutoNoJSAPI performs that magic, but this could be removed too. Bug 1319686 will fix it.
Flags: needinfo?(kikuo)
> Thought I don't quite get into how AutoNoJSAPI performs that magic

It explicitly makes it look like there is no active JSContext, so LegacyIscallerChromeOrNativeCode thinks caller is native code.
Hi, Kilik,
Could you uplift this bug to FF52?
Because we can still see the crash in FF52. (see bug1319729)
Thanks!
Flags: needinfo?(kikuo)
Hi Chris, this issue should be uplifted to FF52. Here's the patch for 52. Could you review it ?
The only difference is that in this patch for 52, I also removed the use of AutoNOJSAPI which was done in Bug 1319686.

Thanks !
Flags: needinfo?(kikuo)
Attachment #8815174 - Flags: review?(cpearce)
Attachment #8815174 - Flags: approval-mozilla-aurora?
Attachment #8815174 - Flags: approval-mozilla-aurora?
Comment on attachment 8815174 [details] [diff] [review]
[FF52] Bug 1316758 - Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1319729
[User impact if declined]: Browser crashes on certain sites when "media.autoplay.enabled = false"
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, it's on nightly now.
[Needs manual test from QE? If yes, steps to reproduce]: Yes,

Desktop
1. set media.autoplay.enabled = false
2. go to youtube website and click one video to play.
3. video won't start playback automatically
4. click "play" on video control, video starts playback.
5. click other video, video starts playback automatically (user interacted)

Fennec
1. set media.autoplay.enabled = false
2. go to youtube website and click one video to play.
3. video should start playback automatically

[List of other uplifts needed for the feature/fix]:N/A
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Those calls are leftovers, see Comment 0
[String changes made/needed]: None
Attachment #8815174 - Flags: approval-mozilla-aurora?
Blocks: 1319729
Comment on attachment 8815174 [details] [diff] [review]
[FF52] Bug 1316758 - Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement

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

Great, thanks Kilik.
Attachment #8815174 - Flags: review?(cpearce) → review+
Comment on attachment 8815174 [details] [diff] [review]
[FF52] Bug 1316758 - Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement

fix content crash on aurora52 with media.autoplay.enabled=false
Attachment #8815174 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flag 'checkin-needed' again for uplifting Attachment 8815174 [details] [diff] to FF52(Aurora).
status-firefox52: --- → affected
Keywords: checkin-needed
Depends on: 1348601
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: