Last Comment Bug 660066 - Crash when comparing a moz-filedata: URI with null principal
: Crash when comparing a moz-filedata: URI with null principal
Status: VERIFIED FIXED
: crash
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks: 543870
  Show dependency treegraph
 
Reported: 2011-05-26 13:04 PDT by Daniel Holbert [:dholbert]
Modified: 2011-09-15 07:33 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0: add more test_URIs.js testcases (4.03 KB, patch)
2011-05-26 14:21 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review
patch 1: fix + test for this bug (3.59 KB, patch)
2011-05-26 14:28 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review
patch 2: keep nsFileDataURI from QI'ing to nsSimpleURI (2.63 KB, patch)
2011-05-26 23:54 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
patch 2 v2: keep nsFileDataURI from QI'ing to nsSimpleURI (3.70 KB, patch)
2011-05-27 13:13 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-05-26 13:04:08 PDT
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] 2011-05-26 14:21:59 PDT
Created attachment 535471 [details] [diff] [review]
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] 2011-05-26 14:28:37 PDT
Created attachment 535476 [details] [diff] [review]
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 [:bz] 2011-05-26 18:44:28 PDT
Comment on attachment 535471 [details] [diff] [review]
patch 0: add more test_URIs.js testcases

r=me
Comment 4 Boris Zbarsky [:bz] 2011-05-26 18:47:29 PDT
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] 2011-05-26 20:10:11 PDT
(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] 2011-05-26 23:02:54 PDT
(er s/dom/content/)
Comment 7 Daniel Holbert [:dholbert] 2011-05-26 23:54:46 PDT
Created attachment 535568 [details] [diff] [review]
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] 2011-05-27 13:13:16 PDT
Created attachment 535724 [details] [diff] [review]
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] 2011-05-27 16:56:51 PDT
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 [:bz] 2011-05-27 20:19:10 PDT
Comment on attachment 535724 [details] [diff] [review]
patch 2 v2: keep nsFileDataURI from QI'ing to nsSimpleURI

r=me
Comment 11 Daniel Holbert [:dholbert] 2011-05-27 21:56:56 PDT
Landed patch 2 on cedar: http://hg.mozilla.org/projects/cedar/rev/7723395180e1
Comment 13 Ioana (away) 2011-09-15 07:33:40 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.