723379 - audio starts playing when it didn't before

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Timothy Nikkel (:tnikkel)]

Goto http://news.google.com click on the summary text (not the title) of a story. Audio relating to that story starts playing.

No audio played in 2012-01-31 nightly. 2012-02-01 nightly has the problem. So I'm going out on a limb and guessing bug 90268. (actual changesets in that range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f26b7bee352&tochange=e18c7bc2c28e )

Doesn't seem to happen on Windows.

Comment 1

[Timothy Nikkel (:tnikkel)]

It does happen on Mac though.

Comment 2

[Marcia Knous [:marcia]]

Adding bc to the bug since he mentioned this to me earlier today on IRC.  I wasn't able to reproduce but I think it is because I wasn't clicking on the summary text.

I can reproduce on Mac 10.6.

Comment 3

[Bob Clary [:bc] (inactive)]

I don't think I need to do anything like clicking on the summary to get the embedded video's playing. They all seem to play at once.  I'm on Mac OS X 10.5. Looking at the source, the embedded youtube videos all have autoplay=1 but the embed is not displayed by default... it is hidden by the thumbnail which appears to prevent the video from beginning to play. At least that is my guess.

Comment 5

[Michael]

I get it on 10.7, starting with the 2-1-2012 build.  I don't have to click on anything.  I just go to news.google.com and it starts playing audio from every video on the page simultaneously.

Comment 7

[Josh Aas]

I think I have a fix for this. I think we need to allow instances to be created by script calls into an instance DOM node even when there isn't an object frame, while not creating instances for nodes without object frames otherwise. Will post a patch soon.

Comment 8

[Josh Aas]

Attachment: fix v1.0

Can and should write a test for this, but I want to start review ASAP.

Comment 9

[Josh Aas]

Fix v1.0 makes the "test_pluginstream_src_dynamic.html" test fail.

Comment 10

[Josh Aas]

The problem with "test_pluginstream_src_dynamic.html" is that we never create an object frame for the DOM node after attaching it to the document. This is almost certainly a problem with the test, but if it isn't then Gecko is incorrectly not making a frame.

Comment 11

[Benjamin Smedberg]

It looks to me like the plugin in test_pluginstream_src_dynamic should be completely visible. In what way does the test fail? I'm a little worried about the heuristics here, and this sounds like something we ought to coordinate pretty carefully with the HTML5 spec text so that everyone ends up with the same behavior.

Comment 12

[Benjamin Smedberg]

Comment on attachment 594549 [details] [diff] [review]
fix v1.0

This patch makes me really nervous because I don't understand what the difference between this and the previous (before bug 90286) behavior is, or why this behavior is at all correct. This seems like magic behavior. Punting to bz for feedback.

Comment 13

[Josh Aas]

Comment on attachment 594549 [details] [diff] [review]
fix v1.0

Canceling review on this patch, I have a newer one coming up that more accurately matches our older behavior and doesn't cause test_pluginstream_src_dynamic to fail.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I don't understand what the difference between this and the previous (before bug 90286)
> behavior is

The patch is restoring an aspect of the pre-bug-90286 behavior.  We used to only allow instantiation when we had a frame.  Now we allow it without a frame.  This patch changes to allow it without a frame only in cases when we're sync-instantiating in response to a script trying to make JS calls on the plug-in object.

I agree it's weird and magic; the whole script access to plugins thing is...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why was this patch causing the test to fail?

Comment 16

[Josh Aas]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Hmm.  Why was this patch causing the test to fail?

Because I think we also have to allow instantiation without a frame in one more case related to streams. New patch will show this.

Comment 17

[Josh Aas]

Attachment: fix v1.1

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594884 [details] [diff] [review]
fix v1.1

If test_pluginstream_src_dynamic is relying on frameless instantiation, how was it passing before plugins moved to content?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594884 [details] [diff] [review]
fix v1.1

r=me per irc discussion (short story is that we used to be able to create a plugin instance owner without an instance and without an nsObjectFrame, and then sometimes actually instantiate off that, still without a frame).

Comment 20

[Benjamin Smedberg]

I still don't understand why there wouldn't be a frame: the plugin should be visible in this test, and we're not scripting it immediately after we create it...

Comment 21

[Josh Aas]

Attachment: fix v2.0

Turns out we have a bug messing with whether or not we create object frames for instances with associated src/data streams. That was throwing off my analysis of instantiation requirements.

This patch fixes that bug and allows us to enforce the object frame requirement more broadly. All tests pass locally (running on try server now) and top sites like YouTube, Pandora, and Hulu work on Mac OS X.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 595096 [details] [diff] [review]
fix v2.0

r=me

Comment 23

[Josh Aas]

pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/4e6f683d3006

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4e6f683d3006

Comment 25

[Jim Jeffery not reading bug-mail 1/2/11]

Looks like this should be fixed or backed out since bugs are piling up against it.