425013 - Regression in Plugin Finder Service for Standards Compliant Flash

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Yi Ding]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

If you visit http://studentindebt.com/flashtest.html with no flash installed:

In Firefox 2, this launches the plugin finder service, and all is well.
In Firefox 3b4, the plugin finder service is not triggered and the user has to find Flash manually.

The markup on the page is very simple, standards compliant, and using the object-only/flash satay method.  This is the ONLY way to create a crossbrowser standards compliant embedded flash applet.  It's also used with a lot of javascript packages out there.

Reproducible: Always

Steps to Reproduce:
1. Uninstall Flash
2. Visit http://studentindebt.com/flashtest.html using Firefox 3b4
3. Nothing!
4. Visit same page using Firefox 2
5. Install plugin using plugin finder.
6. ???
7. Profit!
Actual Results:  
Plugin finder service not triggered.

Expected Results:  
Plugin finder service triggered.

Comment 1

[Yi Ding]

Here's links to the flash uninstaller if you'd like to test this:
http://kb.adobe.com/selfservice/viewContent.do?externalId=tn_14157

Comment 2

[Yi Ding]

Added ? for blocking-firefox3.  Reason being that if this isn't fixed in firefox 3 but some later version, developers will have to specifically check for firefox 3 and produce alternate markup for firefox 3 specifically (in the same way that we have to produce alternate markup for IE)

Comment 3

[Dave Townsend [:mossop]]

The problem here is that GetPluginSupportState is returning ePluginOtherState for this plugin because there is no pluginurl parameter for the object tag. I'm not totally sure why we would need this?

ePluginOtherState is commented as "not a plugin at all as far as we can tell"

Comment 4

[Dave Townsend [:mossop]]

See also bug 362196

Comment 5

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

I think this is by design. <object> supports fallback, so that is what we display instead of the plugin. It's just that in this case the fallback (i.e. the contents of the <object>) is empty.

Marking non-blocking as this but is likely invalid.


That said, this is the second time this has come up. Maybe we should show the plugin finder if there is no fallback at all?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I would be happy enough with showing the plugin finder in the no-fallback case.  We have all the infrastructure for this already, I think: see the :-moz-empty-except-children-with-localname pseudo-class, which we can use to detect <object>s that only contain <param> and whitespace.

Comment 7

[Yi Ding]

That would be fine.  The key is to have a standards-compliant and consistent way for web designers to trigger the plugin finder service.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'd suggest raising the issue in the HTML working group.  You might want such a way while also providing fallback content...

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #5)
> I think this is by design. <object> supports fallback, so that is what we
> display instead of the plugin. It's just that in this case the fallback (i.e.
> the contents of the <object>) is empty.

Perhaps I am missing something, but is it then a bug that if the <object> contains <param name="pluginurl" value=""> then we do display the finder?

If there is a fallback should we maybe always display that but still dispatch the plugin not found event so the browser can still show the PFS notification bar? I'm pretty much positive that that is what we do for blocklisted plugins.

