Last Comment Bug 780075 - Flash Plugin settings for "Tap to play" is ignored for the flash embedded elements
: Flash Plugin settings for "Tap to play" is ignored for the flash embedded ele...
Status: RESOLVED FIXED
regression
: regression
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 16 Branch
: ARM Android
: -- major (vote)
: mozilla16
Assigned To: John Schoenick [:johns]
:
Mentors:
http://people.mozilla.org/~mwargers/t...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-02 23:59 PDT by Adrian Tamas (:AdrianT)
Modified: 2012-08-28 13:38 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
unaffected
unaffected
16+


Attachments
Don't allow instantiation to reach old nsPluginHost channel opening code (988 bytes, patch)
2012-08-22 13:54 PDT, John Schoenick [:johns]
jaas: review+
dkeeler: feedback+
akeybl: approval‑mozilla‑beta+
john: checkin+
Details | Diff | Splinter Review

Description Adrian Tamas (:AdrianT) 2012-08-02 23:59:43 PDT
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 2012-08-03 08:45:39 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-10 11:42:42 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-08-17 17:05:12 PDT
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 2012-08-21 09:55:12 PDT
John - will you be able to look into this soon?
Comment 5 John Schoenick [:johns] 2012-08-22 13:54:10 PDT
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] 2012-08-22 13:54:37 PDT
Created attachment 654360 [details] [diff] [review]
Don't allow instantiation to reach old nsPluginHost channel opening code
Comment 7 John Schoenick [:johns] 2012-08-22 13:57:11 PDT
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] 2012-08-22 15:26:37 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-08-27 14:12:11 PDT
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] 2012-08-27 14:27:01 PDT
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 2012-08-27 15:47:41 PDT
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] 2012-08-27 16:14:40 PDT
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] 2012-08-27 17:40:22 PDT
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] 2012-08-28 12:51:54 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/87929d2524ff

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