Closed Bug 853995 Opened 7 years ago Closed 5 years ago

Move plugin parameters array from nsPluginInstanceOwner to content

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
e10s - ---

People

(Reporter: johns, Assigned: catalinb)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Priority: -- → P5
No longer blocks: jsplugins-base
Depends on: 1015635
Assignee: jschoenick → cbadea
Attachment #8463580 - Flags: review?(jschoenick)
Attachment #8463580 - Attachment is obsolete: true
Attachment #8463580 - Flags: review?(jschoenick)
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
Attachment #8463636 - Flags: review?(jschoenick)
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
Attachment #8463636 - Flags: review?(jschoenick) → review-
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
Attachment #8472613 - Flags: review?(jschoenick)
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
Attachment #8472613 - Flags: review?(jschoenick) → review+
Attachment #8472613 - Flags: review?(jst)
Comment on attachment 8472613 [details] [diff] [review]
(v2) Move plugin parameters array from nsPluginInstanceOwner to content.

r=jst for the webidl changes.
Attachment #8472613 - Flags: review?(jst) → review+
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
Attachment #8472613 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/f8829fe7dfe1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Depends on: 1061995
You need to log in before you can comment on or make changes to this bug.