853995 - Move plugin parameters array from nsPluginInstanceOwner to content

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P5)

Description

[John Schoenick [:johns]]

nsPluginInstanceOwner has a very old manually-managed array of plugin parameters it obtains by inspecting its content. This should be owned by nsObjectLoadingContent (and be an nsTArray) and queried as necessary.

Comment 1

[Cătălin Badea (:catalinb)]

Attachment: Refactor and move the plugin paramaters array to content.

Comment 2

[Cătălin Badea (:catalinb)]

Attachment: Move plugin parameters array from nsPluginInstanceOwner to content.

Comment 3

[Cătălin Badea (:catalinb)]

Comment on attachment 8463636 [details] [diff] [review]
Move plugin parameters array from nsPluginInstanceOwner to content.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=76bf2e28e734

Comment 4

[John Schoenick [:johns]]

Comment on attachment 8463636 [details] [diff] [review]
Move plugin parameters array from nsPluginInstanceOwner to content.

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

This looks good, and removes a lot of really ugly old code. r- for the tweaks below

::: content/base/src/nsObjectLoadingContent.cpp
@@ +909,5 @@
>  }
>  
> +// Helper function to resolve relative URLs for "pluginpage" parameters.
> +void
> +nsObjectLoadingContent::FixRelativeURL(MozPluginParameter& aParam)

This can just be deleted -- Searching dxr, I think the last use of 'pluginspage' was removed long ago. Additionally, even if it did exist, fixing it up to pass to the plugin is meaningless, as it was used for plugins that were not found. This is probably legacy from when we had the "default plugin"

@@ +924,5 @@
> +  }
> +}
> +
> +void
> +nsObjectLoadingContent::OverrideAttributesIfNeeded(bool aIsJava)

Since this is only called once from BuildParametersArray and shouldn't be called elsewhere, I think this should just be inlined.

@@ +1060,5 @@
> +  }
> +}
> +
> +void
> +nsObjectLoadingContent::BuildParametersArray()

This should check that mCachedAttributes and mCachedParameters are empty and assert + clear them if necessary. Codebase and other parameters could lead to security issues if they're not checked right, so if someone finds a way to confuse this class we don't want to accidentally pass the wrong parameters to something.

@@ +1081,5 @@
> +    content->GetAttr(attrName->NamespaceID(), atom, param.mValue);
> +    atom->ToString(param.mName);
> +    FixRelativeURL(param);
> +
> +     mCachedAttributes.AppendElement(param);

Extra indent

@@ +2242,5 @@
>    /// Attempt to load new type
>    ///
>  
> +
> +  // Cache the current attributes and parameters.

Now that I think about it, this should just be ( mType == eType_Plugin || mType == eType_Null ), since addons might want to query this for blocked plugins and such as well.

::: content/base/src/nsObjectLoadingContent.h
@@ +119,5 @@
>      {
>        mNetworkCreated = aNetworkCreated;
>      }
>  
> +    void GetPluginAttributes(nsTArray<mozilla::dom::MozPluginParameter>& attributes);

Add a comment here explaining that these are the cached + overridden-for-quirks attributes and directly-nested <param> tags that plugins see.

@@ +329,5 @@
> +    void FixRelativeURL(mozilla::dom::MozPluginParameter& aParam);
> +    void OverrideAttributesIfNeeded(bool aIsJava);
> +
> +    // Getter for child <param> elements that are not nested in another plugin
> +    // dom element.

Note that GetPluginAttrs/Parameters() is what most code should be using, and note what the aIgnoreCodebase param does

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +225,5 @@
>      mMIMEType = nullptr;
>    }
> +
> +  for (uint32_t i = 0; i < mCachedParamLength; i++) {
> +    if (!mCachedParamValues || !mCachedParamNames) {

Can one of these be set without the other? This could just be before the for loop, e.g.

> if (!mCachedParamValues && !mCachedParamNames) { return; }
> MOZ_ASSERT(mCachedParamValues && mCachedParamNames)

@@ +434,5 @@
>    if (mRunning == RUNNING) {
>      return NS_OK;
>    }
>  
> +  if (!mOwner) {

> MOZ_ASSERT(false, "Should not be calling Start() on unowned plugin");

Given how crazy this code is its always good to add assertions so we notice if this weirdness ever happens

@@ +447,4 @@
>    nsPluginTagType tagtype;
>    nsresult rv = GetTagType(&tagtype);
>    if (NS_SUCCEEDED(rv)) {
>      // Note: If we failed to get the tag type, we may be a full page plugin, so no arguments

the magical full-page-plugins went away a while ago, so this comment can be deleted, and maybe add a else MOZ_ASSERT(false, ...)

@@ +454,4 @@
>  
> +  mCachedParamLength = attributes.Length() + 1 + params.Length();
> +
> +  // The "PARAM" separator is not counted if there are no params values.

Add a bit to this comment that we leave it in the array on purpose, but don't include it in the length value we give the plugin, just due to legacy behavior.

::: dom/plugins/base/nsNPAPIPluginInstance.h
@@ +394,5 @@
>    bool mHaveJavaC2PJSObjectQuirk;
>  
>    static uint32_t gInUnsafePluginCalls;
> +
> +  uint32_t mCachedParamLength;

Comment that we keep these around because the plugin, in in-process mode, might keep a reference to them.

::: dom/webidl/HTMLObjectElement.webidl
@@ +212,5 @@
>    [ChromeOnly, Throws]
>    void cancelPlayPreview();
>  };
>  
> +dictionary MozPluginParameter {

Comment about what this is used for

Comment 5

[Cătălin Badea (:catalinb)]

Attachment: (v2) Move plugin parameters array from nsPluginInstanceOwner to content.

Comment 6

[Cătălin Badea (:catalinb)]

Comment on attachment 8472613 [details] [diff] [review]
(v2) Move plugin parameters array from nsPluginInstanceOwner to content.

Looks like the xpc-shell tests are not failing because of this patch.
https://tbpl.mozilla.org/?tree=Try&rev=fd48755f7218

Comment 7

[John Schoenick [:johns]]

Comment on attachment 8472613 [details] [diff] [review]
(v2) Move plugin parameters array from nsPluginInstanceOwner to content.

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

LGTM! Thanks for cleaning this mess up

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 8472613 [details] [diff] [review]
(v2) Move plugin parameters array from nsPluginInstanceOwner to content.

r=jst for the webidl changes.

Comment 9

[John Schoenick [:johns]]

Comment on attachment 8472613 [details] [diff] [review]
(v2) Move plugin parameters array from nsPluginInstanceOwner to content.

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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