660066 - Crash when comparing a moz-filedata: URI with null principal

Status: VERIFIED FIXED

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

Description

[Daniel Holbert [:dholbert]]

I just tried adding a moz-filedata URI to the xpcshell test test_URIs.js, and it crashed due to nsFileDataURI::EqualsInternal() dereferencing mPrincipal when mPrincipal is null.

It looks like other places in nsFileDataURI have null-checks for mPrincipal, e.g.
> 184 nsFileDataURI::GetPrincipalUri(nsIURI** aUri)
> 185 {
> 186   if (mPrincipal) {
> 187     mPrincipal->GetURI(aUri);
> 188   }
> 189   else {
> 190     *aUri = nsnull;
> 191   }

so EqualsInternal needs to handle a null mPrincipal, too.

Have patch in hand, just polishing/testing.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 0: add more test_URIs.js testcases

This patch just adds some more URIs to test_URIs.js.  These ones all pass.  Posting them here for lack of a better place to put them, and because the moz-file: test addition layers on top of these. (touches same contextual code)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 1: fix + test for this bug

Here's the fix. It allows for null mPrincipal, and makes the ::EqualsInternal logic more similar to nsJSURI::EqualsInternal.

(Note that I avoid passing null to nsPrincipal::Equals() -- technically it can handle a null value, but it spams a NS_WARNING to stderr, which we probably don't need/want here. (I think?))

I also extended test_URIs to test the "uri.Equals(null)" behavior on all of the URI testcases there.  Some Equals impls throw when passed null (e.g. nsSimpleURI, nsStandardURL) whereas others just return false (e.g. nsSimpleNestedURI, nsFileDataURI).  Eventually we should probably make that consistent, but for now the test just checks that we do one of those two reasonable things.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535471 [details] [diff] [review]
patch 0: add more test_URIs.js testcases

r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535476 [details] [diff] [review]
patch 1: fix + test for this bug

>+  if (!nsSimpleURI::EqualsInternal(otherJSURI, aRefHandlingMode)) {

otherFileDataUri, yes?  Did you compile this?  ;)

>+  *aResult = (!mPrincipal && !otherFileDataUri->mPrincipal);

Alternately, *aResult = (mPrincipal == otherFileDataUri->mPrincipal);

r=me either way as long as you fix the otherJSURI thing.

While you're here, do you want to change the QI in filedata to not QI to the nsSimpleURI CID and add a test for that?

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> Comment on attachment 535476 [details] [diff] [review] [review]
> patch 1: fix + test for this bug
> 
> >+  if (!nsSimpleURI::EqualsInternal(otherJSURI, aRefHandlingMode)) {
> 
> otherFileDataUri, yes?  Did you compile this?  ;)

Oops! I transferred that bit of logic (replacing a call to the superclass's normal EqualsInternal method) right before posting, and I swear I recompiled & retested, but I may have accidentally used my standard rebuild-dirs-that-I-frequently-touch script, which doesn't include /dom.  Thanks for catching that. :)

Comment 6

[Daniel Holbert [:dholbert]]

(er s/dom/content/)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch 2: keep nsFileDataURI from QI'ing to nsSimpleURI

(In reply to comment #4)
> While you're here, do you want to change the QI in filedata to not QI to the
> nsSimpleURI CID and add a test for that?

Here's patch & test for that. Confirmed that it compiles ;) and that the test fails before the code-change & passes after.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: keep nsFileDataURI from QI'ing to nsSimpleURI

(cleaned up the test in patch 2 slightly, refactoring the [in]equality checks out into a helper method)

Comment 9

[Daniel Holbert [:dholbert]]

Landed patches 0 and 1, since they don't really depend on patch 2 at all.
http://hg.mozilla.org/projects/cedar/rev/2d6730eb0852
http://hg.mozilla.org/projects/cedar/rev/0173afbf1e62

Will land patch 2 once it's gotten r+.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535724 [details] [diff] [review]
patch 2 v2: keep nsFileDataURI from QI'ing to nsSimpleURI

r=me

Comment 11

[Daniel Holbert [:dholbert]]

Landed patch 2 on cedar: http://hg.mozilla.org/projects/cedar/rev/7723395180e1

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/2d6730eb0852
http://hg.mozilla.org/mozilla-central/rev/0173afbf1e62
http://hg.mozilla.org/mozilla-central/rev/7723395180e1

Comment 13

[Ioana (away)]

As visible here [https://tbpl.mozilla.org/php/getParsedLog.php?id=6407277&full=1], the tests for this bug (test_bug660066.js and test_URIs.js) have passed.