Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removingHTMLEntities will throw fatal error if decode JSONSerialization string #22

Closed
tomcheung opened this issue May 21, 2019 · 4 comments

Comments

@tomcheung
Copy link
Contributor

I am using library with Xcode 10.2.1 (Swift 5), and found following code will throw Fatal error inside removeHTMLEntities

let jsonString = "{\"sepcialCharacter\": \"𝟸𝟺𝟶°\"}"
let json = try! JSONSerialization.jsonObject(with: jsonString.data(using: .utf8)!, options: []) as! [String:String]

let sepcialCharacter = json["sepcialCharacter"]!
sepcialCharacter.removingHTMLEntities

After investigation, seem this error may cause by NSString to swift String bridging with following code, not sure this is the bug in swift or the library itself

let nsSepcialCharacter = NSString("𝟸𝟺𝟶°")
let sepcialCharacter = nsSepcialCharacter as String
sepcialCharacter.removingHTMLEntities
@randarnold
Copy link

I have the same issue.
iOS 12.3, Xcode 10.2.1
Using the Master branch as the pod still points to 4.0.2
4.0.2 does not crash here

I'm calling removingHTMLEntities() on an NSString
These two strings crash every time, with "Fatal error:" at

#0 0x00000001f1c65194 in _swift_runtime_on_report ()
#1 0x00000001f1cbb81c in swift_stdlib_reportFatalError ()
#2 0x00000001f1bdfb00 in specialized fatalErrorMessage(:
:file:line🎏) ()
#3 0x00000001f1be510c in specialized BidirectionalCollection._distance(from:to:) ()
#4 0x00000001f1be56e0 in specialized String.UTF16View._nativeGetOffset(for:) ()
#5 0x00000001f1b4b28c in StringProtocol.toUTF16Offsets(:) ()
#6 0x00000001f1e0a1d4 in StringProtocol.range(of:options:range:locale:) ()
#7 0x0000000101b44310 in String.removeHTMLEntities() at Pods/HTMLString/Sources/HTMLString/HTMLString.swift:146

"Pomp, circumstance and celebrity: people-watching in Windsor – a cartoon"

"

\n \n

Disney needs three wishes for its live-action "Aladdin" remake: for a better Genie, a better script and some soul.

\n

Like the 1992 animated classic, "Aladdin" tells the story of its titular impoverished thief (Mena Massoud) who falls in love with the beautiful Princess Jasmine (Naomi Scott) while she explores the Middle Eastern city of Agrabah using the name of her handmaiden Dalia (Nasim Pedrad). In order to win Jasmine's heart, Aladdin agrees to enter the mysterious Cave of Wonders (Frank Welker, the only voice actor from the main cast of the original to reprise his role here) and acquire a magic lamp for the sinister royal vizier Jafar (Marwan Kenzari), but is soon betrayed and left for dead. Fortunately for him, his pet monkey Abu (Welker again) stole the lamp from Jafar before the betrayal, allowing Aladdin to discover the genie imprisoned inside (Will Smith) and receive three wishes in return.

\n

There are a few things to like about "Aladdin." Whereas the animated movie incurred controversy for having its protagonists look Caucasian, director Guy Ritchie (who co-wrote the script with John August) chose non-white actors to fill the lead hero roles. The film also has a handful of decent moments, such as when Smith and Massoud are allowed to improvise a humorously embarrassing introduction between Massoud's "Prince Ali" alias and Jasmine, or when Scott is able to develop Jasmine into a political idealist with a character arc beyond simply being allowed to choose her own husband.

\n

These are not inconsequential details. Unfortunately they are significantly outweighed by the film's flaws.

\n

Foremost among them is Will Smith, who is terribly miscast as the Genie. The problem is the same obvious one that online commenters pointed out since the day his casting was announced, and doubled down on when seeing him for the first time in the ominous trailers: He is no Robin Williams, not even close. Robin Williams' performance in the original movie was iconic and inimitable. His boundless energy, manic comic verve and overall brilliant showmanship were more responsible than any other single factor for making that film into a classic. This isn't to say that an "Aladdin" remake could not work with a different Genie, but it would need to either find an actor with talents similar to Williams' or go in a completely different direction with the character.

\n

