RustURL's SchemeIs should do a case insensitive string comparison

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Updated

2 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 1

2 years ago
Created attachment 8838816 [details] [diff] [review]
Perform a case insensitive string comparison in RustURL::SchemeIs()
Attachment #8838816 - Flags: review?(mcmanus)
Attachment #8838816 - Flags: review?(mcmanus) → review+

Comment 2

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/239f0df955d1
Perform a case insensitive string comparison in RustURL::SchemeIs(); r=mcmanus
The scheme of an `Url` object is always lower-cased by the parser, so if `SchemeIs` is only used with literal strings that are also lowercase, then the case-sensitivity of the comparison doesn’t change anything.

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/239f0df955d1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 5

2 years ago
I'm with Simon, are we sure this was a good change? Or do we have legacy callers that don't provide lowercase input?
Ehsan, was there a specific bug that motivated this change?
Flags: needinfo?(ehsan)
(Assignee)

Comment 7

2 years ago
(In reply to Simon Sapin (:SimonSapin) from comment #3)
> The scheme of an `Url` object is always lower-cased by the parser, so if
> `SchemeIs` is only used with literal strings that are also lowercase, then
> the case-sensitivity of the comparison doesn’t change anything.

There is nothing mandating what kind of strings can be passed to SchemeIs.  But yes, if you pass a lower case string to that function then this patch won't change anything.  The test that landed with it should have made this quite clear.

(In reply to Anne (:annevk) from comment #5)
> I'm with Simon, are we sure this was a good change?

Yes.  Any implementation of nsIURI needs to adhere to the same API contract.  If we think that an API contract needs to be changed, such a change needs to be made to all nsIURI implementations.  RustURL doesn't get to implement its own semantics.

(In reply to Simon Sapin (:SimonSapin) from comment #6)
> Ehsan, was there a specific bug that motivated this change?

No, I found this during an audit of the code looking for an unrelated issue.
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.