Clean up nsObjectLoadingContent mActivated state

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: johns, Assigned: johns)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

GetActivated() behaves like GetShouldActivate(), and mActivated lives as long as the object such that swapping vulnerable plugin types in to a clicked-on object doesn't re-trigger a click-to-play warning.
Posted patch Tests (obsolete) — Splinter Review
Posted patch Tests v2 (obsolete) — Splinter Review
Dumb whitespace error
Attachment #667201 - Attachment is obsolete: true
Comment on attachment 667200 [details] [diff] [review]
Better handling for plugin activated state for click-to-play

This should cause mActivated to reset when the effective content type changes. As a bonus, calling PlayPlugin() on a playing-plugin is now a no-op, so the front-end can use much simpler logic around it.
Attachment #667200 - Flags: feedback?(dkeeler)
Attachment #667206 - Flags: feedback?(dkeeler)
Comment on attachment 667200 [details] [diff] [review]
Better handling for plugin activated state for click-to-play

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

Great - I think this is exactly what we want.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2518,2 @@
>    mPlayPreviewCanceled = true;
> +  

Looks like some whitespace here.
Attachment #667200 - Flags: feedback?(dkeeler) → feedback+
Attachment #667200 - Flags: review?(joshmoz)
Attachment #667206 - Flags: review?(joshmoz)
Comment on attachment 667206 [details] [diff] [review]
Tests v2

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

Looks good - just a couple of comments.

::: browser/base/content/test/browser_pluginnotification.js
@@ +762,5 @@
> +  var objLoadingContent = pluginNode.QueryInterface(Ci.nsIObjectLoadingContent);
> +  is(objLoadingContent.pluginFallbackType, Ci.nsIObjectLoadingContent.PLUGIN_CLICK_TO_PLAY, "Test 21, plugin fallback type should be PLUGIN_CLICK_TO_PLAY");
> +
> +  // Activate
> +  objLoadingContent.playPlugin();

Is playPlugin synchronous? You might have to use waitForCondition (see, e.g., lines 245-6).

@@ +769,5 @@
> +
> +  // Reload plugin
> +  var oldVal = pluginNode.getObjectValue();
> +  pluginNode.src = pluginNode.src;
> +  is(objLoadingContent.displayedType, Ci.nsIObjectLoadingContent.TYPE_PLUGIN, "Test 21, Plugin should have retained activated state");

should you check that pluginNode.activated is still true?

@@ +793,5 @@
> +  var objLoadingContent = pluginNode.QueryInterface(Ci.nsIObjectLoadingContent);
> +  is(objLoadingContent.pluginFallbackType, Ci.nsIObjectLoadingContent.PLUGIN_CLICK_TO_PLAY, "Test 22, plugin fallback type should be PLUGIN_CLICK_TO_PLAY");
> +
> +  // Activate
> +  objLoadingContent.playPlugin();

same as other playPlugin comment

@@ +803,5 @@
> +  pluginNode.src = pluginNode.src; // We currently don't properly change state just on type change, bug 767631
> +  is(objLoadingContent.displayedType, Ci.nsIObjectLoadingContent.TYPE_NULL, "Test 22, plugin should be unloaded");
> +  pluginNode.type = "application/x-test";
> +  pluginNode.src = pluginNode.src;
> +  is(objLoadingContent.displayedType, Ci.nsIObjectLoadingContent.TYPE_NULL, "Test 22, Plugin should not have activated");

