One of the very first lines of terminal output I get, when starting up a debug build (both fresh profile & non, I think), is: { WARNING: NS_ENSURE_TRUE(aURI) failed: file mozilla/content/base/src/nsContentUtils.cpp, line 4706 } This is an early-return from nsContentUtils::SplitURIAtHash, due to a null parameter being passed in. There's no reason to spam a warning for this -- it's actually an entirely expected situation, as noted in the caller in this case: > 8287 // Split mCurrentURI and aURI on the '#' character. Make sure we read > 8288 // the return values of SplitURIAtHash; it might fail because > 8289 // mCurrentURI is null, for instance, and we don't want to allow a > 8290 // short-circuited navigation in that case. [...] > 8293 splitRv1 = nsContentUtils::SplitURIAtHash(mCurrentURI, curBeforeHash, curHash); http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8287 We could replace the NS_ENSURE_TRUE with a simple if-check/early-return, but it looks like we actually can replace this call entirely using the new method nsIURI::EqualsExceptRef added in bug 308590.
Created attachment 537476 [details] [diff] [review] almost-fix (In reply to comment #0) > but it looks like we actually can replace this call entirely using the new > method nsIURI::EqualsExceptRef added in bug 308590. This patch does that, but it doesn't quite work -- it fails these mochitest checks: > test_bug465263.html | Should have '#' in href now - got "http://mochi.test:8888/", expected "http://mochi.test:8888/#" > test_bug465263.html | Should have only one '#' in href - got "http://mochi.test:8888/", expected "http://mochi.test:8888/#" The old code did this: - split URIs into base-string and "#suffix" - do comparisons on those ...which notably will catch the difference between the suffix "#" and the suffix "". (no-hash vs. just-a-hash) My patch fails on that notable case. I do the initial comparison using "equalsExceptRef", and then I use GetRef to get stuff _after_ the "#". (which returns the empty string for *both* the no-hash and just-a-hash cases) We could work around this in a few ways: (A) use nsIURI::equals() (after equalsExceptRef) to determine whether the "#ref" parts differ. (Drawback: This would make us unnecessarily recompare everything up to the "#" in the equals() call.) (B) Have a special-case for both-refs-are-empty, and in that case, just check whether either URI.spec ends with a "#" character. I favor (B), abstracted out into something like nsContentUtils::CompareURIs(), which returns an enumerated type: enum URIComparisonResult { eURIBasesDiffer, eURIRefsDiffer, eURIsEqual };
(In reply to comment #1) > (B) Have a special-case for both-refs-are-empty, and in that case, just > check whether either URI.spec ends with a "#" character. Oh, I forgot that you can't inspect URI.spec without string-copying the full URI into GetSpec()'s outparam. So (B) isn't quite as cheap as I'd thought, in the both-refs-empty-or-"#" case.
I'm not so concerned with speed here; this code gets run just once per pageload. Correctness is really tricky, though. See also bug 662170.
Created attachment 537589 [details] [diff] [review] fix As noted in bug 662170, we also need the ability to distinguish ends-in-# vs. no-#-at-all for a single URI (not just for the purposes of ref-equality comparisons). This isn't covered by my proposal at the end of comment 1. I'm thinking more and more that we need nsIURI::HasRef(), which would return true if there's a "#" (even if there's nothing after it). That can be done elsewhere, though (e.g. on bug 225910). As for the original issue here (spamming a new warning for NS_ENSURE_* failure at startup), here's a simple patch.
Comment on attachment 537589 [details] [diff] [review] fix r=me I really wish we could make GetRef() return empty string for ending in '#' and void string for no '#' at all....
http://hg.mozilla.org/projects/cedar/rev/e719f1ff8edc
Comment on attachment 537589 [details] [diff] [review] fix >- splitRv1 = nsContentUtils::SplitURIAtHash(mCurrentURI, curBeforeHash, curHash); >+ splitRv1 = mCurrentURI ? >+ nsContentUtils::SplitURIAtHash(mCurrentURI, >+ curBeforeHash, curHash) : NS_OK; [...] > PRBool sameExceptHashes = NS_SUCCEEDED(splitRv1) && > NS_SUCCEEDED(splitRv2) && > curBeforeHash.Equals(newBeforeHash); d'oh, sorry -- to preserve the prior logic, I think I need ": NS_ERROR_FAILURE" there, not ": NS_OK". With bz's thumbs-up, I'll push a followup to fix that. (This has actually gotten a green full test-run on a few platforms on Cedar, btw, so our testsuite doesn't catch this bug... I'm not immediately sure what sort of testcase would hit this bug -- mCurrentURI would need to be nsnull and aURI would need to have an empty ref. I'd like to add a testcase that would've caught this bug.)
Created attachment 537635 [details] [diff] [review] followup to fix nsresult value I can figure out a testcase later on, but I'd like to fix the logic ASAP, before the next cedar merge.
Comment on attachment 537635 [details] [diff] [review] followup to fix nsresult value Er, yes. I should have caught this. :(
Pushed followup to cedar: http://hg.mozilla.org/projects/cedar/rev/61bc35b9ea8a Ticking in-tetstuite? flag to remind myself to add a test for the followup.
Pushed: http://hg.mozilla.org/mozilla-central/rev/e719f1ff8edc
and the follow-up: http://hg.mozilla.org/mozilla-central/rev/61bc35b9ea8a
or
