Closed Bug 428600 Opened 12 years ago Closed 12 years ago

console spam from nsIHandlerService

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9

People

(Reporter: marcia, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

Seen using  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041107 Minefield/3.0pre

STR:
I get this error in the error console today: Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::fillHandlerInfo]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]

I also get Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no], but this seems to be covered by Bug 395228
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041201 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

1. Load <http://download.openoffice.org/other.html#en-US>.
2. Click on a "Download" link.

{{
Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::fillHandlerInfo]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::fillHandlerInfo]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///.../seamonkey/components/nsHelperAppDlg.js :: anonymous :: line 297"  data: no]
Source File: file:///.../seamonkey/components/nsHelperAppDlg.js
Line: 297

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///.../seamonkey/components/nsHelperAppDlg.js :: anonymous :: line 297"  data: no]
Source File: file:///.../seamonkey/components/nsHelperAppDlg.js
Line: 297
}}

Furthermore, if I click on a link with "unknown" type,
like a MacOSX .dmg file, whereas I'm on Windows,
I get the other exception, between the two first:
{{
Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]
}}
I don't know if it's bug 395228 <about:> case,
or only the same error with a different cause.
(Adding dependency, for the time being.)
Blocks: 428537
Component: General → File Handling
Depends on: 395228
QA Contact: general → file-handling
See another case in bug 395228 comment 4 too.
Flags: blocking1.9?
Does the openoffice download site still function?  What's the user experience like?  If it's only a log message, wouldn't block.  Please explain, and re-nom once more info is available on why we should block.
Flags: blocking1.9? → blocking1.9-
The user experience is not that good, at least in my case.  It always happens with torrent files with me.  The download manager displays "checking for viruses" for a few seconds(4-10) or I get a not responding Firefox windows.  It seems to be checking for viruses even after the torrent has been passed to the bit torrent client.
(In reply to comment #3)
> Does the openoffice download site still function?  What's the user experience
> like?  If it's only a log message, wouldn't block.  Please explain, and re-nom

Download works fine.
Only the Error Console is suspicious.

> once more info is available on why we should block.

Don't bother.


(In reply to comment #4)
> It always happens
> with torrent files with me.  The download manager displays "checking for
> viruses" for a few seconds(4-10) or I get a not responding Firefox windows.  

While I did make comment 2, about the errors showing up,
I'm not yet convinced about the relation with the behavior you're experiencing.

Can you confirm that it did work before bug 415498 landed ?
(If it was already "wrong", fill a separate bug about it.)
Flags: wanted1.9.0.x?
(In reply to comment #1)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041701 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

> 1. Load <http://download.openoffice.org/other.html#en-US>.
> 2. Click on a "Download" link.

FWIW, Venkman reports
{{
Exception ``[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: file:///.../seamonkey/components/nsHelperAppDlg.js :: anonymous :: line 361" data: no]'' thrown from function anonymous(filename=string:"OOo_2.4.0_Win32Intel_install_wJRE_fr.exe", url=XPComponent:{13}) in <file:/.../seamonkey/components/nsHelperAppDlg.js> line 361.

Exception ``[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsILocalFile.initWithPath]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///.../seamonkey/components/nsHandlerService.js :: HS__getFileWithPath :: line 604" data: no]'' thrown from function HS__getFileWithPath(aPath=string:"") in <file:/.../seamonkey/components/nsHandlerService.js> line 604.
}}
See
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js&rev=1.75&mark=361#332>
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/uriloader/exthandler/nsHandlerService.js&rev=1.21&mark=600-604#589>
See bug 393492 (discussion)
This(In reply to comment #4)
> The user experience is not that good, at least in my case.  It always happens
> with torrent files with me.  The download manager displays "checking for
> viruses" for a few seconds(4-10) or I get a not responding Firefox windows.  It
> seems to be checking for viruses even after the torrent has been passed to the
> bit torrent client.

I think it's quite unlikely that the NS_ERROR_NOT_AVAILABLE message is connected with any hang behavior or virus check problems.  The error message started reappearing because of a change to XPConnect error message propagation behavior landed by bent, I believe.
(In reply to comment #7)
> I think it's quite unlikely that the NS_ERROR_NOT_AVAILABLE message is
> connected with any hang behavior or virus check problems.  The error message
> started reappearing because of a change to XPConnect error message propagation
> behavior landed by bent, I believe.

(Yes, I already said both things.)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041801 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

With new profile:

Venkman reports:
{{
Exception ``[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: file:///.../seamonkey/components/nsHelperAppDlg.js :: anonymous :: line 361" data: no]'' thrown from function anonymous(filename=string:"OOo_2.4.0_Win32Intel_install_wJRE_fr.exe", url=XPComponent:{13}) in <file:/.../seamonkey/components/nsHelperAppDlg.js> line 361.

Exception ``2147746065'' thrown from function HS_fillHandlerInfo(aOverrideType=string:"", aHandlerInfo=XPComponent:{21}) in <file:/.../seamonkey/components/nsHandlerService.js> line 328.

Exception ``2147746065'' thrown from function HS_getTypeFromExtension(aFileExtension=string:".exe") in <file:/.../seamonkey/components/nsHandlerService.js> line 473.

Error ``this.mLauncher.MIMEInfo.preferredApplicationHandler is null'' [x-] in file ``file:///.../seamonkey/components/nsHelperAppDlg.js'', line 466, character 0.

Exception ``TypeError: this.mLauncher.MIMEInfo.preferredApplicationHandler is null'' thrown from function anonymous() in <file:/.../seamonkey/components/nsHelperAppDlg.js> line 466.

[e] message = [string] "this.mLauncher.MIMEInfo.preferredApplicationHandler is null"
}}

Error Console: (with/without Strict mode)
{{
Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::fillHandlerInfo]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]
}}
If I enable |dom.report_all_js_exceptions = true|, I get 2 more:
{{
Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::fillHandlerInfo]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///D:/Progs/__Mozilla-SR/seamonkey/components/nsHelperAppDlg.js :: anonymous :: line 297"  data: no]
Source File: file:///D:/Progs/__Mozilla-SR/seamonkey/components/nsHelperAppDlg.js
Line: 297

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///D:/Progs/__Mozilla-SR/seamonkey/components/nsHelperAppDlg.js :: anonymous :: line 297"  data: no]
Source File: file:///D:/Progs/__Mozilla-SR/seamonkey/components/nsHelperAppDlg.js
Line: 297
}}
(In reply to comment #7)

> The error message
> started reappearing because of a change to XPConnect error message propagation
> behavior landed by bent, I believe.

Yeah. This particular exception was, I believe, the reason myk made the first change to suppress exceptions like this (which has now essentially been reverted by bug 415498).

So, this bug could be resolved as INVALID -- things are working as designed, it's just reporting things that were deliberately suppressed before. Alternatively, this bug could be mutated to "find a new way to suppress these errors" because they are indeed very annoying to spam the console with.

(In reply to comment #10)
> Yeah. This particular exception was, I believe, the reason myk made the first
> change to suppress exceptions like this (which has now essentially been
> reverted by bug 415498).

Minor correction: I made the change to start throwing the exception, and mrbkap made the change that then suppressed the exception (which regressed display of other exceptions and was thus backed out with bent's recent fix).


> So, this bug could be resolved as INVALID -- things are working as designed,
> it's just reporting things that were deliberately suppressed before.
> Alternatively, this bug could be mutated to "find a new way to suppress these
> errors" because they are indeed very annoying to spam the console with.

I think the latter is the right approach.  We do still want to suppress these exceptions, which it doesn't make sense to display, but we'll need to find a new way to do it, perhaps for Moz2.
> I think the latter is the right approach.  We do still want to suppress these
> exceptions, which it doesn't make sense to display, but we'll need to find a
> new way to do it, perhaps for Moz2.

Agreed, though I suspect this wants to happen in a 1.9.0.* release, because users are likely to find these confusing when they're trying to diagnose actual errors using console messages.
 

(In reply to comment #12)
> Agreed, though I suspect this wants to happen in a 1.9.0.* release, because
> users are likely to find these confusing when they're trying to diagnose actual
> errors using console messages.

I would like that to happen, and I agree that these messages are confusing, but apparently it's hard to do, which is why we intentionally regressed the behavior to show these messages again in order to fix the other regressions from when we hid them.
Would it be possible to add a flag to nsIXPCException, to allow the thrower to control if the error should be reported? Something like:

var e = new Components.Exception("no cheezburger", Cr.NS_ERROR_NOT_AVAILABLE);
e.dontSpamTheConsole = true;
throw e;
We *could* do that, but I'd much rather see us move away from the broken model we have right now where expected events are flagged with exceptions. Exceptions should IMO only be used in exceptional cases, not in cases that are known to happen w/o mis-using the API in question. And yes, XPCOM (and COM) is broken in this regard.
So if I follow this correctly, we will have to tell all users with any errors something like this:
 Please activate the Error Console with Firefox->Tools->Error Console. Ignore the first 70 or so exceptions with red Xs in front, they are perfectly normal.  Look for more red Xs after that.

This does not seem like a reasonable resolution to me.
(In reply to comment #16)
> This does not seem like a reasonable resolution to me.

Everyone agrees that our current situation is not ideal. Please see bug 415498 and its dependencies/blockers for extensive discussion.
So, what's the right path to take with this bug? Either the handler service needs to be changed to not throw (late-compat!), or we need a short-term workaround (ie, hack!) to give throwers an opt-in to decrease their console noise.
I'd vote for tweaking the handler service API.  This seems like something possibly (probably?) worth taking for 1.9, because I'm already seeing this causing misdiagnosis of all sorts of stuff all over bugzilla.
In that case I think we should reconsider blocking on this since we're trying to avoid changing interfaces in a dot release. I agree with others that we should tweak the API rather than somehow hacking around these exceptions.
Flags: blocking1.9- → blocking1.9?
We don't want to change it in a dot release, so if we don't get it before we ship we don't get it until .next.  I don't think that makes it blocking, though, because I would ship with these exception spews rather than delay if it were the last one left.

I look forward to approving a patch for this! :)
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Flags: wanted-next+
Flags: blocking1.9?
Flags: blocking1.9-
I suppose the obvious fix here would be to have nsIHandlerService :: getTypeFromExtension() return null when it can't map an extension to a type (instead of throwing). [I think this was the function responsible for most of the console spam... fillHandlerInfo() can throw too, I'll check to see how often that happens, but it could be changed to return a value too.]

I think this shouldn't be too much of a late-compat problem, because it looks like this interface is new in 1.9. But it's a little confusing because nsIMIMEService also happens to have a getTypeFromExtension() method, so I'll need to do a little more digging beyond my quick mxr scan.
(I'm currently looking at fillHandlerInfo(), which is this bug.
I haven't looked at getTypeFromExtension(), which is some other bug report(s).)
Summary: NS_ERROR_NOT_AVAILABLE in [nsIHandlerService::fillHandlerInfo] → console spam from nsIHandlerService
Attached patch (Av1) throw -> return (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042202 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

See
<http://mxr.mozilla.org/seamonkey/search?string=fillHandlerInfo&find=%2Furiloader%2Fexthandler%2Fns&tree=seamonkey>

This patches fixes the .idl and the .js, per comment 1 steps.

The .cpp looks like it might work as is, as the rv is always set and checked,
but I need someone else to test/confirm this.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #317132 - Flags: superreview?
Attachment #317132 - Flags: review?
Attachment #317132 - Flags: superreview?(cbiesinger)
Attachment #317132 - Flags: superreview?
Attachment #317132 - Flags: review?(jst)
Attachment #317132 - Flags: review?
Attached patch (Av1a) throw -> return (obsolete) — Splinter Review
Av1, with updated return type.

NB: The uuid will need to be updated too.
Attachment #317132 - Attachment is obsolete: true
Attachment #317136 - Flags: superreview?(cbiesinger)
Attachment #317136 - Flags: review?(jst)
Attachment #317132 - Flags: superreview?(cbiesinger)
Attachment #317132 - Flags: review?(jst)
Hmm, how would nsExternalHelperAppService.cpp manage to work with this (or even compile)? The boolean return would be an extra argument for C++ callers, no?

old: rv = svc->FillHandlerInfo(handler, type)
new: PRBool bool; rv = svc->FillHandlerInfo(handler, type, &bool);
(In reply to comment #24)
> Created an attachment (id=317132) [details]
> (Av1) throw -> return
> 
> The .cpp looks like it might work as is, as the rv is always set and checked,
> but I need someone else to test/confirm this.

(In reply to comment #26)
> Hmm, how would nsExternalHelperAppService.cpp manage to work with this (or even
> compile)? The boolean return would be an extra argument for C++ callers, no?

As I +/- wrote, I have no idea (and no compile environment).

(anyone) Feel free to add the missing .cpp fix to my patch.
Attachment #317136 - Flags: superreview?(cbiesinger)
Attachment #317136 - Flags: review?(jst)
Attachment #317136 - Flags: review-
Serge, I'll take this tomorrow since you don't have a compile environment, unless dolske wants it?
Assignee: sgautherie.bz → bent.mozilla
Status: ASSIGNED → NEW
Yeah, I'll take it. I already have the getTypeForExtension() case fixed.
Assignee: bent.mozilla → dolske
Attached patch Patch v.1 (obsolete) — Splinter Review
* Changed getTypeFromExtension() to return an empty string instead of throwing (when it can't get a type for the extension). I suppose it could return null instead, but it doesn't seem like an empty string would ever be a valid return type anyway. This eliminates 95% of the console spam.

* Did *not* change fillHandler() -- it still throws. Instead, I noticed that since there's already an exists() method, it would be cleaner to call that first to see if it has handlerinfo available (and if not, we avoid calling fillHandler in a way that might throw). There was one case I couldn't use this trick for, but I think it's good enough for the majority of cases.

Two bits I'm not sure about:

* The code in exists() is slightly different than the relevant code in getTypeFromExtension(). It's RDF gobbledygook, so I'm not sure if this is a issue. Help!

* I'm not sure how to generate a meaningful test for this. A test for getTypeFromExtension("bogus") is easy to do, but I'm at loss how to test legit file extensions. This is mostly because this  code path is *not* used for normal OS default file extensions (eg, "txt"), and I'm not sure to to inject stuff into mimeTypes.rdf to test this case. But the actual code changes are fairly simple, and manually validating the code is doable, so maybe that's enough.
Attachment #317136 - Attachment is obsolete: true
Attachment #317210 - Flags: review?(dmose)
(In reply to comment #18)
> So, what's the right path to take with this bug? Either the handler service
> needs to be changed to not throw (late-compat!), or we need a short-term
> workaround (ie, hack!) to give throwers an opt-in to decrease their console
> noise.

Then, you're proposing the (partial) workaround solution.
I thought, from comment 15 (jst) and comment 19 (dmose), that we did want the "not throw" change (for both functions) ?
No. Using exists() just means there's a sensible pattern that avoids having to touch a bunch of extra code. I think it's cleaner anyway, since it's separating the tasks of "do you have info for this handler type" and "fill in your info for this handler type". [The analogy would be trying to read from a file that doesn't exist -- you should have checked for the file's existence first. Reading from a non-existent file is an exceptional condition.]
Comment on attachment 317210 [details] [diff] [review]
Patch v.1

Since the API has changed such that the user should almost always be calling be calling exist() first, I think the IDL documentation needs to change to reflect this.

>+  if (handlerSvc) {
>+    PRBool hasHandler;
>+    rv = handlerSvc->Exists(*aHandlerInfo, &hasHandler);
>+    NS_ENSURE_SUCCESS(rv, rv);

I don't think NS_ENSURE_SUCCESS is what's desired here.  One could imagine the RDF glop throwing an exception if there's something wrong with mimeTypes.rdf, and we want to fallback to SetProtocolHandlerDefaults() in that case.

In theory, a number of the uses of exists() in this patch have that problem.  So maybe what should really happen is that exists() wants a try/catch around the RDF flop...

If we don't do that, then callers end up often needing to do a try/catch around an exists, which seems kinda ugly...

Thoughts?
Attachment #317210 - Flags: review?(dmose) → review-
(In reply to comment #30)
>
> * The code in exists() is slightly different than the relevant code in
> getTypeFromExtension(). It's RDF gobbledygook, so I'm not sure if this is a
> issue. Help!

Should be fine.  The latter searches for a node in the graph that has a fileExtensions arc, the former searches by mime-type directly.  From what I can see, either should work as long as the file is not corrupt.

> * I'm not sure how to generate a meaningful test for this.

As per IRC, consider providing your own mimeTypes.rdf file with an  nsIDirectoryServiceProvider.
ctalbert asked me to comment in the bug re: whether we should have an automated test or a manual Litmus test for this type of situation. I believe it would be preferred to have some kind of automated way of testing it if possible.
Blocks: tb-noise
Attached patch Patch v.2 (obsolete) — Splinter Review
So, this should address the review nits.

I'm still not sure how to actually make a test for this. The unit test head*.js deletes any existing mimeTypes.rdf. The same head*.js script is loaded for any unit test there, so it's not clear how to make it act differently for this new test, without changing how it works for the existing tests. The nsIFile is cached at different points, so that makes changing the source file at run time not fun. I'm tempted to just have the test itself overwrite mimeTypes.rdf mid-run, but that's an ugly hack (and doesn't RDF do it's own caching, too?) I really just want to be able to tell the handler service (or mimeinfo service?) to store some junk in mimeTypes.rdf that I can test against, but I can't figure that out either. *throws up hands*

Given the sucktasticness of trying to make this an automated test, I'm inclined to go with just verifying the fix by hand. [Maybe spin off "we'd like a test" ala bug 420401] And, frankly, I'm worried about breaking the existing tests.
Attachment #317210 - Attachment is obsolete: true
Attachment #317947 - Flags: superreview?(dmose)
Attachment #317947 - Flags: review?(cbiesinger)
Test-sucktasticness should definitely be added to bugzilla keyword descriptions
for future reference.
Note: I'm seeing the 2nd error from comment 0 for every page that is blocked by the phishing protection.
Comment on attachment 317947 [details] [diff] [review]
Patch v.2

sr=biesi
Attachment #317947 - Flags: superreview?(dmose)
Attachment #317947 - Flags: superreview+
Attachment #317947 - Flags: review?(dmose)
Attachment #317947 - Flags: review?(cbiesinger)
Comment on attachment 317947 [details] [diff] [review]
Patch v.2

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v
>retrieving revision 1.369
>diff -u -p -8 -r1.369 nsExternalHelperAppService.cpp
>--- uriloader/exthandler/nsExternalHelperAppService.cpp	3 Apr 2008 03:02:10 -0000	1.369
>+++ uriloader/exthandler/nsExternalHelperAppService.cpp	27 Apr 
>
>+  if (handlerSvc) {
>+    PRBool hasHandler;
>+    rv = handlerSvc->Exists(*aHandlerInfo, &hasHandler);
>+    NS_ENSURE_SUCCESS(rv, rv);

From a defensive programming standpoint, this still doesn't want to be NS_ENSURE_SUCCESS: there's no case here we want to return early.

sr=dmose with that fix.

Re the testing stuff, it's not clear to me what the problem about using the API to add stuff to mimeTypes.rdf is.  There are examples of this in test_handlerService.js.
Attachment #317947 - Flags: review?(dmose) → review+
(In reply to comment #40)
> From a defensive programming standpoint, this still doesn't want to be
> NS_ENSURE_SUCCESS: there's no case here we want to return early.

Just don't forget to initialize that variable :)
Duplicate of this bug: 430108
Attached patch Patch v.3 (obsolete) — Splinter Review
Updated with dmose's nits (hopefully for real this time :).

I also attached the test I tried making again...

The first few lines added in the test are testing the case this bug modifies (making getTypeFromExtension() return empty-string instead of throwing). So, maybe that's enough and I'm just overthinking the tests. But what I was trying to add was tests for the normal case, when getTypeFromExtension() finds a type for the specified extension. That's what I can't seem to make work.
Attachment #317947 - Attachment is obsolete: true
Attached patch Patch v.4 (obsolete) — Splinter Review
Ahha, got the test working! I mentioned that part of the problem I was having was that .store() wasn't putting any file extension info into mimeTypes.rdf, and dmose filled in the pieces... Turns out that wasn't implemented, and callers are required to wade into RDF to actually store this info. That's what toolkit/mozapps/downloads/content/helperApps.js does, and so I lifted the code from there.
Attachment #318217 - Attachment is obsolete: true
Attachment #318256 - Flags: review?(dmose)
Comment on attachment 318256 [details] [diff] [review]
Patch v.4

> 
>   exists: function HS_exists(aHandlerInfo) {
>-    var typeID = this._getTypeID(this._getClass(aHandlerInfo), aHandlerInfo.type);
>-    return this._hasLiteralAssertion(typeID, NC_VALUE, aHandlerInfo.type);
>+    var found;
>+
>+    try {
>+      var typeID = this._getTypeID(this._getClass(aHandlerInfo), aHandlerInfo.type);
>+      found = this._hasLiteralAssertion(typeID, NC_VALUE, aHandlerInfo.type);
>+    } catch (e) {
>+      // If the RDF threw (eg, corrupt file), treat as non-existent.
>+      found = false;
>+    }

Treating file corruption as non-existing is worth documenting in the nsIHandlerService IDL, I think.

>+  // add a handler for the extension
>+  // XXX throws: var lolHandler = mimeSvc.getFromTypeAndExtension(null, "lolcat");

Can you make the above comment clearer?  It's not clear whether this is intended to mean "there's a bug that we need to fix and then we should uncomment this line" or something else...

Otherwise, looks great!  r=dmose with those tweaks.
Attachment #318256 - Flags: review?(dmose) → review+
Nits fixed. I just deleted the XXX line, since it wasn't relevant to the test.
Attachment #318256 - Attachment is obsolete: true
Attachment #318284 - Flags: approval1.9?
Comment on attachment 318284 [details] [diff] [review]
Patch v.5
[Checkin: Comment 52]

This is a pretty big patch and I'm unclear on the potential for risk. Dan/Justin, can you comment briefly and renominate? I'd like to take this as per the previous comments' requests, but want to be clear on what sort of risk I'm introducing to nsIHandlerService. I do see a testcase, though, which is promising! It touches all changes?
Attachment #318284 - Flags: approval1.9? → approval1.9-
Comment on attachment 318284 [details] [diff] [review]
Patch v.5
[Checkin: Comment 52]

I think the core change itself is simple, well-understood, and low-risk (despite the long bug discussion :-). It's basically just logic change: make getTypeFromExtension() return empty-string instead of throwing, and guard calls to fillHandlerInfo() with existence checks. In addition to the testcase I've double-checked stuff in the browser prefs and viewing/saving files in different ways, and don't see any problems.
Attachment #318284 - Flags: approval1.9- → approval1.9?
I agree, and would very much prefer that we take this change now than in 3.1 (it's a non-option for 3.0.x, since it's incompatible).  It's likely to be a top complaint of extension developers, whose extensions are checked for error-cleanliness during the AMO review process.

+1
Yeah, I also want us to take this patch.  If we don't take this patch, we'll continue spamming error consoles the world over and we'll get a ton of duplicate bugs filed on this. I'd prefer not to generate bug noise if we can help it.

Let's take this fix please.

It has a test, it's well understood, and we also have manual methods that can reproduce the issue.

+1 more
Comment on attachment 318284 [details] [diff] [review]
Patch v.5
[Checkin: Comment 52]

a1.9+=damons
Attachment #318284 - Flags: approval1.9? → approval1.9+
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
  new revision: 1.370; previous revision: 1.369
Checking in uriloader/exthandler/nsHandlerService.js;
  new revision: 1.22; previous revision: 1.21
Checking in uriloader/exthandler/nsIHandlerService.idl;
  new revision: 1.7; previous revision: 1.6
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
  new revision: 1.18; previous revision: 1.17
Status: NEW → RESOLVED
Closed: 12 years ago
OS: Windows Vista → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Duplicate of this bug: 395228
Duplicate of this bug: 428739
This is an after-checkin comment, but anyway:

First, I am glad that nsHandlerService.js is fixed not to throw, because JS exception are extremely expensive, compared to normal execution paths.

Second, (In reply to comment #10)
> Alternatively, this bug could be mutated to "find a new way to suppress these
> errors" because they are indeed very annoying to spam the console with.

I filed a patch in bug 415498 - attachment 314319 [details] [diff] [review], which suppresses exactly the kind of exception nsHadlerService.js was throwing, but *nothing* else. I can easily update it, if need be.
Attachment #318284 - Attachment description: Patch v.5 → Patch v.5 [Checkin: Comment 52]
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008043001 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed, per comment 2 (both) steps.
Status: RESOLVED → VERIFIED
Keywords: regression
No longer depends on: 395228
Duplicate of this bug: 430169
Duplicate of this bug: 428671
(In reply to comment #55)
> (In reply to comment #10)
> > Alternatively, this bug could be mutated to "find a new way to suppress these
> > errors" because they are indeed very annoying to spam the console with.
> 
> I filed a patch in bug 415498 - attachment 314319 [details] [diff] [review], which suppresses exactly the
> kind of exception nsHadlerService.js was throwing, but *nothing* else. I can
> easily update it, if need be.

As both this and that bug are now fixed, and your patch was r-,
I think you should better file a new bug with a new patch, as needed.
(In reply to comment #59)
> As both this and that bug are now fixed, and your patch was r-,
> I think you should better file a new bug with a new patch, as needed.

The question remains whether filing a new bug is worth the effort. If the answer is positive, sure I will.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.