Closed Bug 392322 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: crosson, Assigned: bent.mozilla)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
This is a regression from bug 389508.  If the principal is the system principal, it has no URI, and things break badly.
Blocks: xxx
Status: UNCONFIRMED → NEW
Component: General → DOM: Mozilla Extensions
Ever confirmed: true
Flags: blocking1.9+
OS: Windows XP → All
QA Contact: general → general
Hardware: PC → All
Jonas, want to patch?  I can promise speedy reviews!
Assignee: nobody → jonas
No longer crashes it seems?
Keywords: crash, regression
Summary: XMLHttpRequest crashes on local file retrieval → XMLHttpRequest crashes on local file retrieval [@ nsCrossSiteListenerProxy::OnStartRequest]
Version: unspecified → Trunk
I don't see why it wouldn't...
Duplicate of this bug: 404818
(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.
It's really the same issue.  This code assumes it can get a principal with a URI, which is an incorrect assumption.
Attached patch Patch, v1 (obsolete) — Splinter Review
This adds a native Init method for C++ callers to specify their own principal. Callers will probably just pass the system principal.
Assignee: jonas → bent.mozilla
Status: NEW → ASSIGNED
Attachment #301419 - Flags: review?(jonas)
Attachment #301419 - Flags: superreview+
Attachment #301419 - Flags: review?(jonas)
Attachment #301419 - Flags: review+
This patch still crashes for me, in nsCrossSiteListenerProxy::AddRequestHeaders because the system principal (mPrincipal) has no uri.
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?
Blocks: 415772
(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.
Attached patch Patch, v2Splinter Review
This should do it. Also added a (pretty simple) C++ test so we can hopefully avoid these in the future.
Attachment #301419 - Attachment is obsolete: true
Attachment #301733 - Flags: review?(jonas)
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.
Priority: P2 → P1
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?
(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.
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 on attachment 301733 [details] [diff] [review]
Patch, v2

Ah, that makes sense then.
Attachment #301733 - Flags: superreview+
Attachment #301733 - Flags: review?(jonas)
Attachment #301733 - Flags: review+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Backed out due to weird test failure, I'll investigate today.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Checked in again.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
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.
(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.
(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.
Duplicate of this bug: 404818
Blocks: 419106
Blocks: 386823
Crash Signature: [@ nsCrossSiteListenerProxy::OnStartRequest]
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.