Closed Bug 662094 Opened 11 years ago Closed 11 years ago

WARNING: NS_ENSURE_TRUE(aURI) failed: file nsContentUtils.cpp, line 4706

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 640387
Attached patch almost-fix (obsolete) — Splinter Review
(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.
Attached patch fixSplinter Review
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.
Attachment #537476 - Attachment is obsolete: true
Attachment #537589 - Flags: review?(bzbarsky)
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....
Attachment #537589 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/projects/cedar/rev/e719f1ff8edc
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-cedar]
OS: Linux → All
Hardware: x86_64 → All
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.)
I can figure out a testcase later on, but I'd like to fix the logic ASAP, before the next cedar merge.
Attachment #537635 - Flags: review?(bzbarsky)
Comment on attachment 537635 [details] [diff] [review]
followup to fix nsresult value

Er, yes.  I should have caught this.  :(
Attachment #537635 - Flags: review?(bzbarsky) → review+
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.
Flags: in-testsuite?
Pushed:
http://hg.mozilla.org/mozilla-central/rev/e719f1ff8edc
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.