search

olivernn / lunr.js

A bit like Solr, but much smaller and not as bright
http://lunrjs.com
MIT License
8.89k stars 545 forks source link

Fix issue #279 (bug with Safari) #361

Closed lucaong closed 6 years ago

lucaong commented 6 years ago

Calling any method defined on String.prototype on the expanded term forces the string to be properly represented, fixing an issue affecting Safari users.

See https://github.com/olivernn/lunr.js/issues/279

lucaong commented 6 years ago

See the discussion on https://github.com/olivernn/lunr.js/pull/359

I think that this fix would be less invasive, as it's very inexpensive for performance, and does not change the expanded term (other than forcing the correct string representation).

As for the underlying causes of this issue, it does not seem to be related to unicode, but more to some low-level detail of the string representation. It would be worth looking deeper into lunr.TokenSet to find exactly where the string gets corrupted.

lucaong commented 6 years ago

Repeating here my comment on the issue, for documentation:

I think I managed to pinpoint the exact place in lunr.TokenSet where the string gets sometimes corrupted in Safari:

https://jsfiddle.net/egLzL24L/77/ (see line 38 and relevant comment)

Which corresponds to this line in the repo: https://github.com/olivernn/lunr.js/blob/f9aeea2c592dc08eec67937bcb5b34903e6c04dc/lib/token_set.js#L309

It seems that the string concatenation (no matter if done with .concat or with +) results in the corruption, which is fixed by calling any String.prototype method on the string. The two strings that get concatenated look fine, only the result is corrupted.

As of why this happens, and what exactly triggers it, I have no clue. I would be inclined to think that some underlying memory optimization of string concatenation is buggy in Safari in some corner case.

olivernn commented 6 years ago

Again, thanks for doing a deep investigation on this, great work!

I'm happy to put together a release with this workaround to unblock people who are being caught by this issue, just a couple of things:

lucaong commented 6 years ago

I removed changes from lunr.js. Tomorrow I might have some time to also write a test.

olivernn commented 6 years ago

I've squashed and merged these changes now.

As a general rule tests are normally great. Having some specifically for safari would be good, though not essential to get this merged. Its unlikely that I will remember to run them in a browser, plus currently there is no browser tests running on travis.

I'm going to start work on improving unicode support within lunr, as such I will introduce a bunch of tests with documents containing non-ascii characters. In my mind this work should allow us to revert this change.

Another lesson from this is to get browser based tests running as part of the build. Browser bugs, though rare, do exist and can cause a lot of disruption.