nsStandardURL::Resolve calls ParseURL with the length of flat, which was the original length and not the actual length if the string happened to have characters removed in the call to FilterString.

This is the obvious fix to the problem. A better one would be for FilterString to additionally return the length of the string it returned.

Comment on attachment 99977 [details] [diff] [review] Correctness patch thx for noticing this! it might even cause a crash if we were looking for a delim but never found it. as for the patch, this extra call to strlen will cost us. Resolve is called _a lot_ during page load. you should instead check to see if |relpath == flat.get()| and if not use buf.Length() instead (a local variable called relpathLen might be nice).

There may be another issue as well. in nsBaseURLParser::ParseURL the line below reverse iterates the string. I think the termination check of *p > 0 is wrong. Shouldn't this test p != spec instead? Also moving *p <= ' ' check to be first might yield a tiny performance improvement. for (p = spec + specLen - 1; (*p > 0) && (*p <= ' '); --p);

yup... nice catch!!

This patch should fix both of David's reported problems. I also added a blank line under the for loop in nsBaseURLParser::ParseURL, otherwise it's really easy to miss the semicolon at the end of that line and read the following line as the mis-indented body of the for loop. I knew the call to strlen would cost too much - I really just wanted to make sure of the logic we needed.

I also considered changing FilterString to accept and return a nsAFlatString& rather than a raw pointer. This does make the use of FilterString more logical, no longer mixing raw pointers and string classes in the function call Offered here for comment. I must note that I haven't been able to test either of these patches. My build environment is horked -- and it's late.

This is a variation on the second patch posted. This test's the length of buf. Length is a cheaper call than get. In a decenet compiler Length will get inlined even though virtual since it's type is concrete. Your third patch has a couple of problems. You need to use *CString classes and FilterString's return generates a temporary on return which causes an error since you're returning a reference.

Comment on attachment 100080 [details] [diff] [review] YAP >+ rv = mParser->ParseURL(relpath, >+ buf.Length() == 0 ? flat.Length() : buf.Length(), r/sr=darin (though it might make sense to declare a local variable called, say, relpathLen that'll help document what you're doing here. it could put up top near the declaration of relpath itself.)

Good point. The lack of documentation on that clause made me wince, but I was too tired to come up with a better way. I went ahead and created a new one. It's conceptially the same, but should really get at least a second look at the variable declaration to make sure I got the boolean logic correct.

Comment on attachment 100098 [details] [diff] [review] Same patch but with Darin's suggestion sr=darin

Ping. Are we going to try to get this fixed for 1.2?

Comment on attachment 100098 [details] [diff] [review] Same patch but with Darin's suggestion r=dougt

Comment on attachment 100098 [details] [diff] [review] Same patch but with Darin's suggestion I urge all empty-loop-body writers to use a separate line for the empty statement, if not use a formally-unnecessary-but-visually-more-obvious continue; as the loop body. Fix that if you agree. a=brendan@mozilla.org for 1.2beta trunk checkin when the tree opens to approved checkins. /be

-> dbradley

fix checked in to trunk Is it worth getting this on the branch?

yes, i think so. nominating.

Discussed blackbird team meeting. Please verify on trunk in order to get adt approval to check in.

verified fixed on trunk

plussing for adt. If other approvals are required please make sure to get them. Need checkin asap.

Tom, please follow up when this lands on the branch. Thanks.

Comment on attachment 100098 [details] [diff] [review] Same patch but with Darin's suggestion This looks good for the 1.0 branch to me, too. a=brendan@mozilla.org on behalf of drivers. /be

Fix checked in to the branch. I checked in the nsBaseURLParser::ParseURL part of the patch. The nsStandardURL.cpp change was not applicable. FilterString only exists on the trunk, so it wasn't an issue on the branch.

adding branch resolution keyword.

verified1.0.2 - patch checked in to branch

this patch seems to have caused regression bug 179026.

this probably also cause 179143