484579 - nsIMIMEService.getTypeFromExtension may fail unexpectedly on Windows when "Content Type" is empty in the registry

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[:Paolo Amadini]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090321 Minefield/3.6a1pre

On Windows, nsIMIMEService.getTypeFromExtension may fail unexpectedly,
returning NS_ERROR_NOT_INITIALIZED, when the "Content Type" value under
the extension's key in HKEY_CLASSES_ROOT is present, but empty.

Another prerequisite for reproducting the problem is that the ProgID
specified in the registry for the given extension must actually exists
and allow the system to find a helper application.

Reproducible: Always

Steps to Reproduce:
Run the attached test case with administrator privileges on the system.



The root of the problem lies in the nsOSHelperAppService::GetByExtension
function (in nsOSHelperAppService.cpp):

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/win/nsOSHelperAppService.cpp#537

The call to ReadStringValue may succeed, but return an empty string.
This empty string is passed to the nsMIMEInfoWin constructor, causing
an invalid nsMIMEInfoWin object to be generated. The object will survive
all the way up to nsExternalHelperAppService::GetTypeFromExtension, where
the call to GetMIMEType generates the error:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2570

Comment 1

[:Paolo Amadini]

Attachment: Test case (may fail if run without privileges)

This test case demonstrates the problem. I'm not sure about how to handle the
case where the test is run under a user account that has not enough privileges
to write the required registry values; in that case, the test cannot be executed,
but this condition does not necessarily indicate a failure of the feature under
test.

Comment 2

[:Paolo Amadini]

Note: While working on this bug, I also noticed that the two functions
GetExtensionFromWindowsMimeDatabase and GetExtensionFrom4xRegistryInfo
in "win/nsOSHelperAppService.cpp" may get called with an empty string
in their aMimeType parameters, but don't check for this case explicitly.
It seems that the two functions work because they try to read registry
keys or values that don't exist under normal conditions.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Shawn, Dan, thoughts?

Comment 4

[Shawn Wilsher :sdwilsh]

