797043 - Clean up nsObjectLoadingContent mActivated state

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

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.

Comment 1

[John Schoenick [:johns]]

Attachment: Better handling for plugin activated state for click-to-play

Comment 2

[John Schoenick [:johns]]

Attachment: Tests

Comment 3

[John Schoenick [:johns]]

Attachment: Tests v2

Dumb whitespace error

Comment 4

[John Schoenick [:johns]]

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.

Comment 5

[John Schoenick [:johns]]

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

Comment 6

[Dana Keeler (she/her) [:keeler]]

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.

Comment 7

[Dana Keeler (she/her) [:keeler]]

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?

Comment 8

[John Schoenick [:johns]]

Attachment: Tests v3

(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

Comment 9

[Mozilla RelEng Bot]

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/[e-mail]-f772a3ce1bf8

Comment 10

[Josh Aas]

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?

Comment 11

[John Schoenick [:johns]]

Attachment: Add canActivate and track plugin-replacement types

Comment 12

[John Schoenick [:johns]]

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?

Comment 13

[Dana Keeler (she/her) [:keeler]]

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?

Comment 14

[John Schoenick [:johns]]

Attachment: Better handling for plugin activated state for click-to-play

Comment 15

[John Schoenick [:johns]]

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.

Comment 16

[John Schoenick [:johns]]

Attachment: Better handling for plugin activated state for click-to-play

And now it's actually different instead of the same patch!

Comment 17

[Josh Aas]

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"?

Comment 18

[John Schoenick [:johns]]

Comment on attachment 668137 [details] [diff] [review]
Better handling for plugin activated state for click-to-play

pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a7448b83c6

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

Comment 19

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/47a7448b83c6

Comment 20

[John Schoenick [:johns]]

Attachment: Followup, only implicitly click-to-play activate actual plugin types

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 21

[John Schoenick [:johns]]

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

Comment 22

[Dana Keeler (she/her) [:keeler]]

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.

Comment 23

[Mozilla RelEng Bot]

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/[e-mail]-2c89e09e5806

Comment 24

[John Schoenick [:johns]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64c5c57132e8