310021 - When Java is disabled in preferences, shouldn't claim we support it

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If Java is disabled in preferences, the plugin host will still claim that
plugins for the relevant types are enabled; it'll get pretty far into
InstantiateEmbeddedPlugin before detecting that we're Java and it's disabled and
bailing out.

I think we should really be checking this much earlier, both earlier in
instantiation and checking in IsPluginEnabledForType.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This tries to keep the java plugin out of our list altogether if it's disabled,
but if we can't take it out (because we've already loaded it this session),
just marks it disabled so it's not used.  The change to nsPluginModule is just
to fix the issue with two nsPluginHostImpls being created that I ran into... :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200417 [details] [diff] [review]
Updated to tip

This is just a merge to jst's changes to IsPluginEnabledForExtension.

Comment 4

[Boris Zbarsky [:bzbarsky]]

When this lands we should back out the fix for bug 311791 on trunk, probably.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 200417 [details] [diff] [review]
Updated to tip

- In nsPluginHostImpl.cpp:

+#define NS_PLUGIN_FLAG_ENABLED    0x0001    //is this plugin enabled?
+#define NS_PLUGIN_FLAG_OLDSCHOOL  0x0002    //is this a pre-xpcom plugin?
+#define NS_PLUGIN_FLAG_FROMCACHE  0x0004    // this plugintag info was loaded from ache
+#define NS_PLUGIN_FLAG_UNWANTED   0x0008    // this is an unwanted plugin

Wanna add spaces after the two first //'s here, even if you're just moving this code?

+  PRBool HasFlag(PRUint32 flag) { return (mFlags & flag) != 0; }

Now that we've got this, maybe we should mark mFlags protected to make sure it's not touched directly? Or is there a reason not to do that?

sr=jst

Comment 6

[Boris Zbarsky [:bzbarsky]]

> maybe we should mark mFlags protected to make sure it's not touched directly?

Did that.  I had to add a Flags() accessor to make serialization to the plugin registry file work... That code should really move into a method on nsPluginTag, probably.  :(

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 200417 [details] [diff] [review]
Updated to tip


nsPluginHostImpl.cpp:
+nsPluginHostImpl::GetInst()
+  NS_IF_ADDREF(sInst);
+  return sInst;

sInst is never null at this point...

     Destroy();
     UnloadUnusedLibraries();
+    NS_RELEASE(sInst);

Hmm, this set sInst to nsnull... is that a good idea? wouldn't it be better to
do that in the destructor?

TrySetupPluginInstance:
+  if(!aMimeType || !(pluginTag = FindPluginForType(aMimeType, PR_TRUE))) {

FindPluginForType handles a null type just fine, and I think lines like this one
would be easier readable if you made use of that:

 nsPluginTag* pluginTag = FindPluginForType(aMimeType, PR_TRUE);
 if (!tag) {
  ...
 }

InstantiateEmbeddedPlugin
-    if((!aMimeType || !isJava) && bCanHandleInternally)
+    if(!isJava && bCanHandleInternally)

Hm... why is this change correct?

nsPluginHostImpl.h:
+    // Remove mime types added to the catagory manager only if we were

catagory -> category :) (twice, actually)



+  nsPluginTag();  // Not to be implemented

So er... why declare this at all? If you have a custom constructor, the default
constructor will not be autogenerated (only the copy constructor will)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> sInst is never null at this point...

It could be if AddPrefObserver failed, but I've added an early return in that case now and made the addref not use IF.

> Hmm, this set sInst to nsnull... is that a good idea? wouldn't it be better to
> do that in the destructor?

Hmm... I guess so, sure.

> FindPluginForType handles a null type just fine

Good catch.

> Hm... why is this change correct?

  (!aMimeType || !isJava) == !(aMimeType && isJava).

But isJava is only true if aMimeType is non-null (both before and after my changes).  So 

  (aMimeType && isJava) == isJava

which is what I changed to here.

> catagory -> category :) (twice, actually)

That's what I get for copy/pasting comments.  ;)

> So er... why declare this at all?

Indeed.  Removed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip and comments

Comment 10

[Simon Fraser [no longer active]]

Related to bug 210724?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Not really.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed.