408103 - Returning null from nsIProtocolHandler.newChannel() crashes [@ nsDocShell::DoURILoad]

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Eric Jung [:ericjung]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

I'm using this custom protocol handler in an extension of mine. Place it in an extension's components/ directory so it gets registered (deleting compreg.dat as necessary), then navigate to magoo://foo/bar/baz.bin. FF3b1 crashes. Returning a valid channel from the newChannel() implementation works as expected, but if that function instead returns null, FFb1 crashes.

// Thanks for the template, doron (http://www.nexgenmedia.net/docs/protocol/)
const kSCHEME = "magoo";
const kPROTOCOL_NAME = "Magoo Protocol";
const kPROTOCOL_CONTRACTID = "@mozilla.org/network/protocol;1?name=" + kSCHEME;
const kPROTOCOL_CID = Components.ID("45505afc-a8f7-11dc-8314-0800200c9a66");
var CI = Components.interfaces, CC = Components.classes, CR = Components.results;

// Mozilla defined
const kSIMPLEURI_CONTRACTID = "@mozilla.org/network/simple-uri;1";
const nsISupports = Components.interfaces.nsISupports;
const nsIIOService = Components.interfaces.nsIIOService;
const nsIProtocolHandler = Components.interfaces.nsIProtocolHandler;
const nsIURI = Components.interfaces.nsIURI;

function Protocol() {}

Protocol.prototype = {
  QueryInterface: function(iid) {
    if (!iid.equals(nsIProtocolHandler) &&
        !iid.equals(nsISupports))
      throw Components.results.NS_ERROR_NO_INTERFACE;
    return this;
  },

  scheme: kSCHEME,
  defaultPort: -1,
  protocolFlags: nsIProtocolHandler.URI_DANGEROUS_TO_LOAD, // required for FF3.x

  allowPort: function(port, scheme) {
    return false;
  },

  newURI: function(spec, charset, baseURI) {
    var uri = Components.classes[kSIMPLEURI_CONTRACTID].createInstance(nsIURI);
    uri.spec = spec;
    return uri;
  },

  newChannel: function(aURI) {
    return null; // CRASHES FF 3.0b1
  },
}

var ProtocolFactory = new Object();

ProtocolFactory.createInstance = function (outer, iid) {
  if (outer != null)
    throw Components.results.NS_ERROR_NO_AGGREGATION;

  if (!iid.equals(nsIProtocolHandler) &&
      !iid.equals(nsISupports))
    throw Components.results.NS_ERROR_NO_INTERFACE;

  return new Protocol();
}


/**
 * JS XPCOM boilerplate component registration code.
 *
 * We set ourselves up to observe the xpcom-startup category.  This provides
 * us with a starting point.
 */

var TestModule = new Object();

TestModule.registerSelf = function (compMgr, fileSpec, location, type) {
  compMgr = compMgr.QueryInterface(Components.interfaces.nsIComponentRegistrar);
  compMgr.registerFactoryLocation(kPROTOCOL_CID,
                                  kPROTOCOL_NAME,
                                  kPROTOCOL_CONTRACTID,
                                  fileSpec,
                                  location,
                                  type);
}

TestModule.getClassObject = function (compMgr, cid, iid) {
  if (!cid.equals(kPROTOCOL_CID))
    throw Components.results.NS_ERROR_NO_INTERFACE;

  if (!iid.equals(Components.interfaces.nsIFactory))
    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

  return ProtocolFactory;
}

TestModule.canUnload = function (compMgr) {
  return true;
}

function NSGetModule(compMgr, fileSpec){
  return TestModule;
}

Reproducible: Always

Steps to Reproduce:
1. Register the custom protocol handler component as described in the "Details" section.
2. Visit magoo://foo/bar/baz.bin or any other URL to invoke the custom protocol handler.
3. Observe FF3b1 crash.
Actual Results:  
FF3b1 crashes.

Expected Results:  
I don't know, but definitely not crash. Perhaps it could dance a little jig or explore the north pole or pet Mr. Bigglesworth.

Comment 1

[Eric Jung [:ericjung]]

Note: if you copy/paste the above code, be sure the JS comment "// required for
FF3.x" doesn't wrap the line, else the component won't register.

Comment 2

[Nickolay_Ponomarev]

899bb0f5-a988-11dc-ae0b-001a4bd43ef6

The IOService should probably be changed to guarantee that the result of newChannel[FromURI] is non-null unless rv indicates failure.

Comment 3

[Nickolay_Ponomarev]

