780075 - Flash Plugin settings for "Tap to play" is ignored for the flash embedded elements

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 17.0a1 2012-08-02/ Aurora 16.0a2 2012-08-02
Device: HTC Desire Z (Android 2.3.3)/ Samsung Galaxy Tab (Android 3.1)

Steps to reproduce:
1. Make sure the plugin options are set to "Tap to play"
2. Go to http://people.mozilla.org/~mwargers/tests/flash/flashembed.html
3. Observe the embedded flash elements

Expected results:
A "Tap to play" placeholder is displayed

Actual results:
The flash elements are displayed

Notes:
This is not reproducible on Firefox Mobile 15.0b3 build 1.
Also for the URL: http://people.mozilla.org/~mwargers/tests/plugins/flash/flashembed_displaynone.html the doorhanger asking for permission to play the plugin content is not displayed while on Beta it is.

Comment 1

[:Margaret Leibovic]

This is reproducible on desktop as well. David or Jared, has some change been made to the core that might have caused this?

Comment 2

[Benjamin Smedberg]

johns, this is probably unrelated to the objectloading refactoring (because it's a 16 regression, not 17), but would you have time to look at it?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Well, looks like I'm the culprit, in a way: http://hg.mozilla.org/releases/mozilla-aurora/diff/18d69de4ff67/content/base/src/nsObjectLoadingContent.cpp#l1.216
nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe calls SyncStartPluginInstance calls InstantiatePluginInstance. The changes I made check if the current mime type is for a plugin that is click-to-play, but if there is no mime type at that time, this results in mCTPPlayable being set to true, which allows the plugin instantiation to proceed.
This actually got fixed by the changes from bug 745030, but it's still broken in Aurora.
Josh, John - what do you think the best way to fix this is?

Comment 4

[Josh Aas]

John - will you be able to look into this soon?

Comment 5

[John Schoenick [:johns]]

I can't get this to reproduce on my phone nor desktop, but the whole GetPluginInstanceIfSafe->SyncStartPluginInstance pathway is just broken prior to bug 745030. If we try to instantiate before even having a mime type, nsPluginHost might open the channel (bug 767633), which is just bad and wrong. Fixing this still leaves several edge cases where we might instantiate before LoadObject() has done its job, but those are not a regression from any recent change...

Comment 6

[John Schoenick [:johns]]

Attachment: Don't allow instantiation to reach old nsPluginHost channel opening code

Comment 7

[John Schoenick [:johns]]

Comment on attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code

David - can you reproduce this? This patch is basically just a hack to prevent InstantiatePluginInstance from firing too early, if it fixes this bug then it will probably wallpaper us over to 17 with the refactoring (which fixes this much more properly)

Comment 8

[John Schoenick [:johns]]

Aurora heads were already pushed to try at some point, so I pushed this to see if it breaks anything (hopefully not, or we have bigger problems):

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

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code

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

Ok - I finally was able to reliably reproduce this (only seems to happen on android), and this fixes the bug. So, if there aren't any other regressions, I think this is a good way to go.

Comment 10

[John Schoenick [:johns]]

Comment on attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code

This is only an issue in 16, so we would land it on beta after the uplift dust has settled

Comment 11

[Josh Aas]

Comment on attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +625,5 @@
>      return NS_OK;
>    }
>  
> +  // We don't want to have nsPluginHost handle channel opening
> +  if (!strlen(aMimeType)) {

I prefer to be explicit about types here, not use a boolean operator on an integer:

if (strlen(aMimeType) == 0)

I don't feel strongly about this, if you do then go ahead and leave it how you had it.

Comment 12

[John Schoenick [:johns]]

Comment on attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Regressed by click-to-play blocklist support / bug 760625, fixed on FF17 by Bug 745030

User impact if declined:
Click-to-play will be skipped for plugins in some circumstances (race condition)

Testing completed (on m-c, etc.):
Passes try, fixes test case. This particular patch isn't applicable to m-c due to bug 745030

Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code

[Triage Comment]
Since we're considering uplifting CTP to FF16, and it's already an issue on mobile, let's take a fix for the next beta.

Comment 14

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-beta/rev/87929d2524ff