Last Comment Bug 662094 - WARNING: NS_ENSURE_TRUE(aURI) failed: file nsContentUtils.cpp, line 4706
: WARNING: NS_ENSURE_TRUE(aURI) failed: file nsContentUtils.cpp, line 4706
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks: 640387
  Show dependency treegraph
 
Reported: 2011-06-04 15:35 PDT by Daniel Holbert [:dholbert]
Modified: 2011-06-07 03:49 PDT (History)
4 users (show)
dholbert: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
almost-fix (6.71 KB, patch)
2011-06-05 13:31 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix (1.78 KB, patch)
2011-06-06 10:55 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review
followup to fix nsresult value (1.36 KB, patch)
2011-06-06 13:54 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-06-04 15:35:15 PDT
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.
Comment 1 Daniel Holbert [:dholbert] 2011-06-05 13:31:27 PDT
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 };
Comment 2 Daniel Holbert [:dholbert] 2011-06-05 23:53:27 PDT
(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.
Comment 3 Justin Lebar (not reading bugmail) 2011-06-06 08:32:50 PDT
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.
Comment 4 Daniel Holbert [:dholbert] 2011-06-06 10:55:01 PDT
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 5 Boris Zbarsky [:bz] 2011-06-06 11:00:25 PDT
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....
Comment 6 Daniel Holbert [:dholbert] 2011-06-06 11:06:53 PDT
http://hg.mozilla.org/projects/cedar/rev/e719f1ff8edc
Comment 7 Daniel Holbert [:dholbert] 2011-06-06 13:50:00 PDT
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.)
Comment 8 Daniel Holbert [:dholbert] 2011-06-06 13:54:46 PDT
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 9 Boris Zbarsky [:bz] 2011-06-06 14:24:57 PDT
Comment on attachment 537635 [details] [diff] [review]
followup to fix nsresult value

Er, yes.  I should have caught this.  :(
Comment 10 Daniel Holbert [:dholbert] 2011-06-06 14:48:21 PDT
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.
Comment 11 Mounir Lamouri (:mounir) 2011-06-07 03:49:39 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/e719f1ff8edc
Comment 12 Mounir Lamouri (:mounir) 2011-06-07 03:49:54 PDT
and the follow-up:
http://hg.mozilla.org/mozilla-central/rev/61bc35b9ea8a

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