47712 - full page plugins don't work by file extension

Status: VERIFIED FIXED

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

Description

[sean echevarria]

Full page plugins are not invoked for local files - mozilla displays the unknown
file type dialog.
Nor are they invoked for http files that don't explicitly have the content type
set on them.
This is related to http://bugzilla.mozilla.org/show_bug.cgi?id=43556 - but that
seems to be addressing only helper apps - plugins are not mentioned.

Before displaying the unknown file type dialog, mozilla should check the file
extensions that are supported by the installed plugins.

Comment 1

[av (gone)]

Right, I know this. There is a bug in mime type service preventing from doing 
this. As far as I remember it is nominated for beta3 -- yes, 43556.

Comment 2

[sean echevarria]

Just wanted to be clear that 43556 would affect plugins as well.

Marking dupe.


*** This bug has been marked as a duplicate of 43556 ***

Comment 3

[shrirang khanzode]

reopening for mac, OS : Mac 9.0.

Comment 4

[shrirang khanzode]

reassign :peterl

Comment 5

[Peter Lubczynski]

Well, this explains bug 14496 and bug 35682. Accepting and marking blocker.

Comment 6

[sean echevarria]

see also bug 53868

Comment 7

[Peter Lubczynski]

Changing peterl-retired to peterlubczynski

Comment 8

[Mike Kaply [:mkaply]]

I can give more information on this bug.

This actually works on Windows only because code in nsOSHelperAppService.cpp 
looks in the Windows registry to associate file extensions.

You can easily get Windows to break by going into the Windows registry, 
searching on .swf, and changing it to .swx. Surprise, no fullscreen Flash 
plugins on Windows.

Anyway, about a possibility of fixing this.

At:

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperApp
Service.cpp#122

There is a call to AddDefaultMimeTypesToCache

There should be another API - AddPluginMimeTypesToCache that queries the mime 
type/file extension pairs and adds them to the cache.

Basically, any extensions that Mozilla understands in an XP way (default and 
plugin) should be preadded to the cache and not queried at the OS level.

So if someone can tell me how to get the plugin mime type/extension pairs, I 
could put together a fix for this.

Comment 9

[sean echevarria]

One place plugin mimetypes and extensions are recorded is at: 
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginHostImpl.c
pp#1754

That one handles xpcom plugins.  There's another place where they are recorded 
for 4x plugins - but I can't find it right now, will come back to it later.

http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginHostImpl.c
pp#2644 might be of interest also.

Comment 10

[sean echevarria]

All mimetype info for all plugins is available via nsPluginHostImpl::mPlugins.  
Might want to take a look at 
nsPluginHostImpl::RegisterPluginMimeTypesWithLayout in 
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginHostImpl.c
pp#2371.  
It gets called for each plugin (xpcom and 4x) with the plugin info 
(nsPluginTag) that is eventually added to mPlugins.

Comment 11

[Mike Kaply [:mkaply]]

OK, this is actually kind of working.

I added a new function called AddPluginMimeTypesToCache.

Please critique - note I am still looking for a better way to handle my 
AutoStrings

nsresult nsExternalHelperAppService::AddPluginMimeTypesToCache()
{
  nsresult rv;
  NS_WITH_SERVICE(nsIPluginHost, pluginHost, kPluginManagerCID, &rv);
  PRUint32 tPluginCount;
  pluginHost->GetPluginCount(&tPluginCount);
  nsIDOMPlugin** tPluginArray = new nsIDOMPlugin*[tPluginCount];
  if (tPluginArray != nsnull) {
    rv = pluginHost->GetPlugins(tPluginCount, tPluginArray);
    if (rv == NS_OK) {
      for (int i=0;i<tPluginCount;i++) {
        PRUint32 tVariants;
        tPluginArray[i]->GetLength(&tVariants);
        for (int j=0;j<tVariants;j++) {
          nsIDOMMimeType* prop;
          rv = tPluginArray[i]->Item(j, &prop);
          if (prop) {
            // create a mime info object for each default plugin entry and add 
it to our cache
            nsCOMPtr<nsIMIMEInfo> mimeInfo 
(do_CreateInstance(NS_MIMEINFO_CONTRACTID));
            char* tempstring;
            PRUnichar* tempustring;
            nsAutoString suffixes;
            prop->GetSuffixes(suffixes);
            tempstring = suffixes.ToNewCString();
            mimeInfo->SetFileExtensions(tempstring);
            nsMemory::Free(tempstring);
            nsAutoString type;
            prop->GetType(type);
            tempstring = type.ToNewCString();
            mimeInfo->SetMIMEType(tempstring);
            nsMemory::Free(tempstring);
            nsAutoString description;
            prop->GetDescription(type);
            tempustring = type.ToNewUnicode();
            mimeInfo->SetDescription(tempustring);
            nsMemory::Free(tempustring);
            AddMimeInfoToCache(mimeInfo);
            NS_RELEASE(prop);
          } /* endif */
        } /* endfor */
      } /* endfor */
    }
    delete[] tPluginArray;
  }
}