While Smith is charismatic and charming, he is nowhere near the comedy virtuoso as Williams, so Option A was never realistic for him — and, unfortunately, that's the direction in which "Aladdin" for the most part chooses to go. I cringed to hear Smith attempt the same lines that Williams improvised in the original film, which fall flat when stripped of Williams' organic enthusiasm. The same can even be said of the musical numbers; Williams belted them out in a manner that felt both cartoonish and epic, while Smith seems to be managing as best he can. Those are fatal shortcomings here, since the humor and showstopping tunes were the greatest strengths of the original.

\n

The second major problem with "Aladdin" is the script by Ritchie and August. Despite being half an hour longer than the original movie, major parts of the story here feel rushed, particularly the original meeting between Aladdin and Jasmine. Other elements that were added to distinguish this story from the original movie feel either tacked on or underdeveloped. This movie gives the Genie a romantic subplot with Dalia that could have been charming — Smith is nothing if not a likable romantic lead — but is barely allowed any screen time to develop. The same is true of Jafar, who gets a hinted-at tragic backstory meant to explain his craving for respect and power. This could have made him into a complex villain, particularly in an early scene when he attempts to commiserate with Aladdin — but, again, that is quickly dropped and only referred to briefly later on.

\n

The underlying issue here is that "Aladdin" is a movie that shouldn't exist. This isn't because remakes are in and of themselves a bad thing — plenty of great movies are actually remakes of earlier films that were also quite good, or even great themselves. Yet Disney isn't churning out these live action versions of animated classics like "The Jungle Book" and "Beauty and the Beast" because there is some deep creative yearning among the filmmakers to retell those stories. It is doing so because those movies make bank at the box office, so it's easy to lure in audiences if you have a beloved title and properly alluring stars.

\n

Yet none of those things guarantee that a movie will have a soul. If a remake has a fundamental creative spirit, it can transcend the limitations of being a copy and instead feel like an enjoyable independent entity of its own. Without that vision, it winds up as nothing more than spectacle, a novelty that cashes in on a better movie without having anything meaningful to offer on its own.

\n

In that sense, the live-action "Aladdin" epitomizes Hollywood's worst impulses: it's a disposable product, a commodity, a cinematic non-entity whose sole purpose is to turn a profit. Few people are naive enough to believe that Hollywood studios create movies solely for their artistic value, but the best films are able to balance their financial imperatives with a genuine vision. This one is so nakedly commercial in its conception that, even if it wasn't part of a glut of live-action adaptations (a live action "Dumbo" remake came out earlier this year and a live-action version of "The Lion King" is due later), it would still feel cynical and cloying.

"

@tomcheung
Copy link
Contributor Author

https://github.com/alexaubry/HTMLString/blob/b6f82382bba2b5b9131eb8634d41d19a8968cf20/Sources/HTMLString/HTMLString.swift#L186-L187

Seem the issue can be fixed by modify the code above to

    let data = self.data(using: .utf8)
    var copy = String(data: data!, encoding: .utf8)!

However, not sure this workaround is appropriate solution, or have better solution available. If this fix is okay, I will make the pull request for it.

@randarnold
Copy link

randarnold commented Jun 21, 2019

Safer would be:

var copy = self
if let data = self.data(using .utf8) {
copy = String(data: data, encoding: .utf8)
copy.removeHTMLEntities()
}

return copy

Although some indication of failure would be good. Throw?

@tomcheung
Copy link
Contributor Author

@randarnold Just found more appropriate solution instead of using workaround mention in #22 (comment) and related PR is made.

I think there are some issue when calling range:of:range with specify range after calling replaceSubrange

let nsSepcialCharacter = NSString("𝟸𝟺𝟶°")
//let nsSepcialCharacter = String("𝟸𝟺𝟶°") // No error if use Swift String, but will crash if convert NSString to String
var sepcialCharacter = nsSepcialCharacter as String

var cursorPosition = sepcialCharacter.startIndex
let delimiterRange = sepcialCharacter.range(of: "&", range: cursorPosition ..< sepcialCharacter.endIndex)!
let semicolonRange = sepcialCharacter.range(of: ";", range: delimiterRange.upperBound ..< sepcialCharacter.endIndex)!

let replaceableRange = delimiterRange.lowerBound ..< semicolonRange.upperBound

sepcialCharacter.replaceSubrange(replaceableRange, with: "°")
sepcialCharacter.range(of: "&", range: delimiterRange.lowerBound ..< sepcialCharacter.endIndex)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants