Skip to content
This repository has been archived by the owner on Mar 6, 2024. It is now read-only.

Fix #2869 - never return nil as the base domain #2940

Merged
merged 4 commits into from
Jan 7, 2022
Merged

Fix #2869 - never return nil as the base domain #2940

merged 4 commits into from
Jan 7, 2022

Conversation

tsekityam
Copy link
Contributor

When the host is an IP address, the baseDomain will be nil. However, the UI fails to handle nil properly and crashes.

I think the best way to fix this issue is to not return nil in the baseDomain.

This PR updates baseDomain function, such that it

  1. returns host if host is an ip address (in ipv4 or ipv6)
  2. returns host if host is not FQDN
  3. returns the domain name from host if publicSuffixFromHost can extract it, otherwise returns host

As a result, the UI will not be crashed because of the nil baseDomain

@tsekityam tsekityam requested a review from a team as a code owner January 4, 2022 16:16
@st3fan st3fan linked an issue Jan 4, 2022 that may be closed by this pull request
Clicking on the connection security icon make Focus crashes if we're connected to an ip adress #2869
Closed
@st3fan
Copy link
Contributor

st3fan commented Jan 4, 2022

@tsekityam thank you for this patch! we are very close to code freeze for Focus 96, so I am not sure if this will ship in that release. But we will definitely pick this up for 97.

@st3fan st3fan added this to the Focus/Klar v96.0 milestone Jan 7, 2022
st3fan
st3fan approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@st3fan st3fan left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I am going to file a followup issue to improve the isIPv4 and isIPv6 functions.

#2949

@mergify mergify bot merged commit e2dc3a3 into mozilla-mobile:main Jan 7, 2022
@st3fan
Copy link
Contributor

st3fan commented Jan 7, 2022

@Mergifyio backport releases_v96

@mergify mergify bot mentioned this pull request Jan 7, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Jan 7, 2022

backport releases_v96

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 7, 2022
@tsekityam
Fix #2869 - never return nil as the base domain (#2940)
0b97a36 
* Fix #2869 - never return nil as the base domain

* Return host if failed to extract the domain

Co-authored-by: Stefan Arentz <[email protected]>
(cherry picked from commit e2dc3a3)
mergify bot added a commit that referenced this pull request Jan 7, 2022
@mergify @tsekityam
Fix #2869 - never return nil as the base domain (#2940) (#2950)
46c9b9b 
* Fix #2869 - never return nil as the base domain

* Return host if failed to extract the domain

Co-authored-by: Stefan Arentz <[email protected]>
(cherry picked from commit e2dc3a3)

Co-authored-by: Tse Kit Yam <[email protected]>
isabelrios pushed a commit to isabelrios/firefox-ios that referenced this pull request Feb 14, 2024
@tsekityam @st3fan
Fix mozilla-mobile/focus-ios#2869 - never return nil as the base doma…
b3ec80c 
…in (mozilla-mobile/focus-ios#2940)

* Fix mozilla-mobile/focus-ios#2869 - never return nil as the base domain

* Return host if failed to extract the domain

Co-authored-by: Stefan Arentz <[email protected]>
isabelrios pushed a commit to isabelrios/firefox-ios that referenced this pull request Feb 20, 2024
@tsekityam @st3fan
[focus-ios] Fix mozilla-mobile/focus-ios#2869 - never return nil as t…
3911479 
…he base domain (mozilla-mobile/focus-ios#2940)

* Fix mozilla-mobile/focus-ios#2869 - never return nil as the base domain

* Return host if failed to extract the domain

Co-authored-by: Stefan Arentz <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@st3fan st3fan st3fan approved these changes

Assignees
No one assigned
Labels
Needs Landing (Squash) Needs Uplift
Projects
None yet
Milestone
Focus/Klar v96.0
Development

Successfully merging this pull request may close these issues.

Clicking on the connection security icon make Focus crashes if we're connected to an ip adress
2 participants
@tsekityam @st3fan