For what it is worth, I hate this code :(

So, I think the right thing to do here would be to have nsOSHelperAppService::GetByExtension fail if the string is also empty, so this:
537     if (NS_FAILED(regKey->ReadStringValue(NS_LITERAL_STRING("Content Type"),
538                   temp))) {
539       return nsnull; 
540     }
should be this:
537     if (NS_FAILED(regKey->ReadStringValue(NS_LITERAL_STRING("Content Type"),
538                   temp)) || temp.IsEmpty()) {
539       return nsnull; 
540     }

At least that will get us farther along.  I think it'll fix the issue too, but I'm not 100% sure of that.  This code is hard and painful.

Comment 5

[:Paolo Amadini]

Attachment: The fix [Checkin: Comment 19 & 28]

Thanks Shawn, here is the fix; it makes the test succeed as expected.

Any idea about the fact that the test case requires administrator
privileges on the system?

Comment 6

[Shawn Wilsher :sdwilsh]

So, you could make an xpcshell test case for this by making a mock object for nsIWindowsRegKey that returns the bogus value.  I'd actually like to see that done for this bug because this code is so hair.  If we ever re-write it, it'd be nice to have regression tests like this so these problems don't come up again in the future.

Comment 7

[:Paolo Amadini]

(In reply to comment #6)
> So, you could make an xpcshell test case for this by making a mock object for
> nsIWindowsRegKey that returns the bogus value.  I'd actually like to see that
> done for this bug because this code is so hair.  If we ever re-write it, it'd
> be nice to have regression tests like this so these problems don't come up
> again in the future.

I'll try that. This is another use case for bug 485205! I'm setting a
dependency just to track this fact.

Comment 8

[:Paolo Amadini]

Attachment: Test case

Here is the new test implementation. Actually, the openChild and createChild
functions are never called by the exercised code, so I've not had the
opportunity to test them. I think the two functions might as well be removed.

In particular I've not tested if this concatenation really works or is
missing some backslash check:

  key._relPath = this._relPath + aRelPath;

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 369723 [details] [diff] [review]
Test case

Test looks fine, but please use a descriptive filename and mention the bug number in a comment in the file.

Also, I don't think this is Mozilla Archive Format code as it says in the license block ;)

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]

r=sdwilsh on this, but I want robstrong to take a look at this too.  This may actually be a bug in our windows registry code.  He also mentioned that we should probably be using the windows native APIs as opposed to the XPCOM stuff.

Comment 11

[Robert Strong (they/them - no direct email)]

This isn't a bug in our windows registry code implementation. If a key value name for a string is defined and the value is an empty string it is suppose to return an empty string.

I am quite sure that the nsIWindowsRegKey implementation was added so the Extension Manager which is a JavaScript component could read the registry. The native api's for accomplishing the same thing are fairly trivial so if this is at all perf sensitive I would recommend just using the native api's.

(In reply to comment #5)
>...
> Any idea about the fact that the test case requires administrator
> privileges on the system?
HKEY_CLASSES_ROOT is a combination of HKEY_LOCAL_MACHINE\Software\classes and HKEY_CURRENT_USER\Software\classes with the entries under HKEY_CURRENT_USER winning out. I suspect that the .txt registry key only exists under HKEY_LOCAL_MACHINE\Software\classes so when you tried to write values under .txt you were actually writing under HKEY_LOCAL_MACHINE\Software\classes which requires administrator privileges. If you create the same keys under HKEY_CURRENT_USER\Software\classes it would probably work.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]

This should be fine.

btw: keep in mind that there is nothing enforcing the format of the strings for Content Type in the registry so it is possible though unlikely that this could return pretty much any random string.

Comment 13

[:Paolo Amadini]

Attachment: Test case (generic multiple-tests-per-file version)

(In reply to comment #9)
> (From update of attachment 369723 [details] [diff] [review])
> Test looks fine, but please use a descriptive filename and mention the bug
> number in a comment in the file.

I renamed the test using the generic name "test_osHandlerService.js", and
mentioned the bug number and URL before the specific test case. I also wrapped
the windows-specific test case in the isOnWindows() call, to allow more tests
to be added at the end of the file.

> Also, I don't think this is Mozilla Archive Format code as it says in the
> license block ;)

Fixed :-)

Comment 14

[:Paolo Amadini]

