Closed
Bug 1262184
Opened 8 years ago
Closed 8 years ago
Audio for embedded videos using browser player autoplays.
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: joshuaglennrussell, Assigned: qdot)
References
Details
(Keywords: regression, Whiteboard: btpp-active)
Attachments
(3 files, 5 obsolete files)
827 bytes,
text/html
|
Details | |
3.41 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.55 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0 Build ID: 20160405030214 Steps to reproduce: For example embedded MP4s using the browser player. Only the audio. Search "mp4" here for example. (Noisy) http://support.proboards.com/search Actual results: Audio of embedded videos that use the browser's player autoplays. Expected results: Audio of the videos shouldn't autoplay.
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Comment 1•8 years ago
|
||
I can reproduce the problem on Windoows7 and Eindows10IP. But I cannot reproduce on IE11, Chrome and Edge.
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Audio/Video
Ever confirmed: true
Product: Firefox → Core
Comment 2•8 years ago
|
||
[Tracking Requested - why for this release]: Regressed since 45 Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d353d43108373a8461ce304a15c23084c8eafb6d&tochange=bc2fc3ae3bd25f619102e8e2ce1cae909280fe2d Regressed by: bc2fc3ae3bd2 Kyle Machulis — Bug 1237963 - Add documents to embed element capabilities; r=bz
Blocks: 1237963
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
tracking-firefox-esr45:
--- → ?
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
Keywords: regression
Version: 48 Branch → 45 Branch
Comment 3•8 years ago
|
||
Hmm. At a guess the <object> is irrelevant there, right? The only relevant parts are the <video> and the <embed>? And at a further guess, we end up playing the <embed>, right? I just checked, and this document: <!DOCTYPE html> <embed src="http://dev.prbrds.com/d/c/Jxsic9J5IO.mp4" qtsrc="http://dev.prbrds.com/d/c/Jxsic9J5IO.mp4" scale="aspect" autoplay="false" loop="false" controller="true" pluginspage="http://www.apple.com/quicktime/" width="640"> does play the video in Chrome, as expected. Looking at https://html.spec.whatwg.org/multipage/embedded-content.html#the-embed-element it looks like the definition of "potentially active" includes "The element is not a descendant of a media element." (where "media element" means <audio> or <video>) just like it includes "The element is not a descendant of an object element that is not showing its fallback content". So per spec it seems like we should never play (or, really, load) an <embed> that's a descendant of <video>, or a descendant of <object> that's playing itself, for that matter. I don't think we have any logic to that effect right now... and adding it in beta sounds pretty scary.
Comment 4•8 years ago
|
||
A simple testcase illustrating the issue with just <object>: <object data="http://example.com" type="text/html"> <embed src="http://dev.prbrds.com/d/c/Jxsic9J5IO.mp4" qtsrc="http://dev.prbrds.com/d/c/Jxsic9J5IO.mp4" scale="aspect" autoplay="false" loop="false" controller="true" pluginspage="http://www.apple.com/quicktime/" width="640"> </object> This doesn't play audio in Chrome, but does in Firefox. And I expect if you picked a type handled by plug-in it might play it there too, except for the autoplay="false" thing which some plug-ins might care about.
Updated•8 years ago
|
Component: Audio/Video → DOM
Assignee | ||
Comment 5•8 years ago
|
||
Ok, gonna try for 2 solutions here, since we're close enough to the 46 release that it warrants a less risky fix than what we'd like on trunk. For branches, we'll not allow embeds to load as documents when they're the descendant of a media tag or object that's already got content to play itself. This means we'll still at least load the the embed as we always have. For trunk, we'll not run nsObjectLoadingContent::LoadObject for any embed that's the descendant of a media tag or an object that's already got content to play itself, which is what the spec says (see Comment 3).
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 6•8 years ago
|
||
Here's a first swipe at the patch for trunk. For the branches, I can just move the check down to where we change the MIME type. The way I'm traversing up the tree seems a little messy, so if there's a better way to do that, let me know.
Attachment #8739278 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•8 years ago
|
||
Mochitests coming, just figured I'd at least get the base patch in for review now.
Comment 8•8 years ago
|
||
Comment on attachment 8739278 [details] [diff] [review] Patch 1 (v1) - Block embed content loading in certain cases It might make more sense to do this inside HTMLSharedObjectElement, as in skip calling LoadObject() altogether. But maybe it's simpler here, if we call LoadObject in multiple places there. >+ if (!BlockEmbedContentLoading()) { Isn't this test backwards? As in, shouldn't have the '!' there, unless I'm missing something. We need tests... >+ nsCOMPtr<nsIContent> thisContent = >+ do_QueryInterface(static_cast<nsIImageLoadingContent*>(this)); Could just pass that from the one caller. >+ if (!thisContent->NodeInfo()->Equals(nsGkAtoms::embed)) { if (!thisContent->IsHTMLElement(nsGkAtoms::embed)) { is probably a bit more idiomatic and doesn't make quite so many assumptions about what sorts of elements can come through here (though I do think those assumptions hold in this case, but still). >+ nsIDOMNode* node = thisContent->AsDOMNode(); Yeah, you don't want that. In general, you don't want any nsIDOM*; we're trying to remove them. ;) >+ node->GetParentNode(getter_AddRefs(parent)); nsIContent* parent = thisContent->GetParent(); >+ domObject = do_QueryInterface(parent); >+ if (domObject) { if (parent->IsHTMLElement(nsGkAtoms::object)) { The other thing you could do is add this to HTMLObjectElement.h (inside the class decl): NS_IMPL_FROMCONTENT_HTML_WITH_TAG(HTMLObjectElement, object) and then do: if (HTMLObjectElement* object = HTMLObjectElement::FromContent(parent)) { uint32_t type = object->DisplayedType(); } and hence avoid the QI and XPCOM getter bits. >+ if (type != eType_Null) { >+ return true; So there's one complication here. If that <object> _becomes_ eType_Null in the future (e.g. the loading on it finishes and it decides to fall back), we need to go ahead and load the embed at that point. Otherwise things will definitely be broken, I expect. Given that, it's probably worth it to just do the media element thing on branches (and possibly trunk), since that fixes this particular bug and is much simpler, then do a second bug for the <object> thing. >+ domMedia = do_QueryInterface(parent); Or you could do: if (parent->IsAnyOfHTMLElements(nsGkAtoms::video, nsGkAtoms::audio)) { } or even add an nsIContent::IsMediaElement that's implemented that way.
Attachment #8739278 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 9•8 years ago
|
||
Vastly simplified the patch, and moved everything down to HTMLSharedObjectElement, which seems to work fine for the couple of manual test cases I have. Still working on tests, and am breaking object descendant changes out into another bug.
Attachment #8739278 -
Attachment is obsolete: true
Attachment #8739614 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•8 years ago
|
||
And now, I actually upload the correct patch.
Attachment #8739614 -
Attachment is obsolete: true
Attachment #8739614 -
Flags: review?(bzbarsky)
Attachment #8739615 -
Flags: review?(bzbarsky)
Comment 11•8 years ago
|
||
Comment on attachment 8739615 [details] [diff] [review] Patch 1 (v3) - Block embed content loading when child of media element >+ nsCOMPtr<nsIContent> thisContent = >+ do_QueryInterface(static_cast<nsIImageLoadingContent*>(this)); No need. HTMLSharedObjectElement inherits from nsIContent. >+ nsIContent* parent = thisContent->GetParent(); So this can just be: nsIContent* parent = GetParent(); In fact, I'd probably write this like so: for (nsIContent* parent = GetParent(); parent; parent = parent->GetParent()) { if (arent->IsAnyOfHTMLElements(nsGkAtoms::video, nsGkAtoms::audio)) { return true; } } return false; But also, you need to check somewhere in here that IsHTMLElement(nsGkAtoms::embed), right? Because it's not clear to me that we want to change the behavior for <applet>. >+ * If this nsObjectLoadingContent is a superclass of an embed node This comment needs adjusting; more like: If this HTMLSharedObjectElement is an <embed>. >+ * - If the embed node is the child of an object node that already has >+ * content being loaded. We're not implementing that yet. >+ * In these cases, this function will return false, which will cause >+ * LoadObject to exit early. Actually, we're just not calling LoadObject at all. r=me with the above fixed.
Attachment #8739615 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Whiteboard: btpp-active
Updated•8 years ago
|
Flags: needinfo?(kyle)
Assignee | ||
Comment 12•8 years ago
|
||
I'm working on tests, this should hopefully land quickly after that.
Flags: needinfo?(kyle)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8739615 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8740167 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 15•8 years ago
|
||
Fixed extra include that we don't need yet.
Attachment #8740166 -
Attachment is obsolete: true
Comment 16•8 years ago
|
||
Comment on attachment 8740167 [details] [diff] [review] Patch 2 (v1) - Mochitest for embed load blocking with media node ancestor This is ok as far as it goes, but is there a reason this is not a web platform test? And why do you need the <object> bits in there? I don't think you do...
Attachment #8740167 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•8 years ago
|
||
Ok, removed object tags, changed it to a wpt.
Attachment #8740167 -
Attachment is obsolete: true
Attachment #8740682 -
Flags: review?(bzbarsky)
Comment 18•8 years ago
|
||
Comment on attachment 8740682 [details] [diff] [review] Patch 2 (v2) - Web Platform Test for embed content ignoring in media nodes You need to hg add embed_iframe.html. And probably also have a test that asserts that having such an <embed> _outside_ a media element does load the iframe in it, if there is no such thing already. r=me with that.
Attachment #8740682 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 19•8 years ago
|
||
embed_iframe.html was added as part of bug 1239586, alongside a test that uses it in an embed. Just reusing it here 'cause it does what I need.
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f95bb4a2a46d https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcbec37ca27
Comment 21•8 years ago
|
||
It would be nice to fix this in 46 and avoid shipping this again - please let me know if you think you have something safe for last minute beta uplift.
Comment 22•8 years ago
|
||
I believe the patch we checked in for this bug is safe for uplift to beta. Safer than shipping the existing breakage for another release.
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8740169 [details] [diff] [review] Patch 1 (v5) - Block embed content loading when child of media element; r=bz Approval Request Comment [Feature/regressing bug #]: Bug 1237963 [User impact if declined]: Some hidden embed elements may play video/audio [Describe test coverage new/current, TreeHerder]: Web platform tests [Risks and why]: None known [String/UUID change made/needed]: None
Attachment #8740169 -
Flags: approval-mozilla-beta?
Attachment #8740169 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 24•8 years ago
|
||
I'll just bring the tests in with A=TEST-ONLY if patch 1 is approved.
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f95bb4a2a46d https://hg.mozilla.org/mozilla-central/rev/6fcbec37ca27
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 26•8 years ago
|
||
Comment on attachment 8740169 [details] [diff] [review] Patch 1 (v5) - Block embed content loading when child of media element; r=bz Fix for some autoplay issues, please uplift.
Attachment #8740169 -
Flags: approval-mozilla-beta?
Attachment #8740169 -
Flags: approval-mozilla-beta+
Attachment #8740169 -
Flags: approval-mozilla-aurora?
Attachment #8740169 -
Flags: approval-mozilla-aurora+
Comment 27•8 years ago
|
||
Kyle: can you land the tests today on aurora and beta and keep an eye on this?
Flags: needinfo?(kyle)
Comment 28•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/51ec91ed78a5
Comment 29•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4c2bab43bcdf
Assignee | ||
Comment 30•8 years ago
|
||
Tests fixed (had to fix up w-p-t manifests, then had to backout and reland due to comma issues) and pushed to aurora and beta. Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/4633816cde73beee6ed194e571c3a2ba14b15069 Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/ec296bd7ad8978395010bd36061ff6037bcabe4f
Flags: needinfo?(kyle)
Comment 31•8 years ago
|
||
Not sure it is a big deal, not taking the fix in esr45
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•