(In reply to comment #7)
> That would be fine.  The key is to have a standards-compliant and consistent
> way for web designers to trigger the plugin finder service.

Is including the pluginurl param a major issue? Doesn't seem like it would be standards in-compliant to me but I don't know how it behaves elsewhere.

Comment 10

[Yi Ding]

I created a second webpage with the pluginurl:
http://studentindebt.com/flashtest2.html

It seems like it now hits a different issue where it complains about some install-xxx..rdf file.

Unfortunately, this method of putting pluginurl in the param tag isn't very well known, and even the developer wiki currently tells the user to use the first method (with the mime-type):
http://developer.mozilla.org/en/docs/Using_the_Right_Markup_to_Invoke_Plugins

I'll send the html wg an e-mail and see if they can shed some light on this.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> I created a second webpage with the pluginurl:
> http://studentindebt.com/flashtest2.html
> 
> It seems like it now hits a different issue where it complains about some
> install-xxx..rdf file.

That is just bug 416396

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Perhaps I am missing something

Nope.  We special-case having a pluginurl attribute or param.

> If there is a fallback should we maybe always display that but still dispatch
> the plugin not found event

The problem is that this will fire for the common <object><embed/></object> case...  we don't want that, no matter what.

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #12)
> The problem is that this will fire for the common <object><embed/></object>
> case...  we don't want that, no matter what.

Would it be too harsh to fire in the inverse case where we have an object that does *not* have an <embed> as a descendant, even if it has other content?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Some objects also have an <object> as a descendant...

But yeah, I suppose I can deal with firing (but not attaching the XBL binding, right?) if the <object> is one we would otherwise process (e.g. doesn't have a classid) and doesn't have an embed/object/applet as a descendant.

Comment 15

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

A lot of ads do <object><img></object> I don't think we want to show the finder for those either.

I think we should only do it if there is no fallback at all to be honest, i.e. only when there is only <param>s and whitespace.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I can certainly live with that.  ;)

Comment 17

[Johnny Stenback (:jst)]

Attachment: Possible fix.

Boris, what would you think of this then?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 311695 [details] [diff] [review]
Possible fix.

>+      if (child->IsNodeOfType(nsINode::eELEMENT) ||
>+          !child->IsNodeOfType(nsINode::eTEXT)) {
>+        hasAlternateContent = PR_TRUE;
>+      } else {
>+        hasAlternateContent = !child->TextIsOnlyWhitespace();
>+      }

How about replacing that with:

hasAlternateContent = nsStyleUtil::IsSignificantChild(child, PR_TRUE, PR_FALSE);

?  With that, r+sr=bzbarsky.

Comment 19

[Johnny Stenback (:jst)]

Attachment: Updated fix, with tests.

Boris, mind glancing over the tests here?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 311744 [details] [diff] [review]
Updated fix, with tests.

Please replace the 

  ok($1 == $2, "")

pattern with

  is($1, $2, "")

(the latter has better error reporting), and looks wonderful.

Comment 21

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

Comment on attachment 311744 [details] [diff] [review]
Updated fix, with tests.

>@@ -1733,13 +1736,19 @@ nsObjectLoadingContent::GetPluginSupportState(nsIContent* aContent,
>     NS_ASSERTION(child, "GetChildCount lied!");
> 
>     if (child->IsNodeOfType(nsINode::eHTML) &&
>-        child->Tag() == nsGkAtoms::param &&
>-        child->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
>-                           NS_LITERAL_STRING("pluginurl"), eIgnoreCase)) {
>-      return GetPluginDisabledState(aContentType);
>+        child->Tag() == nsGkAtoms::param) {
>+      if (child->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
>+                             NS_LITERAL_STRING("pluginurl"), eIgnoreCase)) {
>+        return GetPluginDisabledState(aContentType);
>+      }
>+    } else if (!hasAlternateContent) {
>+      hasAlternateContent =
>+        nsStyleUtil::IsSignificantChild(child, PR_TRUE, PR_FALSE);
>     }

Won't we consider almost any <param> as alternate content. We won't fall into the |if| most of the time when name isn't plugin url, and then IsSignificantChild will consider the <param> significant since it considers all elements significant.

Comment 22

[Boris Zbarsky [:bzbarsky]]

If I read the patch right, the new logic is:

  hasAlternateContent = false;
  for (each kid) {
    if (kid is param) {
      if (kid is pluginurl param) {
        return stuff;
      }
    } else if (!hasAlternateContent) {
      hasAlternateContent = ...;
    }
  }

So that looks correct to me in terms of ignoring params for hasAlternateContent purposes...

Comment 23

[Johnny Stenback (:jst)]

Attachment: Updated fix for checkin.

Comment 24

[Johnny Stenback (:jst)]

Marking this regression a blocker, we should get this in once the tree opens.

Comment 25

[Johnny Stenback (:jst)]

Fix checked in.