When trying to access "jar:http://foo.bar/test.jar!/abc.html, foo.bar is a domain that does not exist and that should be the message shown to the user. Instead it shows a error saying "Unsafe file type"

Yikes. We should be checking for an error status in OnDownloadComplete. Right now we're clobbering error status values. In particular, we probably shouldn't be looking at some of the data we look at if the status is an error. It's not guaranteed to have sane values.

... and in the case of a redirected jar the status was actually being incorrectly overwritten. This patch handles redirects outside of NS_SUCCEEDED(status), moves security info fetch and content type check into an NS_SUCCEEDED(status), fixes the status clobbering in redirect, and removes a testing comment.

Comment on attachment 292431 [details] [diff] [review] v1 Looks great. Can we add a test somehow?

unit testing the redirect case will be a bit tougher, but this tests the simple case.

I'm wondering if this would also cause netError.xhtml to complain about missing entities, which my wife just started seeing on 2.0.0.11 (not to say that it wasn't there in previous versions, like 2.0.0.10, just that she's on .11 now). I know that netError runs without privs, and I suspect that there are some redirection-like tricks involved in getting it displayed. I'm not sure what sort of error she would have been seeing, but she got an XML error about &generic.longDesc not being defined instead.

I don't know all the details, but the "redirection-like trick," as I understand it, is that nsDocShell::DisplayLoadError calls nsDocShell::LoadErrorPage to construct an about:neterror URL that it passes to nsDocShell::InternalLoad, which loads the URL in a way that does not cause the URL bar to change or history to get updated (perhaps via nsIDocShellLoadInfo::loadBypassHistory?). According to nsAboutRedirector.cpp, about:neterror is chrome://global/content/netError.xhtml , which references chrome://global/locale/netError.dtd , which for Firefox is http://lxr.mozilla.org/mozilla/source/browser/locales/en-US/chrome/overrides/netError.dtd , which does define generic.longDesc, so it's unclear why the app would complain about that entity not being defined.

Checking in nsJARChannel.cpp; /cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v <-- nsJARChannel.cpp new revision: 1.128; previous revision: 1.127 done RCS file: /cvsroot/mozilla/modules/libjar/test/unit/test_bug407303.js,v done Checking in test/unit/test_bug407303.js; /cvsroot/mozilla/modules/libjar/test/unit/test_bug407303.js,v <-- test_bug407303.js initial revision: 1.1 done

(In reply to comment #5) > I'm wondering if this would also cause netError.xhtml to complain about missing > entities, which my wife just started seeing on 2.0.0.11 (not to say that it > wasn't there in previous versions, like 2.0.0.10, just that she's on .11 now). > I know that netError runs without privs, and I suspect that there are some > redirection-like tricks involved in getting it displayed. I'm not sure what > sort of error she would have been seeing, but she got an XML error about > &generic.longDesc not being defined instead. This bug would have caused an "Unsafe Content Type" error, but probably wouldn't cause unknown entity errors in netError. I opened bug 408267 about the unknown entity error.

The redirect case still needs to be tested, although the non-existent-site case got hit in the committed test.

Comment on attachment 292431 [details] [diff] [review] v1 approved for 1.8.1.12, a=dveditz for release-drivers

Checking in nsJARChannel.cpp; /cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v <-- nsJARChannel.cpp new revision: 1.116.2.4; previous revision: 1.116.2.3 done

In 2.0.0.11, I get an error page stating: The address isn't valid The URL is not valid and cannot be loaded. if I try to load jar:http://foo.bar/test.jar!/abc.html. Do we have a testcase for verification that I can use in branch? The attached test file is for a unit test.