416466 - Extra useless option in file handling options

Status: VERIFIED FIXED

(Firefox :: File Handling, defect)

Description

[Hussam Al-Tayeb]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020901 Firefox/3.0b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020901 Firefox/3.0b4pre

There is a extra 'use' option in the options for programs for file handling.
This was happening in my previous profile with all file contents.

I made a new profile and added one file type (pdf) and the issue is still there.
I made a screenshot and will attach it.
I think the issue it that firefox thinks 'firefox' is one of the applications in the list.

Reproducible: Always

Comment 1

[Hussam Al-Tayeb]

Attachment: extra 'use' entry.

notice the extra 'use' entry in the screenshot.

Comment 2

[Marcia Knous [:marcia]]

Confirmed using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020804 Minefield/3.0b4pre. I reproduced by just downloading a PDF file.

Comment 3

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

Do you have Acrobat Reader or any other PDF viewing software on your system besides Document Viewer (a.k.a. evince, the default GNOME PDF viewing app), and which application did you select when downloading the PDF file?  I'm just wondering if maybe Firefox has stored a custom entry for the default app or perhaps for another app.

Comment 4

[Marcia Knous [:marcia]]

Myk: I tested in a fairly clean Ubuntu VM, and I checked and do not see Acrobat reader installed. This VM has open office installed. Now that I am trying to reproduce, I am getting an unknown file error. Will revert my VM snapshot and try again. 

Comment 5

[Hussam Al-Tayeb]

No, the only pdf viewer installed is evince and it is the only program that can open this type of file. I even verified using /usr/share/applications/mimeinfo.cache

If I choose the 'use' entry and click on a link to a pdf file, it automatically downloads it and I get this error when the download completes.

/tmp/sports.pdf could not be opened, because an unknown error occurred.

Try saving to disk first and then opening the file.

Comment 6

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

Thanks for the info!  Can you also attach the mimeTypes.rdf file from one of the testing profiles that exhibits this problem?  Don't attach the one from your regular profile, as it may contain information about what kinds of files you download, which you may not want to reveal publicly (unless you look at it first and are ok revealing the information in it).

Comment 7

[Florian Quèze [:florian]]

Attachment: patch v1

This patch fixes it for me on Ubuntu 7.04, aExecutable.leafName is actually "firefox" (when stepping through the code with Venkman).

Myk, do you remember cases with the leafName set to "firefox-bin"?  If in doubt we should probable check for both...

Comment 8

[Hussam Al-Tayeb]

Attachment: mimeTypes.rdf file

Attached mimeTypes.rdf file

Comment 9

[Florian Quèze [:florian]]

I've just seen that this code is duplicated in the _chooseClientApp method of FeedWriter.js, if we fix it this way, it should be fixed there too.

Comment 10

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

Comment on attachment 302652 [details] [diff] [review]
patch v1