check pluginNode.activated is false?
Attachment #667206 - Flags: feedback?(dkeeler) → feedback+
Posted patch Tests v3Splinter Review
(In reply to David Keeler from comment #7)
> Is playPlugin synchronous? You might have to use waitForCondition (see,
> e.g., lines 245-6).

Currently it will change object state synchronously, I added the check for the plugin-reload below it to ensure that if we change this in the future the test will break, but there's no real defined behavior yet.

> should you check that pluginNode.activated is still true?
> ...
> check pluginNode.activated is false?

Added
Attachment #667206 - Attachment is obsolete: true
Attachment #667206 - Flags: review?(joshmoz)
Attachment #667220 - Flags: review?(joshmoz)
Try run for f772a3ce1bf8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f772a3ce1bf8
Results (out of 91 total builds):
    success: 85
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jschoenick@mozilla.com-f772a3ce1bf8
Comment on attachment 667200 [details] [diff] [review]
Better handling for plugin activated state for click-to-play

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2486,5 @@
> +  }
> +
> +  // If we're in a click-to-play or play preview state, we need to reload
> +  if (mType == eType_Null && (mFallbackType == eFallbackPlayPreview ||
> +                              mFallbackType == eFallbackClickToPlay)) {

Aren't there other states we can be in where a click should lead to playing the plugin, like eFallbackVulnerableNoUpdate or eFallbackOutdated? Why only allow loading for these two types here?
Attachment #667200 - Flags: review?(joshmoz)
Comment on attachment 667743 [details] [diff] [review]
Add canActivate and track plugin-replacement types

(In reply to Josh Aas (Mozilla Corporation) from comment #10)
> Aren't there other states we can be in where a click should lead to playing
> the plugin, like eFallbackVulnerableNoUpdate or eFallbackOutdated? Why only
> allow loading for these two types here?

This is a good point, we don't really have a definitive list of what types are 'plugin-replacing' like this.

This patch explicitly denotes that all fallback types >= eFallbackClickToPlay are 'activatable' types and adds a (chrome-only) canActivate parameter, so the other patch can just gate PlayPlugin() on this

@david does this look sane?
Attachment #667743 - Flags: feedback?(dkeeler)
Comment on attachment 667743 [details] [diff] [review]
Add canActivate and track plugin-replacement types

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

Unfortunately, PLUGIN_PLAY_PREVIEW is handled (in browser-plugins.js) separately from PLUGIN_CLICK_TO_PLAY and friends, so I don't think it makes sense to have canActivate be all values >= PLUGIN_CLICK_TO_PLAY. Maybe we could move PLUGIN_PLAY_PREVIEW to be less than PLUGIN_CLICK_TO_PLAY?

::: content/base/public/nsIObjectLoadingContent.idl
@@ +48,5 @@
>    // Suppressed by security policy
>    const unsigned long PLUGIN_SUPPRESSED           = 6;
>    // Blocked by content policy
>    const unsigned long PLUGIN_USER_DISABLED        = 7;
> +  /// ** All values >= PLUGIN_CLICK_TO_PLAY are plugin placeholder types that

If we do go ahead with this, maybe we want to leave some space for future non-click-to-play plugin types?
Attachment #667743 - Flags: feedback?(dkeeler)
Attachment #667200 - Attachment is obsolete: true
Attachment #667743 - Attachment is obsolete: true
Comment on attachment 668133 [details] [diff] [review]
Better handling for plugin activated state for click-to-play

Alright, lets scrap the canActivate part -- the frontend needs to be aware of the available types either way, so it's fairly superfluous. This is the same as the original patch except it adds explicit notes that types >= PLUGIN_CLICK_TO_PLAY are plugin-replacement-types so we don't need to list them.
Attachment #668133 - Flags: review?(joshmoz)
And now it's actually different instead of the same patch!
Attachment #668133 - Attachment is obsolete: true
Attachment #668133 - Flags: review?(joshmoz)
Attachment #668137 - Flags: review?(joshmoz)
Comment on attachment 668137 [details] [diff] [review]
Better handling for plugin activated state for click-to-play

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2487,5 @@
> +
> +  // If we're in a click-to-play or play preview state, we need to reload
> +  // Fallback types >= eFallbackClickToPlay are plugin-replacement types, see
> +  // header
> +  if (mType == eType_Null && mFallbackType >= eFallbackClickToPlay) {

The "mType == eType_Null" check seems fine, but is it really necessary? In what situation would we be executing ::PlayPlugin with "mType != eType_Null"?
Attachment #668137 - Flags: review?(joshmoz) → review+
Attachment #667220 - Flags: review?(joshmoz) → review+
Attachment #667220 - Flags: checkin+
Attachment #668137 - Flags: review+ → checkin+
https://hg.mozilla.org/mozilla-central/rev/47a7448b83c6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Argh, so, |mType != eType_Null| is wrong here, because we'd implicitly activate
type Loading, which is never inspected by CTP (as the MIME type is not yet
known).  Changing this to only actually activate plugin types is much saner, and
as a bonus, if a document reloads and is suddenly handled by a plugin, we'll CTP
the plugin (since previous load was based on the assumption the type was native).
Comment on attachment 672987 [details] [diff] [review]
Followup, only implicitly click-to-play activate actual plugin types

david, will this cause front end issues by not activating images/documents? PlayPlugin() wont reload said types, and they're never checked for CTP as is anyway, so I'd think it would be sane
Attachment #672987 - Flags: feedback?(dkeeler)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 672987 [details] [diff] [review]
Followup, only implicitly click-to-play activate actual plugin types

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

We only use the activated property for plugins, as far as I know, so I don't think there would be a problem with images/documents.
Attachment #672987 - Flags: feedback?(dkeeler) → feedback+
Attachment #672987 - Flags: review?(joshmoz)
Try run for 2c89e09e5806 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2c89e09e5806
Results (out of 49 total builds):
    success: 46
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jschoenick@mozilla.com-2c89e09e5806
Attachment #672987 - Flags: review?(joshmoz) → review+
Comment on attachment 672987 [details] [diff] [review]
Followup, only implicitly click-to-play activate actual plugin types

https://hg.mozilla.org/integration/mozilla-inbound/rev/64c5c57132e8
Attachment #672987 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/64c5c57132e8
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.