Follow-on noticed in working bug 125608. nsStandardURL::SetPort() allows you to set negative values for mPort other than -1 (which is 'unset'). This would result in an invalid URL.
Should I add this: NS_ABORT_IF_FALSE(mPort > 0 || mPort == -1, "Bogus mPort"); Or return an NS_ERROR? It appears most callers don't check the return value...
Just return an error if a bogus port value is passed in. You can't assert anything about what people will pass to a public API like this.
Created attachment 535274 [details] [diff] [review] Simple patch
Comment on attachment 535274 [details] [diff] [review] Simple patch Please make that |port <= 0 && port != -1| and r=me.
checked in as http://hg.mozilla.org/mozilla-central/rev/0cf4fa02c0f2
Backed out due to perma-orange on XPCshell tests: http://tbpl.mozilla.org/?tree=Firefox&rev=0cf4fa02c0f2
Randell, are you still working on this?
Yes; just not high priority.
Fix was checked in on 6/7/2011 when the other patch (which caused the orange) was checked in as patch http://hg.mozilla.org/mozilla-central/rev/a44485cd1682
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 Could you provide a clear set of STRs or a test case in order to verify this issue in QA?
Created attachment 554497 [details] [diff] [review] Test for negative ports You'd need to write a short program to create a URL and then do SetPort(-10) on it. I've attached a patch to test_URIs.js that tests the behavior - it passes on current trunk. It should fail without this fix. Note that test_URIs.js changed a fair bit, so you'll probably have to merge it by hand only an older one (should be easy).
qa+ for verification on Firefox 7 using comment 11.
Based on comment 11, I don't think QA can easily verify this bug fix. If someone has the time to code and attach the "short program" required to verify this fix, please do so. Thanks
Anthony - I think just applying that testcode patch onto an older test_URIs.js would let you test it ("cd checkoutdir; patch -p1 </tmp/attachment_554497"), then run the xpcshell tests - This should do it: $ cd objdir $ make SOLO_FILE="test_URIs.js" -C netwerk/test/ check-one
Adding qa+ for this to be re-evaluated for verification; if I misunderstood the reasoning please change back
Resetting this to qa? so we can evaluate if this is doable by QA. I suspect not, given the demands of the rapid release schedule. I would appreciate if someone who is already set up to test as per comment 15 could verify this fix.
Comment on attachment 566458 [details] [diff] [review] Add negative ports in URLs to tests My only issue with adding this is: do we really need test setting the port for all those (absolute) http URLs? But it's likely down in the noise. Also, probably a new bug should be opened to properly track this test as it's checked in.
Comment on attachment 566458 [details] [diff] [review] Add negative ports in URLs to tests r=me