REGRESSION(r173188): Text inserted when trying to delete a word from the Twitter message box

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alberto Garcia]

Go to Twitter (you need an account), type a few words in the "What's happening?" box and then try to delete them using the backspace button.

At some point the whole text you're deleting is replaced by " #document ". If you try to delete that as well you'll see more glitches.

I can reproduce this with the MiniBrowser in the GTK+ port. It looks like this was a regression introduced in r173188 and it still happens now (r176549). I can still revert the change cleanly and it seems to fix the problem.

[Andreas Kling]

This is very slippery, but I managed to reproduce it once so far when using Safari 7.1 user-agent string. Will try to debug.

If you've seen any assertions when this happens, that would be very helpful.

[Alberto Garcia]

Hey, I just tried a debug build and I cannot see any assertions or any other messages.

In my case I can reproduce it easily in 100% of the cases, here's my user agent in case it's relevant:

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/601.1 (KHTML, like Gecko) Version/8.0 Safari/601.1

[Andreas Kling]

I still can't reproduce this reliably. The times I did succeed it seemed to happen when transitioning between empty/nonempty text field. Is that the case for you as well?

Could you describe the repro steps in detail just in case I'm misinterpreting your description?

[Alberto Garcia]

Sure, here they are:

- Open the MiniBrowser
- Go to https://twitter.com/ and log in
- On top of the timeline there's a box with the "What's happening?" message.
- Click there, the message disappears so you can enter your tweet.
- Type "foo bar baz".
- Press backspace a few times, it will start deleting the letters one by one but at some point the whole text is replaced by " #document ".

[Michael Catanzaro]

(In reply to comment #0)
> It looks like
> this was a regression introduced in r173188 and it still happens now
> (r176549). I can still revert the change cleanly and it seems to fix the
> problem.

Hey Carlos, this would probably be a good one to revert for the 2.6 branch.

[Gustavo Noronha (kov)]

This happens a lot in GTK+ in both Twitter and Etherpads, but it is not just related to deleting, it may happen at any point while typing text. Unfortunately I couldn't find a simple way to reproduce it reliably.

[Andreas Kling]

FYI I am actively working on this bug.

[Andreas Kling]

Created attachment 242587 [details]
Patch

[Geoffrey Garen]

Comment on attachment 242587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242587&action=review

r=me

> Source/JavaScriptCore/runtime/JSString.cpp:447
> +        // NOTE: If the StringImpl* key no longer matches the StringImpl* inside the cached JSString,
> +        //       it's because the JSString had its StringImpl replaced with an atomic version.
> +        //       This causes trouble when a new StringImpl gets allocated at exactly the same address
> +        //       as a previously deleted one.
> +        //       Leaving stale keys in the string cache is otherwise safe, so we manage this issue
> +        //       here on the slow path instead of removing things from the cache on the hot path.

I think I would simplify this comment and just say something like: A JSString is allowed to change its underlying StringImpl*, so we need to double-check that the JSString in the cache still matches the StringImpl* in the cache.

No need to restrict the definition of when JSString might change itself. It do what it want.

[Geoffrey Garen]

Seems like we need to do something about our other 2 StringImpl=>JSString maps, since they are technically not safe.

[Andreas Kling]

Created attachment 242593 [details]
Patch II

Less scary solution.

[Andreas Kling]

Created attachment 242594 [details]
Patch II

[Geoffrey Garen]

Comment on attachment 242594 [details]
Patch II

r=me -- even better!

[WebKit Commit Bot]

Comment on attachment 242594 [details]
Patch II

Clearing flags on attachment: 242594

Committed r176824: <http://trac.webkit.org/changeset/176824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.