audio starts playing when it didn't before

RESOLVED FIXED in mozilla13

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: tnikkel, Assigned: Josh Aas)

Tracking

Trunk
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
It does happen on Mac though.
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

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → joshmoz

Updated

6 years ago
Duplicate of this bug: 723116

Comment 5

6 years ago
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.
Duplicate of this bug: 723650
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Created attachment 594549 [details] [diff] [review]
fix v1.0

Can and should write a test for this, but I want to start review ASAP.
Attachment #594549 - Flags: review?(benjamin)
(Assignee)

Comment 9

6 years ago
Fix v1.0 makes the "test_pluginstream_src_dynamic.html" test fail.
(Assignee)

Comment 10

6 years ago
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.
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 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.
Attachment #594549 - Flags: review?(benjamin) → review?(bzbarsky)
(Assignee)

Comment 13

6 years ago
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.
Attachment #594549 - Flags: review?(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...
Hmm.  Why was this patch causing the test to fail?
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
Created attachment 594884 [details] [diff] [review]
fix v1.1
Attachment #594549 - Attachment is obsolete: true
Attachment #594884 - Flags: review?(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 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).
Attachment #594884 - Flags: review?(bzbarsky) → review+
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...
(Assignee)

Comment 21

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #595096 - Flags: review?(bzbarsky)
Comment on attachment 595096 [details] [diff] [review]
fix v2.0

r=me
Attachment #595096 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

6 years ago
Attachment #594884 - Attachment is obsolete: true
Attachment #594884 - Flags: review+
(Assignee)

Comment 23

6 years ago
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/4e6f683d3006
https://hg.mozilla.org/mozilla-central/rev/4e6f683d3006
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
Depends on: 725855
Depends on: 727427

Updated

5 years ago
Depends on: 728125
Looks like this should be fixed or backed out since bugs are piling up against it.

Updated

5 years ago
Depends on: 728576

Updated

5 years ago
Depends on: 729546
You need to log in before you can comment on or make changes to this bug.