Closed Bug 1262184 Opened 4 years ago Closed 4 years ago

Audio for embedded videos using browser player autoplays.

Categories

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

45 Branch
x86_64
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr38 --- unaffected
firefox-esr45 - wontfix

People

(Reporter: joshuaglennrussell, Assigned: qdot)

References

Details

(Keywords: regression, Whiteboard: btpp-active)

Attachments

(3 files, 5 obsolete files)

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.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Attached file reduced html
I can reproduce the problem on Windoows7 and Eindows10IP. 
But I cannot reproduce on IE11, Chrome and Edge.
Status: UNCONFIRMED → NEW
Component: Untriaged → Audio/Video
Ever confirmed: true
Product: Firefox → Core
[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
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
Keywords: regression
Version: 48 Branch → 45 Branch
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.
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.
Component: Audio/Video → DOM
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)
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)
Mochitests coming, just figured I'd at least get the base patch in for review now.
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-
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)
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 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+
Whiteboard: btpp-active
Flags: needinfo?(kyle)
I'm working on tests, this should hopefully land quickly after that.
Flags: needinfo?(kyle)
Fixed extra include that we don't need yet.
Attachment #8740166 - Attachment is obsolete: true
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+
Ok, removed object tags, changed it to a wpt.
Attachment #8740167 - Attachment is obsolete: true
Attachment #8740682 - Flags: review?(bzbarsky)
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+
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.
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.
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.
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?
I'll just bring the tests in with A=TEST-ONLY if patch 1 is approved.
https://hg.mozilla.org/mozilla-central/rev/f95bb4a2a46d
https://hg.mozilla.org/mozilla-central/rev/6fcbec37ca27
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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+
Kyle: can you land the tests today on aurora and beta and keep an eye on this?
Flags: needinfo?(kyle)
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)
Not sure it is a big deal, not taking the fix in esr45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.