Closed Bug 484579 Opened 11 years ago Closed 11 years ago

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

Categories

(Core Graveyard :: File Handling, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b4])

Attachments

(4 files, 3 obsolete files)

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
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Shawn, Dan, thoughts?
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.
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?
Attachment #369681 - Flags: review?(bzbarsky)
Attachment #369681 - Flags: superreview+
Attachment #369681 - Flags: review?(sdwilsh)
Attachment #369681 - Flags: review?(bzbarsky)
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.
(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.
Depends on: 485205
Attached patch Test case (obsolete) — Splinter Review
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;
Attachment #368702 - Attachment is obsolete: true
Attachment #369723 - Flags: review?(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 ;)
Attachment #369723 - Flags: review?(sdwilsh) → review+
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.
Attachment #369681 - Flags: review?(sdwilsh)
Attachment #369681 - Flags: review?(robert.bugzilla)
Attachment #369681 - Flags: review+
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 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.
Attachment #369681 - Flags: review?(robert.bugzilla) → review+
(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 :-)
Attachment #369723 - Attachment is obsolete: true
(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.
(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).
(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.
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?
(In reply to comment #17)
> How about test_getTypeFromExtension_with_empty_Content_Type.js?

Here you go :-)
Attachment #370470 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #369681 - Attachment description: The fix → The fix [Checkin: Comment 19]
Comment on attachment 371272 [details] [diff] [review]
Test case
[Checkin: Comment 20]


http://hg.mozilla.org/mozilla-central/rev/93bc0760d0d2
Attachment #371272 - Attachment description: Test case → Test case [Checkin: Comment 20]
Attachment #369681 - Flags: approval1.9.1?
Assignee: nobody → paolo.02.prg
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing: needs approval]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
{
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?
Blocks: mlkTests
Status: RESOLVED → REOPENED
Depends on: 469523
Resolution: FIXED → ---
Whiteboard: [needs 1.9.1 landing: needs approval]
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 489078
(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.
No longer blocks: mlkTests
No longer depends on: 469523
(In reply to comment #23)
> (Silently) Adding new leaking tests certainly doesn't help there!

This leak existed, undetected, before this patch landed.
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?
(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)
Attachment #369681 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]

a191=beltzner, please make sure to land appropriate tests, as well!
Keywords: checkin-needed
Comment on attachment 369681 [details] [diff] [review]
The fix
[Checkin: Comment 19 & 28]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/393022d4bdc9
Attachment #369681 - Attachment description: The fix [Checkin: Comment 19] → The fix [Checkin: Comment 19 & 28]
Comment on attachment 371272 [details] [diff] [review]
Test case
[Checkin: Comment 20]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/746f21b74a8d
Attachment #371272 - Attachment description: Test case [Checkin: Comment 20] → Test case [Checkin: Comment 20 & 29]
Whiteboard: [fixed1.9.1b4]
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)
}
Attachment #371272 - Attachment description: Test case [Checkin: Comment 20 & 29] → Test case [Checkin: Comment 20]
Whiteboard: [fixed1.9.1b4] → [needs 1.9.1 landing: needs "fixed" test] [fixed1.9.1b4]
> 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!
nsIWindowsRegKey only exists on Windows, so the test should only be running on Windows.
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.
Depends on: 477735
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.
Attachment #374654 - Flags: superreview?(bzbarsky)
Attachment #374654 - Flags: review?(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
Attachment #374654 - Flags: review?(sdwilsh) → review+
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)?
Attachment #374654 - Flags: superreview?(bzbarsky) → superreview+
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing: needs "fixed" test] [fixed1.9.1b4] → [c-n: new test diff to trunk] [needs 1.9.1 landing: needs "fixed" test] [fixed1.9.1b4]
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
Attachment #374654 - Attachment description: Modification to make the test work without the fix for bug 477735 → Modification to make the test work without the fix for bug 477735 [Checkin: Comment 37]
Keywords: checkin-needed
Whiteboard: [c-n: new test diff to trunk] [needs 1.9.1 landing: needs "fixed" test] [fixed1.9.1b4] → [needs 1.9.1 landing: needs full test patch] [fixed1.9.1b4]
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.
Checkin-needed for "Full patch to align the 1.9.1 branch with trunk".
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing: needs full test patch] [fixed1.9.1b4] → [needs 1.9.1 landing] [fixed1.9.1b4]
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).
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing] [fixed1.9.1b4] → [fixed1.9.1b4]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.