Closed Bug 1274044 Opened 5 years ago Closed 5 years ago

"ASSERTION: Caller should check its passed-in value and pass -1 instead of mDefaultPort..."

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 + fixed

People

(Reporter: jruderman, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [necko-active])

Attachments

(3 files)

Attached file testcase
###!!! ASSERTION: Caller should check its passed-in value and pass -1 instead of mDefaultPort, to avoid encoding default port into mSpec: 'aNewPort != mDefaultPort', file netwerk/base/nsStandardURL.cpp, line 1862

This assertion was added in:

changeset:   https://hg.mozilla.org/mozilla-central/rev/66ff85f4450f
user:        Daniel Holbert
date:        Wed Feb 17 19:24:34 2016 -0800
summary:     Bug 1247733 part 1: Create a helper function for nsStandardURL's code to add/remove/replace a port in the URL string. r=valentin

However, I suspect this bug is more recent.
Attached file stack
URL code - Valentin?
Assignee: nobody → valentin.gosu
Whiteboard: [necko-active]
So the "problem" here is that mDefaultPort is *itself* -1.

The caller (SetPort) is doing what the assertion asks -- it checks if the new port matches the default port -- and it does -- and so it passes -1 instead.  But then it turns out they're all -1, so the assertion complains because it thinks the caller is just passing in the default port without having checked.

The assertion needs a tweak to allow for aNewPort to match mDefaultPort *if* they're both -1.
Assignee: valentin.gosu → dholbert
(note that mDefaultPort gets initialized to -1 in the constructor, so it's not too surprising that it happens to have a value of -1 here.)
The assertion is trying to require that the caller should never pass in mDefaultPort; instead, the caller should pass -1. BUT, if mDefaultPort is itself -1 (i.e. there's no default port), then we need to allow the caller to pass in -1 without spamming an assertion.

Review commit: https://reviewboard.mozilla.org/r/53980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53980/
Attachment #8754463 - Flags: review?(valentin.gosu)
Comment on attachment 8754463 [details]
MozReview Request: Bug 1274044: Relax assertion in nsStandardURL for case when mDefaultPort is -1. r?valentin

https://reviewboard.mozilla.org/r/53980/#review50700
Attachment #8754463 - Flags: review?(valentin.gosu) → review+
dholbert - Does this affect 48 and if so should it be uplifted?
Flags: needinfo?(dholbert)
I think it affects every release that has bug 1247733's fix. However, there's no need to uplift; the bug here is simply that this (debug-only) assertion was a bit too strict, and the fix relaxes the assertion a bit.

The fix has no impact on opt build behavior.
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/385e27c898e6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.