409025 - OBJECTs with type attribute but no data attribute do not fire plugin errors

Status: RESOLVED FIXED

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

Description

[Vlad Alexander]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

We produce a plug-in for Firefox. It used to auto-install via PFS. In FF3b2, you no longer get a prompt saying that this Web page requires a plug-in with the option to install the plug-in.

Reproducible: Always

Steps to Reproduce:
1. Go to: http://xstandard.com/test.asp
2. You should be prompted to install the plug-in.

Actual Results:  
No prompt to install the plug-in.

Expected Results:  
Prompt to install the plug-in.

Comment 1

[Dave Townsend [:mossop]]

The problem is that we aren't firing the PluginNotFound error when the OBJECT has a type but no data to load and there is no installed plugin for the type.

Comment 2

[Dave Townsend [:mossop]]

In certain cases this will mean that the user will get no warning that content in the page is not displayed because the plugin is blocklisted so requesting blocking.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is pretty much right I think.

When there is no data uri and we do have a mime type then we need to check whether there is just no installed plugin or if there is an installed but disabled or blocklisted plugin and send errors accordingly.

When there is a uri that we can't handle we check for a plugin to handle it and if not then do as above.

Only snag I have is that for some reason this doesn't properly attach the plugin binding to unknown types, but it looks like we didn't do that in 2.0 either.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Lowering priority. I don't think this'll affect terribly many sites. Still nice to have fixed of course.

Comment 5

[Vlad Alexander]

I don't think it's right to lower priority on this bug. The PFS, a major FF feature, is broken. Our customers depend on this feature.

Comment 6

[Dave Townsend [:mossop]]

I should add that this also affects object tags with a data attribute that is a URI we are unable to handle (mms:// f.e.)

Comment 7

[Johnny Stenback (:jst)]

Boris, any objections to taking the attached patch?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Not offhand.  It seems like a reasonable approach.  I'd like biesi to review, and it would be good to have all that stuff starting with GetPluginDisabledState() factored out into a method (one that takes an nsAutoFallback object reference) instead of being copy/pasted, but other than that looks ok.

Comment 9

[Johnny Stenback (:jst)]

Upping the priority on this one in hope to prevent this from falling through the cracks.

Comment 10

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

Comment on attachment 294730 [details] [diff] [review]
patch rev 1

+        fallback.TypeUnsupported();
+      }
+    }
+
     return NS_OK;

don't you need to set rv to NS_OK, to avoid the AutoFallback triggering fallback in the disabled/blocklisted cases?

I also agree with bz

+      if (pluginState != ePluginDisabled &&
+          pluginState != ePluginBlocklisted) {
+        fallback.TypeUnsupported();
+      }

really, that should probably be an if (state == disabled || state == blocklisted) rv = NS_OK;

(you set rv to failure above)

r-, I'd like to look a the final patch

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This splits out the notification bits into a static method per bz's comments and sprinkles some additional comments in.

>      return NS_OK;
> 
> don't you need to set rv to NS_OK, to avoid the AutoFallback triggering
> fallback in the disabled/blocklisted cases?

No I don't believe so. We still want to display any fallback content specified in the html for these cases, we do not however want the plugin not found binding to attach so we don't mark it as an unsupported type.

> really, that should probably be an if (state == disabled || state ==
> blocklisted) rv = NS_OK;
> 
> (you set rv to failure above)

I've tweaked the actual small bit of code to what I /think/ is more correct. Same as before if it is unsupported or blocklisted we notify with an event. Now though we only attach the notfound binding if the result was unsupported, before it would happen on unsupported or "OtherState" which looking for when it happens doesn't seem right to me.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 305643 [details] [diff] [review]
patch rev 2

+void
+nsObjectLoadingContent::UpdateFallbackState(nsIContent* aContent,
+                                            AutoFallback* fallback,
+                                            const nsCString& aTypeHint)

I'd rather see the fallback argument be a reference than a pointer, but I can live either way.

sr=jst

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Switch to reference, ready for checkin.

Comment 14

[Dave Townsend [:mossop]]

Landed

Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.68; previous revision: 1.67
done
Checking in content/base/src/nsObjectLoadingContent.h;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.h,v  <--  nsObjectLoadingContent.h
new revision: 1.23; previous revision: 1.22
done

Comment 15

[Vlad Alexander]

Just tested this in FF 3 Beta 4, and I get the following error:

http://misc.xstandard.com/mozilla/pfs-error.gif

Not sure what this means.

Comment 16

[Dave Townsend [:mossop]]

It means you are still using the unsupported install.js for plugin installation you need to switch to an installer or extension package for installation.

Comment 17

[Vlad Alexander]

Thanks Dave. Where can I get information on the "installer" and "extension package" options?

Comment 18

[Vlad Alexander]

RE: extension package
In FF3, plug-in installs require a restart of FF but the PFS installer does not prompt the user to restart FF. So the plug-in gets installed but it's not visible. If the restart cannot be automatic, can you please make the restart option part of the PFS dialog box, instead of the information bar. This will make the install process more seamless.

You can use this page for testing on Windows:
http://xstandard.com/test.asp


RE: installer
For us, many users install via PFS and then upgrade via installer (EXE or DMG). Because of the order in which FF looks for plug-ins, plugs-ins installed via PFS will be found before plug-ins in the "plugins" folder (Windows) or "Internet Plugs-ins" folder (OS X) or another location specified by the registry (on Windows). This makes it really difficult to upgrade via installer. On Windows, our installer first deletes the plug-in from the "plugins" folder, then deletes it from the "extensions", then installs the new version in Program Files and sets the registry key to the location of the plug-in. However, the Package Installer on OS X is very limited in functionality and cannot do the same as our Windows installer. This means we cannot upgrade the plug-in via DMG on OS X.

I would therefore ask that the order in which plug-ins are found should be changed. FF should first look for the plug-in the the registry (on Windows), then the "plugins"/"Internet Plugs-ins" folder, then the extensions folder in the user's profile. IE works a similar way - COM objects registered last takes precedence.

FYI, we cannot use FF3's extensions upgrade mechanism because developers need to control the version of the plug-in installed in their Web applications and when the upgrade occurs. Otherwise applications break. In IE, they have this facility through CAB files.

Comment 19

[Johnny Stenback (:jst)]

Changing the plugin lookup order is *not* something I'd be willing to do this far into a release. Early in a release sure, but not way right now unless something is *really* wrong and a large percentage of our users would suffer from it :(

Comment 20

[Vlad Alexander]

I totally understand Johnny.

Talking about the future, the ideal plug-in install/upgrade feature, which could replace PFS, would be to use XPI files in the same way IE uses CAB files. For example:

<object type="application/x-xstandard" id="editor1" width="100%" height="300" codebase="http://domain.com/XStandard.xpi#Version=2,0,0,0">

Johnny, in regards to the absence of browser restart when installing via PFS, should I report it as a separate bug or can this bug report be re-opened? This is quite a serious issue for us.

Comment 21

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

That should be a separate bug. This bug is not really related to that at all.

Comment 22

[Vlad Alexander]

Thank you, reported the bug as #424137.