392322 - XMLHttpRequest crashes on local file retrieval [@ nsCrossSiteListenerProxy::OnStartRequest]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Jerry Crosson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7) Gecko/2007081419 Minefield/3.0a7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7) Gecko/2007081419 Minefield/3.0a7

When getting a local file via XMLHttpRequest in a C++ component,  crashes in nsCrossSiteListenerProxy::OnStartRequest
with a NULL mRequestingURI

Reproducible: Always

Steps to Reproduce:
1. Request local file, say "resource:/config.xml" 
2. 
3.
Actual Results:  
crashes in nsCrossSiteListenerProxy::OnStartRequest
with a NULL mRequestingURI

Expected Results:  
config.xml is retrieved.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 389508.  If the principal is the system principal, it has no URI, and things break badly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jonas, want to patch?  I can promise speedy reviews!

Comment 3

[Nickolay_Ponomarev]

No longer crashes it seems?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't see why it wouldn't...

Comment 6

[aaronr]

(In reply to comment #5)
> *** Bug 404818 has been marked as a duplicate of this bug. ***
> 

I saw this bug and didn't think my issue was the same because in my case there wasn't a principal to begin with.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's really the same issue.  This code assumes it can get a principal with a URI, which is an incorrect assumption.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This adds a native Init method for C++ callers to specify their own principal. Callers will probably just pass the system principal.

Comment 9

[Neil Deakin]

This patch still crashes for me, in nsCrossSiteListenerProxy::AddRequestHeaders because the system principal (mPrincipal) has no uri.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 301419 [details] [diff] [review]
Patch, v1

Can you add some xpcshell tests, please, so we know what's guaranteed not to crash (especially the scenario Neil mentions) and ensure it won't regress in the future?

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #9)
> This patch still crashes for me, in nsCrossSiteListenerProxy::AddRequestHeaders
> because the system principal (mPrincipal) has no uri.

Oh, sorry Neil. If you're using the system principal you're never supposed to get there, but I failed to update our IsSameOrigin function. Patch in a sec.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

This should do it. Also added a (pretty simple) C++ test so we can hopefully avoid these in the future.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 301733 [details] [diff] [review]
Patch, v2

> static PRBool
> IsSameOrigin(nsIPrincipal* aPrincipal, nsIChannel* aChannel)
> ...
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_SUCCESS(rv, PR_FALSE);

Oh, and notice how that was going to return true if anything failed! Fixed those and removed a random bit of commented code.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand how Neil could hit a crash with a null principal there. The nullchecks at the top of Open and Send should catch that, no? That said, I don't see where you added checks to protect against that?

Comment 15

[Neil Deakin]

(In reply to comment #14)
> I don't understand how Neil could hit a crash with a null principal there.

The principal isn't null; it's the system principal but it has a null uri.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jonas, this is the real fix:

-  if (!(mState & XML_HTTP_REQUEST_XSITEENABLED) &&
-      !IsSameOrigin(mPrincipal, mChannel)) {
+  if (IsSystemPrincipal(mPrincipal)) {
+    // Cchrome callers are always allowed to read from different origins.
+    mState |= XML_HTTP_REQUEST_XSITEENABLED;
+  }
+  else if (!(mState & XML_HTTP_REQUEST_XSITEENABLED) &&
+           !IsSameOrigin(mPrincipal, mChannel)) {
     mState |= XML_HTTP_REQUEST_USE_XSITE_AC;
   }

That change prevents us from ever hooking up an nsCrossSiteListenerProxy (which does assume that it can get a URI from its principal).

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 301733 [details] [diff] [review]
Patch, v2

Ah, that makes sense then.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Backed out due to weird test failure, I'll investigate today.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Checked in again.

Comment 21

[Philipp Wagner [:imphil]]

Bug 404818, which has been marked to be a duplicate of this bug, is not yet fixed.  The attached testcase URL gives an "Unable to load schema" error, while it is working in FF2.

Comment 22

[aaronr]

(In reply to comment #21)
> Bug 404818, which has been marked to be a duplicate of this bug, is not yet
> fixed.  The attached testcase URL gives an "Unable to load schema" error, while
> it is working in FF2.
> 

I debugged this down.  We aren't crashing anymore, which is good, but nsSchemaLoader::LoadAsync's call to nsXMLHttpRequest::OpenRequest (to load the schema file) is failing because nsXMLHttpRequest's mPrincipal is nsnull.

Comment 23

[Neil Deakin]

(In reply to comment #22)

> I debugged this down.  We aren't crashing anymore, which is good, but
> nsSchemaLoader::LoadAsync's call to nsXMLHttpRequest::OpenRequest (to load the
> schema file) is failing because nsXMLHttpRequest's mPrincipal is nsnull.
> 

You will need to call the new Init method of the nsXMLHttpRequest, added by this bug, and supply the principal as an argument.