Last Comment Bug 425013 - Regression in Plugin Finder Service for Standards Compliant Flash
: Regression in Plugin Finder Service for Standards Compliant Flash
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
:
Mentors:
http://studentindebt.com/flashtest.html
Depends on: 1200602
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-25 09:09 PDT by Yi Ding
Modified: 2015-09-01 07:22 PDT (History)
7 users (show)
jst: blocking1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible fix. (1.68 KB, patch)
2008-03-25 17:29 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Updated fix, with tests. (4.46 KB, patch)
2008-03-25 23:52 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Updated fix for checkin. (4.82 KB, patch)
2008-03-26 12:53 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
jst: superreview+
Details | Diff | Splinter Review

Description Yi Ding 2008-03-25 09:09:20 PDT
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 2008-03-25 09:13:33 PDT
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 2008-03-25 09:20:46 PDT
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] 2008-03-25 11:38:34 PDT
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] 2008-03-25 11:45:48 PDT
See also bug 362196
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-03-25 13:03:02 PDT
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 [:bz] (still a bit busy) 2008-03-25 13:20:41 PDT
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 2008-03-25 13:38:50 PDT
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 [:bz] (still a bit busy) 2008-03-25 13:48:52 PDT
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] 2008-03-25 14:01:37 PDT
(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 2008-03-25 14:16:50 PDT
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] 2008-03-25 14:23:49 PDT
(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 [:bz] (still a bit busy) 2008-03-25 14:25:00 PDT
> 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, jst@mozilla.com) 2008-03-25 14:53:36 PDT
(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 [:bz] (still a bit busy) 2008-03-25 15:24:41 PDT
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 2008-03-25 15:46:34 PDT
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 [:bz] (still a bit busy) 2008-03-25 15:50:13 PDT
I can certainly live with that.  ;)
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-25 17:29:10 PDT
Created attachment 311695 [details] [diff] [review]
Possible fix.

Boris, what would you think of this then?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2008-03-25 19:19:13 PDT
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, jst@mozilla.com) 2008-03-25 23:52:21 PDT
Created attachment 311744 [details] [diff] [review]
Updated fix, with tests.

Boris, mind glancing over the tests here?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2008-03-26 05:40:31 PDT
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 2008-03-26 11:09:08 PDT
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 [:bz] (still a bit busy) 2008-03-26 11:25:08 PDT
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, jst@mozilla.com) 2008-03-26 12:53:11 PDT
Created attachment 311846 [details] [diff] [review]
Updated fix for checkin.
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-26 12:54:56 PDT
Marking this regression a blocker, we should get this in once the tree opens.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-26 16:05:29 PDT
Fix checked in.

Note You need to log in before you can comment on or make changes to this bug.