444477 - Spurious "Use" entries in external application selection preference panel

Status: RESOLVED WORKSFORME

(Core Graveyard :: File Handling, defect)

Description

[Mike Hommey [:glandium]]

Attachment: patch

This is very reproducible under linux. I haven't verified on windows.

- Remove mimeType.rdf in the profile (to ensure that the RDF resource for the mime type that we're going to configure doesn't already exist)
- Launch firefox
- Go to a page where a pdf can be downloaded
- In the selection dialog, select "Save as", check "Do this automatically...", and click on OK
- Do whatever you want with the file selection dialog, it's not important (Cancel is fine)

Now, if you check the mimeTypes.rdf file, you can see this:

<RDF:Description RDF:about="urn:mimetype:externalApplication:application/pdf"
                   NC:prettyName=""
                   NC:path="" />

Now, the problem is that the code is actually using this empty path, because it exists, to fill preferred and possible applications, which results in the preference panel to show a "Use" entry between "Use (default application)" and "Use other..."

There are two possible ways to fix this:
- avoid having this path set
- avoid having an empty possible application being set (actually, the possible application is the application directory because of the fail path in _getFileWithPath, but its display name is empty because of the trailing /, i think)

The latter should be better, because going with the former won't fix the problem with already borked mimeTypes.rdf files in user profiles.

Attached patch just does this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Mike, you probably want dmose or sdwilsh to review this.

Comment 2

[Shawn Wilsher :sdwilsh]

Hey, this looks like an issue I hit once and couldn't figure out how I reproduced it...

I'm not technically a peer in the code in question, but I think myk wrote it, so he's probably best here.

It'd be great to see an xpcshell test harness written for this as well.

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> It'd be great to see an xpcshell test harness written for this as well.

If you need me to do this, I will need pointers as to how to do it, and what you want to be tested.
Please also decide who should be reviewing this ;)

Comment 4

[Mike Hommey [:glandium]]

Attachment: test for testsuite

I could find a way to get this tested. The test properly fails with current source and passes with patch applied.

Note this patch conflicts with patch implementing the test for bug 442629, but only context lines are concerned.

Now, who should be reviewing these 2 patches here ?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 328786 [details] [diff] [review]
patch

This patch is ok, as far as it goes, but there's an additional problem.  It's possible to select that invalid "Use" action in the Applications prefpane, and that updates the record for the external application with the name and path of the current directory, f.e.:

  <RDF:Description RDF:about="urn:mimetype:externalApplication:application/pdf"
                   NC:prettyName="bin"
                   NC:path="/home/myk/Mozilla/builds/minefield-opt/dist/bin" />

Clicking a link to a PDF file then triggers an alert dialog: "/tmp/Movable_Type_Cheatsheet_v1.pdf could not be opened, because an unknown error occurred.  Try saving to disk first and then opening the file."

I've actually had this problem with my regular profile, although I'm not sure how I got into the state where "Use" was the selected action, as I don't recall ever selecting it manually.

The current patch doesn't handle this situation, since it only checks for a path equal to the empty string, which is not the case anymore.

Strangely, this problem shouldn't be happening at all, since the Applications prefpane checks to see if handler apps are valid executables before adding them to the list of handlers via its _isValidHandlerExecutable function <http://mxr.mozilla.org/mozilla/source/browser/components/preferences/applications.js#1348>.  Perhaps that function returns true in this case because the directory is marked "executable"?

I think the patch should fix whatever is wrong with _isValidHandlerExecutable, then copy that function to nsHandlerService.js and use it to check the return value of getFileWithPath (which is a redundant check for the prefpane but not for other consumers of the handler service).

Even better would be to additionally figure out why these corrupt entries are getting added to the datasource in the first place, but we can fix that as a separate bug.

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> (From update of attachment 328786 [details] [diff] [review])
> This patch is ok, as far as it goes, but there's an additional problem.  It's
> possible to select that invalid "Use" action in the Applications prefpane, and
> that updates the record for the external application with the name and path of
> the current directory, f.e.:
> 
>   <RDF:Description RDF:about="urn:mimetype:externalApplication:application/pdf"
>                    NC:prettyName="bin"
>                    NC:path="/home/myk/Mozilla/builds/minefield-opt/dist/bin" />

Instead of current directory, I would say application directory, because of
http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsHandlerService.js#616
(iirc, XCurProcD is MOZILLA_FIVE_HOME)
 
> Clicking a link to a PDF file then triggers an alert dialog:
> "/tmp/Movable_Type_Cheatsheet_v1.pdf could not be opened, because an unknown
> error occurred.  Try saving to disk first and then opening the file."
> 
> I've actually had this problem with my regular profile, although I'm not sure
> how I got into the state where "Use" was the selected action, as I don't recall
> ever selecting it manually.
> 
> The current patch doesn't handle this situation, since it only checks for a
> path equal to the empty string, which is not the case anymore.
> 
> Strangely, this problem shouldn't be happening at all, since the Applications
> prefpane checks to see if handler apps are valid executables before adding them
> to the list of handlers via its _isValidHandlerExecutable function
> <http://mxr.mozilla.org/mozilla/source/browser/components/preferences/applications.js#1348>.
>  Perhaps that function returns true in this case because the directory is
> marked "executable"?

Yes, a directory is executable. Adding a test to check whether it's a regular file should protect better.
 
> I think the patch should fix whatever is wrong with _isValidHandlerExecutable,
> then copy that function to nsHandlerService.js and use it to check the return
> value of getFileWithPath (which is a redundant check for the prefpane but not
> for other consumers of the handler service).
> 
> Even better would be to additionally figure out why these corrupt entries are
> getting added to the datasource in the first place, but we can fix that as a
> separate bug.

