Last Comment Bug 723379 - audio starts playing when it didn't before
: audio starts playing when it didn't before
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla13
Assigned To: Josh Aas
:
Mentors:
http://news.google.com
: 723116 723650 (view as bug list)
Depends on: 725855 727427 728125 728576 729546
Blocks: 90268
  Show dependency treegraph
 
Reported: 2012-02-01 18:45 PST by Timothy Nikkel (:tnikkel)
Modified: 2012-02-22 10:37 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (13.56 KB, patch)
2012-02-05 08:10 PST, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.1 (13.30 KB, patch)
2012-02-06 19:10 PST, Josh Aas
no flags Details | Diff | Splinter Review
fix v2.0 (1.93 KB, patch)
2012-02-07 10:52 PST, Josh Aas
bzbarsky: review+
Details | Diff | Splinter Review

Description Timothy Nikkel (:tnikkel) 2012-02-01 18:45:40 PST
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) 2012-02-01 19:02:01 PST
It does happen on Mac though.
Comment 2 Marcia Knous [:marcia - use ni] 2012-02-01 19:07:36 PST
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:] 2012-02-01 19:14:07 PST
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 4 John P Baker 2012-02-02 05:08:47 PST
*** Bug 723116 has been marked as a duplicate of this bug. ***
Comment 5 Michael 2012-02-02 10:24:15 PST
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 6 Matthias Versen [:Matti] 2012-02-02 12:00:46 PST
*** Bug 723650 has been marked as a duplicate of this bug. ***
Comment 7 Josh Aas 2012-02-05 07:32:23 PST
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 2012-02-05 08:10:45 PST
Created attachment 594549 [details] [diff] [review]
fix v1.0

Can and should write a test for this, but I want to start review ASAP.
Comment 9 Josh Aas 2012-02-05 10:25:53 PST
Fix v1.0 makes the "test_pluginstream_src_dynamic.html" test fail.
Comment 10 Josh Aas 2012-02-05 11:24:23 PST
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 [:bsmedberg] 2012-02-06 07:57:25 PST
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 [:bsmedberg] 2012-02-06 14:38:14 PST
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 2012-02-06 15:15:43 PST
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 [:bz] 2012-02-06 15:38:19 PST
> 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 [:bz] 2012-02-06 15:38:37 PST
Hmm.  Why was this patch causing the test to fail?
Comment 16 Josh Aas 2012-02-06 15:55:25 PST
(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 2012-02-06 19:10:02 PST
Created attachment 594884 [details] [diff] [review]
fix v1.1
Comment 18 Boris Zbarsky [:bz] 2012-02-06 19:13:42 PST
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 [:bz] 2012-02-06 19:24:40 PST
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 [:bsmedberg] 2012-02-07 06:08:24 PST
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 2012-02-07 10:52:35 PST
Created attachment 595096 [details] [diff] [review]
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 [:bz] 2012-02-07 11:38:02 PST
Comment on attachment 595096 [details] [diff] [review]
fix v2.0

r=me
Comment 23 Josh Aas 2012-02-07 14:26:19 PST
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/4e6f683d3006
Comment 24 Ed Morley [:emorley] 2012-02-08 09:02:40 PST
https://hg.mozilla.org/mozilla-central/rev/4e6f683d3006
Comment 25 Jim Jeffery not reading bug-mail 1/2/11 2012-02-17 03:34:14 PST
Looks like this should be fixed or backed out since bugs are piling up against it.

Note You need to log in before you can comment on or make changes to this bug.