Cool, does bugzilla not link crash IDs from breakpad? http://crash-stats.mozilla.com/report/index/899bb0f5-a988-11dc-ae0b-001a4bd43ef6?date=2007-12-13-14

Comment 4

[timeless]

bugzilla /should/ link: bp-899bb0f5-a988-11dc-ae0b-001a4bd43ef6

or:
ID: 899bb0f5-a988-11dc-ae0b-001a4bd43ef6
Signature	nsDocShell::DoURILoad(nsIURI*, nsIURI*, int, nsISupports*, char const*, nsIInputStream*, nsIInputStream*, int, nsIDocShell**, nsIRequest**, int)
UUID	899bb0f5-a988-11dc-ae0b-001a4bd43ef6
Time	2007-12-13 06:34:26-08:00
Build ID	2007121108
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 6
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0
Crashing Thread
Frame 	Signature 	Source
0 	nsDocShell::DoURILoad(nsIURI*, nsIURI*, int, nsISupports*, char const*, nsIInputStream*, nsIInputStream*, int, nsIDocShell**, nsIRequest**, int) 	mozilla/docshell/base/nsDocShell.cpp:7134
1 	nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, unsigned short const*, char const*, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**) 	mozilla/docshell/base/nsDocShell.cpp:7037
2 	nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, int) 	mozilla/docshell/base/nsDocShell.cpp:896
3 	nsDocShell::LoadURI(unsigned short const*, unsigned int, nsIURI*, nsIInputStream*, nsIInputStream*) 	mozilla/docshell/base/nsDocShell.cpp:2884
4 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
5 	AutoJSSuspendRequest::SuspendRequest() 	mozilla/js/src/xpconnect/src/xpcprivate.h:3366
6 	@0x12e18b 	

Comment 5

[timeless]

Attachment: be careful

OK, got it... I'm slow.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.876&mark=6477-6478,6515-6516,6693,7033,7096-7097,7129,7135,7154-7155,7157#7000

6477 locka           1.283 NS_IMETHODIMP
6478 rpotts          1.296 nsDocShell::InternalLoad(nsIURI * aURI,
/* public entry point */
6515 jst             1.545     if (!aURI) {
6516                               return NS_ERROR_NULL_POINTER;
/* after this point, aURI should not be null */
6693                                   if (targetDocShell && currentCodebase && aURI) {
/* useless null check */
7033 dveditz         1.646     rv = DoURILoad(aURI, aReferrer,
/* chain to next function */
7096 jkeiser         1.432 nsresult
7097                       nsDocShell::DoURILoad(nsIURI * aURI,
/* private method */
7129 darin           1.430     rv = NS_NewChannel(getter_AddRefs(channel),
/* returned NS_OK + out nsnull
7135 bzbarsky        1.495     if (NS_FAILED(rv)) {
/* rv check, but no null check */
7154                           if (aRequest)
7155                               NS_ADDREF(*aRequest = channel);
/* #1 maybe crash */
7157 locka           1.283     channel->SetOriginalURI(aURI);
/* #2 crash */

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/base/public/nsIIOService.idl&rev=1.67&mark=54

As far as I'm concerned, this interface makes no promises. it doesn't say whether things will throw an exception or return null. Given the current *frozen* contract, this means someone could replace the io-service with an implementation which continues to return 0 instead of one that returns an exception. As such, while I will change the io service, I'm going to change the docshell code too, because it is relying on something beyond the contract provided.

Comment 6

[timeless]

Attachment: whitespace

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293368 [details] [diff] [review]
be careful

I don't see how anyone reading nsIProtocolHandler can possibly decide that returning a null channel without throwing is a possible valid result of a newChannel call.  Which means that I think this is not a bug we should be fixing.  If you decide to "fix" it (which I don't think you should), fix it in the ioservice or NS_NewChannel (which have many other callers), not at this random callsite.  And please don't make unrelated whitespace changes.

Put another way, there are many other API contract violations extensions can perform that would cause crashes.  The answer is not to bloat the core code with all possible protections against malicious misbehavior by extensions...

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 293368 [details] [diff] [review]
> Put another way, there are many other API contract violations extensions can
> perform that would cause crashes.  The answer is not to bloat the core code
> with all possible protections against malicious misbehavior by extensions...

is there a will to decide wontfix ?


there is still a small class of startup crashes with signatures of :

nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsISupports*, char const*, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool)

nsDocShell::DoURILoad(nsIURI*, nsIURI*, int, nsISupports*, char const*, nsIInputStream*, nsIInputStream*, int, nsIDocShell**, nsIRequest**, int, int, int) 

nsDocShell::DoURILoad