See my link to nsHandlerService.js above, that should be the root cause.

Comment 7

[Mike Hommey [:glandium]]

Attachment: patch v2

If I'm not mistaken, this should fix all issues, including already corrupted mimeTypes.rdf. The unit test also tries to check all possible brokenness.

Note that other tests were using a directory as handler file, which was not a good idea, especially now the issue is fixed with the rest of the patch. So, just use run-mozilla.sh that we know is in XCurProcD.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 329140 [details] [diff] [review]
patch v2

>+  var executable = HandlerServiceTest._dirSvc.get("XCurProcD", Ci.nsIFile);
>+  executable.append("run-mozilla.sh");

This will fail on windows... Any better idea?

Comment 9

[Mike Hommey [:glandium]]

Attachment: patch v3

This one should work everywhere. Note it is against mozilla-central+fix for unit test in bug 442629

Comment 10

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

Shouldn't you also make sure that the empty path doesn't get written to the RDF DS?

Comment 11

[Mike Hommey [:glandium]]

I can't find where I saw that (don't have time right now) but it looked like this was done on purpose.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 329222 [details] [diff] [review]
patch v3

This looks great!  As biesi notes, it'd be nice if the patch also fixed whatever code is adding the invalid record to the datasource in the first place (I bet it's in helperApps.js, which is the only code left in Firefox that is still accessing mimeTypes.rdf directly rather than through the handler service).  But I think it's ok to address that as a separate bug.

The one remaining problem here is that downloading the file remains broken if the invalid "Use" action is the currently selected action, but it doesn't appear broken when the user opens the prefpane, since the prefpane no longer shows that action (my prefpane shows "Use Document Viewer (default)" selected).

Selecting anything from the Actions menu fixes that, but it's not apparent why it's broken, given that a valid item is selected, and that could confuse some users who are in the broken state and trying to figure out how to get out of it.

The ideal solution to that would be to detect the invalid app when the user tries to download the file (and when constructing the Actions menu) and automagically revert the user to Always Ask in that case.  A minimal fix, however, would be to make rebuildActionsMenu display the app in the Actions menu, even though it is invalid, if it's the currently selected action (giving the user a chance to see that it is invalid and change it).

Note: the last portion of the patch didn't apply because of a conflict, but it was easy to work around it.


>diff --git a/uriloader/exthandler/nsHandlerService.js b/uriloader/exthandler/nsHandlerService.js

>+    if (file && file.exists() &&
>+        file.isExecutable() &&
>+        file.isFile())
>+      return file;

Nit: I don't think you need to check for |file| to be defined here, as it must be defined by this point (even if the file it references doesn't exist), if I'm reading the code above it correctly.

Comment 13

[Mike Hommey [:glandium]]

I haven't had enough time to check it thouroughly, but in the meanwhile I found where the empty entries are added:

if (!isNewMIMEType) {
  // Creating a new entry, set path.
  entry.appPath = "";
} 

in toolkit/mozapps/downloads/content/helperApps.js.
Unfortunately, blame on this file is not verbose enough, because the file was copied to toolkit in 2003 with this code already in. Do you have an idea where it was before ?

The only remaining broken case I can see is, as you say, if the broken entry was selected, in which case a failure dialog opens. A half broken case can also happen when you select Always ask after that, in which case the helper dialog will have an empty application name in its selection.

Where does the code handling the automatic download lie ? in helperApps.js ?

Comment 14

[Mike Hommey [:glandium]]

> >diff --git a/uriloader/exthandler/nsHandlerService.js b/uriloader/exthandler/nsHandlerService.js
> 
> >+    if (file && file.exists() &&
> >+        file.isExecutable() &&
> >+        file.isFile())
> >+      return file;
> 
> Nit: I don't think you need to check for |file| to be defined here, as it must
> be defined by this point (even if the file it references doesn't exist), if I'm
> reading the code above it correctly.

It depends whether
file = this._dirSvc.get("XCurProcD", Ci.nsIFile);

can return something empty or not.

Comment 15

[Shawn Wilsher :sdwilsh]

Automatic download code is in http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> in toolkit/mozapps/downloads/content/helperApps.js.
> Unfortunately, blame on this file is not verbose enough, because the file was
> copied to toolkit in 2003 with this code already in. Do you have an idea where
> it was before ?

Nice catch!  Unfortunately I don't know where that file used to live.  I asked around on IRC, but I didn't get a response there.


> The only remaining broken case I can see is, as you say, if the broken entry
> was selected, in which case a failure dialog opens. A half broken case can also
> happen when you select Always ask after that, in which case the helper dialog
> will have an empty application name in its selection.

Ah, right!


> Where does the code handling the automatic download lie ? in helperApps.js ?

I think the code that retrieves the nsIHandlerInfo object from the handler service is in nsExternalHelperAppService::GetFromTypeAndExtension.  But I'm not sure where the request originates or what actually uses that object to decide what to do with the request.

dmose might know.  Also cc:ing Florian, who might know.  And cc:ing myself so I see the comments posted to this bug.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

http://mxr.mozilla.org/mozilla/source/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js is where the toolkit version was forked from.  I'm not sure about the automatic download code, but I bet sdwilsh knows...

Comment 18

[Shawn Wilsher :sdwilsh]

What is meant by "automatic download code"?

Comment 19

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

Comment on attachment 329222 [details] [diff] [review]
patch v3

clearing review request, as this has r-

Comment 20

[Mike Hommey [:glandium]]

I think this can't happen anymore.