(In reply to comment #7)

> Myk, do you remember cases with the leafName set to "firefox-bin"?  If in doubt
> we should probable check for both...

(In reply to comment #9)
> I've just seen that this code is duplicated in the _chooseClientApp method of
> FeedWriter.js, if we fix it this way, it should be fixed there too.

Yeah, I copied the code from FeedWriter.js.  But the issue isn't the name.  The problem is that in the mimeTypes.rdf file the "preferred application" record for PDF files is pointing to the Firefox installation directory, which isn't an application, much less a preferred one (and in my case it's called "firefox-trunk", but it can be an arbitrary name, so no name checking will catch this case).

So the issue here is that corrupt information is being written to mimeTypes.rdf.

What I see happening is this:

In a fresh profile, mimeTypes.rdf contains no info about PDF.

If I click on a PDF link and select "Save File" or "Open in Application" but don't check "Do this from now on...", mimeTypes.rdf does not change.

If I select either of those options and do check "Do this from now on", mimeTypes.rdf gets updated with info about PDF handling, and most of the various records added seem fine, but the record for the handler app incorrectly contains blank "prettyName" and "path" fields:

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

At this point the wierd "Use " entry appears in the Applications prefpane.  If I select it, mimeTypes.rdf gets updated, and that record changes to point to the Firefox installation directory:

  <RDF:Description RDF:about="urn:mimetype:externalApplication:application/pdf"
                   NC:prettyName="firefox-trunk"
                   NC:path="/home/myk/Applications/firefox-trunk" />

I think the bug is in the file handling dialog.  For some reason it writes a preferred application ("externalApplication") record to the RDF file but leaves prettyName and path blank.  It should be writing the name of the app and its path in those fields when you select an application and check "Do this from now on".

And when you select "Save File" and check "Do this from now on" it shouldn't write any preferred application record to mimeTypes.rdf at all.

A secondary problem is that the Applications prefpane then treats this empty path like a pointer to the current working directory, and when you change the configuration it writes that back to the datastore (and also uses the "leafName" as the "prettyName" of the app).

But that's a minor issue and not necessarily worth addressing, since it wouldn't happen if the datastore never got corrupted in the first place.

Comment 11

[Hussam Al-Tayeb]

(In reply to comment #10)
> But that's a minor issue and not necessarily worth addressing, since it
> wouldn't happen if the datastore never got corrupted in the first place.
> 

Yes, but there is a 100% chance the datastore will get corrupted with each new additional file association.

Comment 12

[Michael Ventnor]

I'm still wondering why corrupt data is written to mimeTypes.rdf, however a very easy fix for this bug would be to add a null-check for getDisplayNameForFile() in _isValidHandlerExecutable, as these null entries are directories anyway.

Comment 13

[Michael Ventnor]

...that is, if you're willing to take that fix.

Comment 14

[Michael Ventnor]

Attachment: Unassert blank properties

This fixes it for me (it doesn't remove existing blank apps but it stops new ones from appearing). It does this by not writing blank properties to the RDF, which it does for app name and path (because we select the system default, which is its own property in itself). To me this seems like the right fix because it stops the corrupt data from being written in the first place (when I checked my mimeTypes.rdf).

Also, I wish swift death upon RDF. And its API, too. I hope you're still handing out presents to fixers of polish-hard bugs, Alex :)

Comment 15

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

Comment on attachment 351237 [details] [diff] [review]
Unassert blank properties

Sorry for the delay getting to this review.

This patch does fix the problem, but it also regresses behavior.

Without the patch, loading the PDF document I use for testing <http://www.irs.gov/pub/irs-pdf/fw4.pdf>, selecting "Open with [Document Viewer (default)]" in the Unknown Content Type dialog, checking "Do this automatically for files like this from now on." in that dialog, and pressing its OK button causes the Applications prefpane to display "Use Document Viewer (default)" as the action for the "PDF document" content type, and loading the PDF document again automatically opens it again in Document Viewer, as it should.

With the patch, however, the Applications prefpane continues to display "Always Ask" as the action for that content type, and loading the PDF document again prompts me with the Unknown Content Type dialog instead of automatically opening the file in Document Viewer.

I'm not very familiar with the content of helperApps.js.  It's the one piece of code that still touches the mimeTypes.rdf datastore directly instead of going through the new nsIHandlerService API, which all other code uses, and which abstracts details of the datastore's RDF format (so we can replace it with something better in the future), as I ran out of time in the 3.0 cycle to rewrite that last file.

But if I had to take a guess, I'd say the problem is that changeMIMEStuff is being used to assert a bunch of other properties, some of which need to be asserted even if their values are empty strings, and the check you're doing would need to be hoisted up the call chain into the appDisplayName and appPath setters (or possibly further).

Comment 16

[Michael Ventnor]

Attachment: Patch 2

I don't think I experienced that, but I moved up the unasserting code.

Comment 17

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

Comment on attachment 352230 [details] [diff] [review]
Patch 2

>   set appDisplayName(aDisplayName)
>   {
>+    if (!aDisplayName) {
>+      var currentValue = this.getLiteralForContentType(APP_URI(this.mimeType), "prettyName");
>+      this.unassertMIMEStuff(APP_URI(this.mimeType), "prettyName", currentValue);
>+      return aDisplayName;
>+    }
>+
>     this.changeMIMEStuff(APP_URI(this.mimeType), "prettyName", aDisplayName);
>     return aDisplayName;
>   },

Nit: this would be a bit less duplicative as:

  if (aDisplayName)
    this.changeMIMEStuff(...);
  else {
   var currentValue ...
   this.unassertMIMEStuff(...);
  }

  return aDisplayName;

Otherwise this looks great, fixes the problem, and doesn't cause the regression I experienced with the first patch.

Comment 18

[Michael Ventnor]

Attachment: Patch 3

Fix the nit.

Comment 19

[Shawn Wilsher :sdwilsh]

myk's review is sufficient here.

Comment 20

[Michael Ventnor]

Ok, thanks.

Comment 21

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/070850731d2d

Comment 22

[Michael Ventnor]

Comment on attachment 352360 [details] [diff] [review]
Patch 3

This would be good to get into 1.9.1 as the fix is not very risky and the effect is noticeable.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352360 [details] [diff] [review]
Patch 3

a191=beltzner

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/84c152f2aa9a

Comment 25

[Marcia Knous [:marcia]]

Verified fixed on the trunk using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre.

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.

This bug is easily identifiable, but was in a menu off of a secondary interface, so isn't constantly encountered.