786650 - Broken "Listen" buttons in Google Translate

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Vladimir_S]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120828042007

Steps to reproduce:

1. Open Google Tranlate site (http://translate.google.com/)
2. Type any phrase or word in left fild; receive the translation in right one;
3. Push the audio button in any of two fields.


Actual results:

Nothing except the winking of right field frame.


Expected results:

The inserted text or its translation pronounced.

Comment 1

[Vladimir_S]

System: 
Windows XP Sp3 Rus Home Edition (but according to the discussion on Russian Mozilla support forum, the OS, at least Windows, doesn't matter).

Shockwave Flash:
NPSWF32_11_4_402_265.dll
11.4.402.265
Shockwave Flash 11.4 r402

Results:
In Firefox 16.0a2 audio works only in the Plugin Container allowed mode.
In Firefox 17.0a2 and Firefox 18.0a1 it refuses to work independently on Plugin Container mode. 

Running the browser in Safe Mode does not help: nothing changes. So it is not the extensions or style problem.
In IE8 this function works fine.

Comment 2

[Benjamin Smedberg]

I can confirm that the site is broken. This is probably a regression from bug 745030, but can somebody check the regression range to be sure? That would have a regression range on mozilla-central between the 2012-08-07 and 2012-08-08 nightlies.

Comment 3

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b4a63a0b90c2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120806153305
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/89ea9764f9e9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120806140630
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b4a63a0b90c2&tochange=89ea9764f9e9


In local build
Last good: b4a63a0b90c2
First bad: f3bd764deb31

rigggered by: Bug 745030

Comment 4

[John Schoenick [:johns]]

Investigating

Comment 5

[John Schoenick [:johns]]

Attachment: Abort loading plugins without a frame, properly re-instantiate channel-loaded plugins

Comment 6

[John Schoenick [:johns]]

Comment on attachment 656688 [details] [diff] [review]
Abort loading plugins without a frame, properly re-instantiate channel-loaded plugins

The plugin on this page is initially setup as display:none. We open a channel, get to spawn the plugin, fail because we lack a frame, and revert to fallback on account of failed-to-spawn-plugin.

Previous to Bug 745030 we would ignore errors from NewEmbeddedPluginStreamListener, and things just-happened to work because we HasNewFrame would trigger the instantiation later, (after the channel had disappeared...)

This patch detects a missing frame and leaves the tag loaded as a plugin, then fixes bug 767627 so that we actually re-open the channel later when we have a frame.

This solution stinks, though, as we open channels for display:none plugins and then close them - only to re-open them later when shown (this is what happened before, except we left it up to the plugin to look at its data param and open its own channel...).

Aside from not kicking off the load of object tags with channels until they're displayed, I'm not sure how we want to fix this - or do we just want to keep this somewhat-silly behavior?

Comment 7

[Benjamin Smedberg]

Do we know why the tests from bug 601064 didn't catch this (we broke this once before!)? Can we make sure that those tests include this failure mode?

Comment 8

[John Schoenick [:johns]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Do we know why the tests from bug 601064 didn't catch this (we broke this
> once before!)? Can we make sure that those tests include this failure mode?

Bug 601064 looks to be a different cause - frame management and painting vs instantiating. I opened bug 783059 to create some tests specifically aimed at testing various instantiation scenarios, as that is where we're sorely lacking.

Comment 9

[Josh Aas]

Comment on attachment 656688 [details] [diff] [review]
Abort loading plugins without a frame, properly re-instantiate channel-loaded plugins

Review of attachment 656688 [details] [diff] [review]:
-----------------------------------------------------------------

Since you're bailing earlier in instantiation when we have no frame, lets assert at the later check for a frame since we must have one by then. Make it clear that it isn't a case that should happen.

I think we should take a patch like this and then look into the possibility of never loading a stream for display:none.

Great work, thanks for figuring this out!

Comment 10

[John Schoenick [:johns]]

Attachment: Abort instantiating plugins without a frame early such that HasNewFrame can continue the load when appropriate

Adds assertion, splits out part for re-instantiating stream-loading plugins to bug 767627

Comment 11

[John Schoenick [:johns]]

Attachment: Part 2, don't bail out for no frame until we've flushed layout

So I broke the async instantiation path:
https://tbpl.mozilla.org/?tree=Try&rev=f8c03c95595a

Because we bail out for no frame, but can reach InstantiatePluginInstance() in async instantiation with incomplete layout. The instantiation still works fine, but because nsDOMClassInfo.cpp can't speed up the async instantiation things like |obj.data = blah.swf; obj.callIntoFlash();| fail.

The solution is to just bail out after we do the Flush_Layout call that is already there.

Comment 12

[John Schoenick [:johns]]

Attachment: Abort load of channel-having plugins without frames at the appropriate point. r=josh

Folded together and rebased

Comment 13

[John Schoenick [:johns]]

Comment on attachment 658601 [details] [diff] [review]
Abort load of channel-having plugins without frames at the appropriate point. r=josh

https://hg.mozilla.org/integration/mozilla-inbound/rev/827807082790

try:
https://tbpl.mozilla.org/?tree=Try&rev=7e7d0ac01c7f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/827807082790

Comment 16

[John Schoenick [:johns]]

Comment on attachment 658601 [details] [diff] [review]
Abort load of channel-having plugins without frames at the appropriate point. r=josh

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 745030

User impact if declined: 
Some plugin-using sites (e.g. Google Translate) will not load

Testing completed (on m-c, etc.):
On m-c

Risk to taking this patch (and alternatives if risky): 
Low, fixes specific failure case

String or UUID changes made by this patch:
None

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 658601 [details] [diff] [review]
Abort load of channel-having plugins without frames at the appropriate point. r=josh

[Triage Comment]
Low risk FF17 web regression fix, approved for Aurora.

Comment 18

[Philip Chee]

> Low risk FF17 web regression fix, approved for Aurora.

This may have caused Bug 789033 - Crash with Flashblock [@ nsIView::GetViewFor ][@ nsViewManager::InvalidateWidgetArea(nsView*, nsRegion const&) ]

Comment 19

[John Schoenick [:johns]]

This caused 789033 by re-ordering the re-entry lock to above the layout flush, which flashblock reenters upon. Going to wait a few days to make sure that's okay and nom it for aurora to land with this.

Comment 20

[John Schoenick [:johns]]

789033's fix caused more issues on try, so I'm going to back this out for the time being

https://hg.mozilla.org/mozilla-central/rev/36427d4b2cf6

Comment 21

[John Schoenick [:johns]]

Attachment: Abort load of channel-having plugins without frames at the appropriate point (second attempt)

Without breaking the mInstantiating lock this time

https://tbpl.mozilla.org/?tree=Try&rev=ed277fa9e10c

Comment 22

[John Schoenick [:johns]]

Comment on attachment 659347 [details] [diff] [review]
Abort load of channel-having plugins without frames at the appropriate point (second attempt)

Looks good on try, doesn't cause bug 789033 this time.

Comment 23

[John Schoenick [:johns]]

Comment on attachment 659347 [details] [diff] [review]
Abort load of channel-having plugins without frames at the appropriate point (second attempt)

https://hg.mozilla.org/integration/mozilla-inbound/rev/69bdd87c6c91

try
https://tbpl.mozilla.org/?tree=Try&rev=ed277fa9e10c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69bdd87c6c91

Comment 26

[Alex Keybl [:akeybl]]

Please nominate for Aurora approval when comfortable. Thanks!

Comment 27

[John Schoenick [:johns]]

Comment on attachment 659347 [details] [diff] [review]
Abort load of channel-having plugins without frames at the appropriate point (second attempt)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 745030

User impact if declined:
Broken plugin content on some sites

Testing completed (on m-c, etc.):
on m-c

Risk to taking this patch (and alternatives if risky): 
Low, fixes specific failure case

String or UUID changes made by this patch:
None

Comment 28

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f22cb258b111

Comment 29

[Scoobidiver (away)]

The target milestone is always m-c.

Comment 30

[Vladimir_S]

From bugreporter: now, after the today's update of Aurora, it really works. Thank you!

Comment 31

[Mihai Morar, (:MihaiMorar)]

Verified for FF 18.b2 WFM on Windows 7 x64, Ubuntu x32 and Mac OS 10.7.

Windows 7 x64:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)

Ubuntu x32
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)

Mac OS 10.7
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)

Couldn't reproduce this issue in safe mode and new profile too. Please someone with permission set flag for FF18 as Verified.

Comment 32

[Georg Fritzsche [:gfritzsche]]

Verified fixed for FF18 as per comment 31.

Comment 33

[John Schoenick [:johns]]

Bug 783059 takes care of the missing test coverage here