Clean up the PluginHost API

RESOLVED FIXED in Firefox 39

Status

()

Core
Plug-ins
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

This is John's part 2 from bug 558184, spun out into a separate bug.
Created attachment 8568785 [details] [diff] [review]
Attachment 8535419 [details] [diff] merged to tip
Attachment #8568785 - Attachment description: Attachment merged to tip → Attachment 8535419 merged to tip
Depends on: 1136388
Comment on attachment 8568785 [details] [diff] [review]
Attachment 8535419 [details] [diff] merged to tip

>+++ b/dom/base/nsObjectLoadingContent.cpp
>@@ -1611,18 +1592,19 @@ nsObjectLoadingContent::UpdateObjectPara
>+      nsRefPtr<nsPluginHost> pluginHost = nsPluginHost::GetInst();
>       if (StringBeginsWith(classIDAttr, NS_LITERAL_STRING("java:")) &&
>-          PluginExistsForType(javaMIME)) {
>+          pluginHost->HavePluginForType(javaMIME)) {

Need to null-check pluginHost.

>@@ -2663,17 +2645,19 @@ nsObjectLoadingContent::GetTypeOfContent
>+  nsRefPtr<nsPluginHost> pluginHost = nsPluginHost::GetInst();
>+  if (caps & eSupportPlugins &&
>+      pluginHost->HavePluginForType(aMIMEType, nsPluginHost::eExcludeNone)) {

And here.

>@@ -3201,26 +3185,29 @@ nsObjectLoadingContent::ShouldPlay(Fallb
>+  pluginHost->GetBlocklistStateForType(mContentType.get(),

Why not have GetBlocklistStateForType also take an XPCOM string?

>+++ b/dom/plugins/base/nsIPluginHost.idl
>+  const uint32_t EXCLUDE_NONE     = 0;
>+  const uint32_t EXCLUDE_DISABLED = 1 << 0;

So this means the default here will be EXCLUDE_NONE if the argument is not passed, whereas for the C++ bits that were added the default is EXCLUDE_DISABLED.

That's probably OK, I guess.

>+ // FIXME doc below four

Added some documentation.

>+++ b/dom/plugins/base/nsPluginHost.cpp
>@@ -943,17 +938,18 @@ nsPluginHost::TrySetUpPluginInstance(con
>+  const nsDependentCString type(aMimeType);

What guarantees aMimeType is not null?  Probably better to just change this to take an XPCOM string.

>+nsPluginHost::FindNativePluginForType(const nsACString & aMimeType,
>+  if (!aMimeType.Length()) {

aMimeType.IsEmpty()

>+nsPluginHost::FindNativePluginForExtension(const nsACString & aExtension,
>+  if (!aExtension.Length()) {

aExtension.IsEmpty()

>@@ -1370,17 +1337,18 @@ nsresult nsPluginHost::GetPlugin(const c
>+  const nsDependentCString type(aMimeType);

Nothing guarantees aMimeType is non-null here.  Luckily, this needs to change to take nsACString anyway for some of the above review comments.

> nsPluginHost::SiteHasData(nsIPluginTag* plugin, const nsACString& domain,
>+  // FIXME audit casts

FIXME-jsplugins.  Not an issue for now, obviously.

Similar for other FIXMEs in this patch.

>+++ b/dom/plugins/base/nsPluginTags.cpp
>+// check comma delimitered extensions

"delimited"

>+  const char *pComma = strchr(pExt, ',');

Seems like a use case for nsCCharSeparatedTokenizer.

>+// Search for an extension in an extensions array, and optionally return its
>+// matching mime type
>+static bool SearchExtensions(const nsTArray<nsCString> & aExtensions,

Returning the MIME type is not optional, right?

>+    if (ExtensionInList(aExtensions[i].get(), aFindExtension.Data())) {

.Data() is not safe, since the result might not be null-terminated.  Better to
just work with XPCOM strings throughout here.

>+MakeNiceFileName(const nsCString & aFileName)
>+  NS_ASSERTION(niceNameLength != kNotFound, "mFileName doesn't have a '.'?");

aFileName

>+  // entire mFileName (which we've already taken care of, a few lines back)

aFileName

>+CStringArrayToXPCArray(nsTArray<nsCString> & aArray,
>+  if (!count) {
>+    return NS_ERROR_NOT_AVAILABLE;

That's a behavior change... Maybe it's OK.

>+  *aResults =
>+    static_cast<char16_t**>(nsMemory::Alloc(count * sizeof(**aResults)));
>+  if (!*aResults)

nsMemory::Alloc is infallible.

>+++ b/dom/plugins/base/nsPluginTags.h
>+  bool          HasMimeType(const nsACString & aMimeType) const;
>+  bool          HasExtension(const nsACString & aExtension,
>+                             /* out */ nsACString & aMatchingType) const;

Needs more documentation.

>+++ b/layout/build/nsContentDLF.cpp
>+  const nsDependentCString contentType(aContentType);

What guarantees that aContentType is not null?  Filed bug 1136388 on this.

>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
>-  nsPluginHost* pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
>+  nsRefPtr<nsPluginHost> pluginHost = nsPluginHost::GetInst();
>   if (NS_SUCCEEDED(rv)) {

Need to null-check pluginHost now.

r=me with the above fixed
Attachment #8568785 - Flags: review+
Created attachment 8568858 [details] [diff] [review]
Updated to review comments
Attachment #8568785 - Attachment is obsolete: true
Created attachment 8569198 [details] [diff] [review]
Updated to fix orange: we can't make nsIPluginTag builtinclass because there is JS code that pretends to have them around
Attachment #8568858 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b5487e4ca9
https://hg.mozilla.org/mozilla-central/rev/82b5487e4ca9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Updated

3 years ago
Depends on: 1137897
You need to log in before you can comment on or make changes to this bug.