Closed Bug 409025 Opened 14 years ago Closed 14 years ago

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

Categories

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

All
Windows Vista
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: vlad.alexander, Assigned: mossop)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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.
Blocks: 1156
Severity: major → normal
Component: Plugin Finder Service → Plug-ins
Product: Firefox → Core
QA Contact: plugin.finder → plugins
Summary: PFS is not working in FF3b2 → OBJECTs with type attribute but no data attribute do not fire plugin errors
Version: unspecified → Trunk
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.
Flags: blocking1.9?
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #294730 - Flags: review?(cbiesinger)
Whiteboard: [has patch]
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Lowering priority. I don't think this'll affect terribly many sites. Still nice to have fixed of course.
Priority: P1 → P4
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.
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.)
Boris, any objections to taking the attached patch?
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.

Upping the priority on this one in hope to prevent this from falling through the cracks.
Priority: P4 → P2
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
Attachment #294730 - Flags: review?(cbiesinger) → review-
Whiteboard: [has patch]
Attached patch patch rev 2 (obsolete) — Splinter Review
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.
Attachment #294730 - Attachment is obsolete: true
Attachment #305643 - Flags: review?(cbiesinger)
Whiteboard: [has patch]
Attachment #305643 - Flags: review?(cbiesinger) → review+
Attachment #305643 - Flags: superreview?(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
Attachment #305643 - Flags: superreview?(jst) → superreview+
Switch to reference, ready for checkin.
Attachment #305643 - Attachment is obsolete: true
Attachment #305874 - Flags: superreview+
Attachment #305874 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla1.9beta4
Flags: in-testsuite?
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.
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.
Thanks Dave. Where can I get information on the "installer" and "extension package" options?
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.

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 :(
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.
That should be a separate bug. This bug is not really related to that at all.
Thank you, reported the bug as #424137.
Depends on: 429157
You need to log in before you can comment on or make changes to this bug.