Comment 12

[timeless]

i think NS_WITH_SERVICE might not be the prefered way to do services

|if (tPluginArray != nsnull) {| should be |if (tPluginArray) {|
|for (int i=0;i<tPluginCount;i++) {| should probably be unsigned, and probably 
a prtype
|for (int j=0;j<tVariants;j++) {| similar, especially since you have a 
signed/unsigned comparison warning

|nsIDOMPlugin** tPluginArray = new nsIDOMPlugin*[tPluginCount];| might possibly 
be converted to nscomptr
-PC/Mac System 9.x now there's a strange animal => All/All

Comment 13

[timeless]

from jag:
nsCOMPtr<nsIFoo> foo(do_GetService(contractID, &rv));
// prefer contractID, otherwise use CID
i think this is what you want:
nsCOMPtr<nsIPluginHost> pluginHost(do_GetService(kPluginManagerCID, &rv));

char* tempstring; tempstring = suffixes.ToNewCString(); 
mimeInfo->SetFileExtensions(tempstring); nsMemory::Free(tempstring);

SetFileExtensions takes a const char* :)
so try
mimeInfo->SetFileExtensions(NS_CONST_CAST(char*, suffixes.get()));

Comment 14

[Mike Kaply [:mkaply]]

I've done some thinking and I think I am going the wrong direction with this.

We need to externalize (IDL) the API that adds things to the MimeInfoDatabase.

Registering the plugins at startup only solves 1/4 of the problem. We still 
don't have MIME types for plugins registered later, helpers at startup, and 
helpers registered later.

So I will look at externalizing the API, and then we should add calls to add 
info to the mime database whenever we register a plugin or helper.

Comment 15

[Mike Kaply [:mkaply]]

Attachment: Much more elegant solution for the plugin side

Comment 16

[sean echevarria]

Nice - much more elegant.  Tested on Win2000.
FYI: On Windows, this solution gives priority to mimetypes found in the registry
over file extensions handled by plugins.  That is, if a plugin handles a given
file extension and there is a mimetype handler specified in the windows registry
for that file extension, the external app (as specified by the mimetype handler
entry) is used rather than the plugin within the browser.  No opinion on whether
or not this is correct/desired.

Comment 17

[timeless]

that's fine. As long as plugin mime types take priority over this registry 
stuff.

Comment 18

[sean echevarria]

Well it looks like the native OS registry is given priority over plugins.

On Windows, the way to confirm this is, in the registry, to modify the "Content
Type" value for a file extension that is handled by an installed plugin and then
click on a file for which you have an installed plugin.

1. If you change it to a value that is supported by a plugin, then the plugin
that supports that mimetype is invoked within the browser.  
2. If you remove it, then the plugin that supports the file extension is invoked
within the browser.  
3. If you change it to another mimetype that none of your plugins handle (even
though an installed plugin supports the file extension), then you get a dialog
asking whether to save the file or launch an app to handle it.

And again - this could be a good thing or a bad thing.  The first 2 cases are
the most important.

Comment 19

[Peter Lubczynski]

Sean/Michael/Timeless, what's the status of this bug?

I don't think I fully understand it or I think it's something else.

Comment 20

[sean echevarria]

I think Mike's patch is good and should go in.

Comment 21

[Mike Kaply [:mkaply]]

Attachment: Forgot to return error in one failure case.

Comment 22

[Peter Lubczynski]

This file is out of my domain. cc:ing mscott

Scott/Michael, do one of you want to take over this bug?

Comment 23

[Mike Kaply [:mkaply]]

I verified with mscotts latest helper fixes that this is still a problem.

The only thing  I don't like about my fix is that is gives an "nsiplugin not 
thread safe" assert.

Scott, could you take a look at the latest fix and see what you think?

Essentially if we don't get an extension for a mime type, we ask plugins.

Comment 24

[sean echevarria]

The patch for this bug doesn't address the problem anymore.  The unhandled 
content dialog is now displayed stating that mozilla can't view a file with a 
mime type that is handled by an installed plugin.  Can anyone else verify?

Comment 25

[Peter Lubczynski]

No, the test url works for me fine without any patch on Win32  tip.

Comment 26

[Mike Kaply [:mkaply]]

Win32 is the only platform this works on because it uses the system mime type 
registry.

Assuming you have the flash plugin installed.

To recreate this problem on Windows, open regedit and find .swf for instance.

Change .swf to .swt

The try to open a local .swf file.

Comment 27

[Peter Lubczynski]

Reassigning to Michael as it looks he has a grip on this and a fix in hand.

Chris, you may want to add this bug to your META plugin tracking one. This 
involves full-page plugins as well on both platforms.

Comment 28

[Peter Lubczynski]

Mike/Sean what is the status of this patch? This is a P1 blocker bug.

Comment 29

[Peter Lubczynski]

Nominating nsCatfood and getting more people on the radar

Comment 30

[sean echevarria]

Although the attached patch used to work great, it no longer works.  Clicking 
on http links to content handled by plugins or dragging local files handled by 
plugins into the browser causes the unhandled content dialog to appear.  The 
dialog correctly shows the mimetype of the content (as displayed in 
about:plugins) but states that it can't be displayed by mozilla.

Tested on mac and win (though on Win you need to hack the system registry to 
test this).

Comment 31

[Peter Lubczynski]

What is the status of this bug and how important is it?

Michael, is this something your patch can fix (with a little tweaking)?

Comment 32

[Mike Kaply [:mkaply]]

I'll look at this more for 0.9.1

Comment 33

[Arun Ranganathan]

nominating nsbeta1.  i'll also add this to the meta bug.  0.9.1!

Comment 34

[Marek Z. Jeziorek]

shouldn't this be made to work in 0.9.1?

Comment 35

[Peter Lubczynski]

Yes, this needs to work so one can open local PDF documents. I heard a rumor 
that this was already working?

Comment 36

[Peter Lubczynski]

But can't test due to blocker bug 78741.

Comment 37

[Peter Lubczynski]

Michael, do you think you can make mozilla 0.9.1? 

Who currently owns RCS file: 
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp

That's where this patch goes.

Comment 38

[Mike Kaply [:mkaply]]

I've heard rumblings that this patch doesn't actually work anymore.

With the Bidi and other things, I haven't had time to work on this. I am going 
to target it for 0.9.2 and do my best to get something happening with it.

Comment 39

[u29221]

I just tried this fix on my latest Windows build and it worked. The only 
problem with it is the annoying thread assertions.

Again, to recreate the problem on Windows, open a local .swf file - notice it 
works.

REGEDIT search on .swf change it to .swd, open the same local .swf file, it 
doesn't work.

Apply the patch - after getting a few assertions, it works.

Copying Bill Law for his opinion.

Comment 40

[Peter Lubczynski]

Michael, have you seen bug 84160? Could this be a dup or vice versa?

Comment 41

[u29221]

Nope, that one isn't a dupe.

The issue here is that the uriloader/exthandler code isn't smart enough to look 
at the extension/mimetype matchings it already knows about (plugins, helpers) 
when it loads a local file that doesn't have a mimetype. It relies on the OS 
(only Win and Mac support this)

This patch fixes the plugin case.

Comment 42

[Christopher Blizzard (:blizzard)]

So what's the deal with this bug?  Does Michael have the right answer or not?

Comment 43

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 44

[Scott MacGregor]

your change looks good to me Michael.
sr=mscott

Comment 45

[Peter Lubczynski]

r=peterl

Comment 46

[Mike Kaply [:mkaply]]

Hurrah.

Checked in.

I changed a !NS_FAILED to an NS_SUCCEEDED before checking in.

Done.

Comment 47

[rbs]

Note: adding this here to evangelize nicer coding practices.

+  if (NS_FAILED(rv)) {
+    /* Try the plugins */
+    const char* mimeType;
+    nsCOMPtr<nsIPluginHost> pluginHost (do_GetService(kPluginManagerCID, &rv));
+    if (NS_SUCCEEDED(rv)) {
+      if (pluginHost->IsPluginEnabledForExtension(aFileExt, mimeType) == NS_OK)
+      {
+        *aContentType = nsCRT::strdup(mimeType);
+        rv = NS_OK;
+        return rv;
+      }
+      else 
+      {
+        rv = NS_ERROR_FAILURE;
+      }
+    }
+  }
+  if (NS_FAILED(rv)) {
+    return rv;
+  } /* endif */
+

==== could be:

+  if (NS_FAILED(rv)) {
+    /* Try the plugins */
+    const char* mimeType;
+    nsCOMPtr<nsIPluginHost> pluginHost (do_GetService(kPluginManagerCID, &rv));
+    if (NS_SUCCEEDED(rv)) {
+      if (pluginHost->IsPluginEnabledForExtension(aFileExt, mimeType) == NS_OK)
+      {
+        *aContentType = nsCRT::strdup(mimeType);
+        return NS_OK;
+      }
+      return NS_ERROR_FAILURE;
+    }
+    return rv;
+  }

Comment 48

[shrirang khanzode]

this works now..verif on 1113 trunk builds on all plfs.

Comment 49

[Boris Zbarsky [:bzbarsky]]

*** Bug 53868 has been marked as a duplicate of this bug. ***