stuart: http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#462 stuart: that NS_ConvertUTF8toUTF16 is generating a lot of assertions the assertions: ###!!! ASSERTION: not a UTF8 string: 'Error', file c:\builds\mozilla\objdir\dist \include\string\nsUTF8Utils.h, line 130 ###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file c:/builds/mozilla/objdir/xpcom/string/src/../../../../xpcom/string/src/nsR eadableUtils.cpp, line 287 ###!!! ASSERTION: not a UTF8 string: 'Error', file c:\builds\mozilla\objdir\dist \include\string\nsUTF8Utils.h, line 130 ###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file c:/builds/mozilla/objdir/xpcom/string/src/../../../../xpcom/string/src/nsR eadableUtils.cpp, line 287

Do you know what strings in particular it's complaining about? NS_ConvertUTF16toUTF8 is being called with the output of a NS_UnescapeURL which should be be UTF8. Is it that the length is wrong? http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/io/nsEscape.h&rev=1.30&mark=238-246#238 241 inline nsCString & 242 NS_UnescapeURL(nsCString &str) 243 { 244 str.SetLength(nsUnescapeCount(str.BeginWriting())); 245 return str; 246 }

So the issue is with URIs that have things that look like html escaped characters. One example: http://site/%EAid data:text/html;charset=utf-8,bad:%EA,good:%E4%BB%BB The unescaping doesn't seem to mind, but NS_ConvertUTF8toUTF16 doesn't seem to like the resulting unescaped string.

I think this code should decode the URL the same way the URL bar does. See URLBarSetURI in browser.js. It basically tries to call decodeURI() on the URL and if that fails (e.g. when the URL contains escape sequences that do not represent UTF8-encoded characters like the example in comment 2) it falls back to using the non-decoded URL.

Oh thanks for commenting. Some reason I had this in my stack of patches, but never got around to attaching it here. Use nsITextToSubURI which will avoid the assert as well as do some loop invariant code motion by only unescaping when doing QUERY_FULL searches.

Comment on attachment 311286 [details] [diff] [review] v1 >+ // Fallback on using this if the service is unavailable for some reason is there a concrete reason why it would be unavailable or is this just a sanity check?

(In reply to comment #5) > >+ // Fallback on using this if the service is unavailable for some reason > is there a concrete reason why it would be unavailable or is this just a sanity > check? Just a sanity check. Should I just NS_ASSERT that we have the service?

Just some name changes to be more flexible for changes down the line (e.g., bug 422698, bug 424717, and bug 424509).

Comment on attachment 311608 [details] [diff] [review] v1.1 Get rid of some assertions :)

Comment on attachment 311608 [details] [diff] [review] v1.1 Please re-request approval once the following information is included. When requesting approvals, please: * Include an assessment of the risk associated with this patch. * Give a sentence or two why it's so important to take your patch this late in the release process. * Included tests so we don't have to ask for them.

Comment on attachment 311608 [details] [diff] [review] v1.1 (In reply to comment #9) > * Include an assessment of the risk associated with this patch. At worst it does what it does right now and at best/common case is there won't be any assertions in debug builds. > * Give a sentence or two why it's so important to take your patch this late in > the release process. Not hitting assertions sounds like a good idea. And this patch is needed for a wanted-firefox3 bug 424509. > * Included tests so we don't have to ask for them. Are there tests for debug builds? Even so, is there a way to check for assertions?

Well.. I thought about it a little bit more and came up with a testcase. :)

Comment on attachment 313757 [details] [diff] [review] v1.2 a=beltzner, yay, tests!

Checking in toolkit/components/places/src/Makefile.in; /cvsroot/mozilla/toolkit/components/places/src/Makefile.in,v <-- Makefile.in new revision: 1.44; previous revision: 1.43 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h new revision: 1.155; previous revision: 1.154 done Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v <-- nsNavHistoryAutoComplete.cpp new revision: 1.55; previous revision: 1.54 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_422277.js,v done Checking in toolkit/components/places/tests/unit/test_422277.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_422277.js,v <-- test_422277.js initial revision: 1.1 done