(In reply to comment #11)
> I am quite sure that the nsIWindowsRegKey implementation was added so the
> Extension Manager which is a JavaScript component could read the registry. The
> native api's for accomplishing the same thing are fairly trivial so if this is
> at all perf sensitive I would recommend just using the native api's.

I don't think performance would be affected in any way by using the XPCOM
registry components. Now we also use mock implementations of the same
interfaces to ease automated testing; that solution wouldn't be viable
if the code used Windows API calls. In the end, I think using the XPCOM
components in native code too is the right way to go.

> (In reply to comment #5)
> >...
> > Any idea about the fact that the test case requires administrator
> > privileges on the system?
> HKEY_CLASSES_ROOT is a combination of HKEY_LOCAL_MACHINE\Software\classes and
> HKEY_CURRENT_USER\Software\classes with the entries under HKEY_CURRENT_USER
> winning out. I suspect that the .txt registry key only exists under
> HKEY_LOCAL_MACHINE\Software\classes so when you tried to write values under
> .txt you were actually writing under HKEY_LOCAL_MACHINE\Software\classes which
> requires administrator privileges. If you create the same keys under
> HKEY_CURRENT_USER\Software\classes it would probably work.

Thanks for the tip! At present we don't need to write anything to the registry
anymore, since we are using the mock objects, but I'll keep it in mind since
it may be needed for more advanced test cases.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> I renamed the test using the generic name "test_osHandlerService.js", and
> mentioned the bug number and URL before the specific test case. I also wrapped
> the windows-specific test case in the isOnWindows() call, to allow more tests
> to be added at the end of the file.
I actually prefer a test file with a specific file name for this bug.  Big test files are generally a pain to debug when they fail, so small, simple, and easy to understand tests are a big win (IMO).

Comment 16

[:Paolo Amadini]

(In reply to comment #15)
> (In reply to comment #13)
> > I renamed the test using the generic name "test_osHandlerService.js", and
> > mentioned the bug number and URL before the specific test case. I also wrapped
> > the windows-specific test case in the isOnWindows() call, to allow more tests
> > to be added at the end of the file.
> I actually prefer a test file with a specific file name for this bug.  Big test
> files are generally a pain to debug when they fail, so small, simple, and easy
> to understand tests are a big win (IMO).

Well, I agree that one-test-per-file may be more appropriate in this case,
with the xpcshell testing framework. But if the only thing you'd like me to
do is to rename the file, please be more specific; it's better than me trying
to guess what you have in mind :-) Actually, I can't think of any filename
that suggests that we're testing that "nsIMIMEService.getTypeFromExtension
may fail unexpectedly on Windows when "Content Type" is empty in the registry",
and nothing else, while being reasonably short. I'm fine with whatever name
you can come up with.

Comment 17

[Shawn Wilsher :sdwilsh]

I actually don't care if it's short.  It's a pain trying to find the right test file in a list of a whole bunch (especially when a lot of the test files are just bug numbers).

How about test_getTypeFromExtension_with_empty_Content_Type.js?

Comment 18

[:Paolo Amadini]

Attachment: Test case [Checkin: Comment 20]

(In reply to comment #17)
> How about test_getTypeFromExtension_with_empty_Content_Type.js?

Here you go :-)

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]


http://hg.mozilla.org/mozilla-central/rev/d355205de080

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 371272 [details] [diff] [review]
Test case
[Checkin: Comment 20]


http://hg.mozilla.org/mozilla-central/rev/93bc0760d0d2

Comment 21

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239905472.1239917703.3668.gz&fulltext=1
Linux mozilla-central unit test on 2009/04/16 11:11:12

Leaked: 3328
}

Didn't this test leak when you tested it?

Comment 22

[Shawn Wilsher :sdwilsh]

There is no reason to re-open this bug because of a unit test leak (since they don't prevent it from landing yet).  You should file a new bug.

Also, I don't think leak testing for xpcshell tests was turned on yet when this patch was written.

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> There is no reason to re-open this bug because of a unit test leak (since they
> don't prevent it from landing yet).  You should file a new bug.

Right now, I can't enable the leak _error_ reporting because there are still leaking tests. (and no "ignore" feature yet...)
(Silently) Adding new leaking tests certainly doesn't help there!

I filed bug 489078.

> Also, I don't think leak testing for xpcshell tests was turned on yet when this
> patch was written.

I agree the 2 former attachments were done before it was turned on (Mar 27 11:00:10 2009 -0700).
But:
It was turned on when these patches were landed.
It was turned on when checkin-needed was requested.
It was turned on when the 3 latter attachments were done.
The feature has even been available to developers _long_ before that.

Then:
*I didn't see Paolo tell that his test was leaking nor file a (follow-up) bug about it.
*This still doesn't answer the question whether this leak is a recent regression or was expected to be there.

Comment 24

[:Paolo Amadini]

(In reply to comment #23)
> (Silently) Adding new leaking tests certainly doesn't help there!

This leak existed, undetected, before this patch landed.

Comment 25

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

Sorry, I can't figure out what drivers are being asked here.

 - why do we want this? why do we need it on branch?
 - do we know if it leaks?

Comment 26

[Shawn Wilsher :sdwilsh]

(In reply to comment #25)
>  - why do we want this? why do we need it on branch?
If someone has a registry entry that is set to an empty string, we throw up our arms in disgust pretty much.  This patch makes us fail more gracefully, and should result in a better user experience for users in this situation.

>  - do we know if it leaks?
The xpcshell tests leak, but we don't track those.  The leaks are fixed in bug 489078, with the exception of a small leak which was WONTFIXed (bug 203794)

Comment 27

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

Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]

a191=beltzner, please make sure to land appropriate tests, as well!

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/393022d4bdc9

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 371272 [details] [diff] [review]
Test case
[Checkin: Comment 20]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/746f21b74a8d

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 371272 [details] [diff] [review]
Test case
[Checkin: Comment 20]


(In reply to comment #29)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/746f21b74a8d

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a4388577ddd2
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bcfec86579c1
Backed out changeset: 746f21b74a8d, because
it fails with "TypeError: i is undefined",
which I don't understand right away and I've got to go :-/

Linux and MacOSX failed, Windows passed:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1240418432.1240426209.4546.gz
Linux mozilla-1.9.1 unit test on 2009/04/22 09:40:32
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1240419106.1240422192.28968.gz
OS X 10.5.2 mozilla-1.9.1 unit test on 2009/04/22 09:51:46
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1240419772.1240425458.3296.gz
Linux comm-central unit test on 2009/04/22 10:02:52
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1240422528.1240428881.8692.gz
OS X 10.4 comm-central unit test on 2009/04/22 10:48:48
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.0/1240419145.1240420811.26466.gz
Linux comm-1.9.1 check on 2009/04/22 09:52:25
(No TB/OSX build for this changeset)

TypeError: i is undefined

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1240418432.1240428118.7619.gz&fulltext=1
WINNT 5.2 mozilla-1.9.1 unit test on 2009/04/22 09:40:32
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1240422384.1240428405.8027.gz
WINNT 5.2 comm-central unit test on 2009/04/22 10:46:24
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.0/1240423780.1240429635.9846.gz&fulltext=1
Win2k3 comm-1.9.1 check on 2009/04/22 11:09:40

(green)
}

Comment 31

[:Paolo Amadini]

> it fails with "TypeError: i is undefined",

I guess it's because of bug 477735. Can you please see that it's backported
to 1.9.1? Thanks!

Comment 32

[:Gavin Sharp [email: [e-mail]]]

nsIWindowsRegKey only exists on Windows, so the test should only be running on Windows.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Oh, I see, it already returns early on non-Windows, but not before calling generateQI. I suppose we could just land bug 477735, but just returning before the MockWindowsRegKey.prototype definition is probably a more expedient fix.

Comment 34

[:Paolo Amadini]

Attachment: Modification to make the test work without the fix for bug 477735 [Checkin: Comment 37]

This patch, to be applied on top of attachment 371272 [details] [diff] [review], should make the test work
on the 1.9.1 branch too, without the fix for bug 477735. It's required only on
the branch, but it doesn't hurt to check it in on trunk too, to keep the code
aligned (it also makes the test theoretically more efficient).

In any case, if someone has the opportunity to test this on the branch before
the check-in, it would be interesting to observe if the patch for bug 477735
applies cleanly and actually solves the problem before this change is made.

Comment 35

[Shawn Wilsher :sdwilsh]

Comment on attachment 374654 [details] [diff] [review]
Modification to make the test work without the fix for bug 477735
[Checkin: Comment 37]

r=sdwilsh

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 374654 [details] [diff] [review]
Modification to make the test work without the fix for bug 477735
[Checkin: Comment 37]

sr=me; please add the checkin-needed keyword as needed (and put exactly what needs to be landed in the status whiteboard)?

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 374654 [details] [diff] [review]
Modification to make the test work without the fix for bug 477735
[Checkin: Comment 37]


http://hg.mozilla.org/mozilla-central/rev/866ea0c4e7a8

Comment 38

[:Paolo Amadini]

Attachment: Full patch to align the 1.9.1 branch with trunk

Strangely enough, I didn't realize this bug was waiting for a patch that
combined the two that were made for trunk, before being able to land on
the 1.9.1 branch, even though now I see it's stated in the whiteboard.
Sorry for the long wait.

Here is the patch, which is simply a copy of the file in trunk.

I guess there's no need to request approval on the attachment since it's
been already granted for the original patch.

Comment 39

[:Paolo Amadini]

Checkin-needed for "Full patch to align the 1.9.1 branch with trunk".

Comment 40

[:Paolo Amadini]

Note: bug 508030 needs checkin in the same branch and folder (in fact, I
noticed this bug was stuck while updating that one for checkin-needed).

Comment 41

[Boris Zbarsky [:bzbarsky]]

Pushed test as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c2d9a3ca71a6