Bug 139076 - REGRESSION(r173188): Text inserted when trying to delete a word from the Twitter message box
Summary: REGRESSION(r173188): Text inserted when trying to delete a word from the Twit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Gtk, Regression
Depends on:
Blocks:
 
Reported: 2014-11-27 06:21 PST by Alberto Garcia
Modified: 2014-12-04 16:55 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.08 KB, patch)
2014-12-04 14:09 PST, Andreas Kling
ggaren: review+
Details | Formatted Diff | Diff
Patch II (2.68 KB, patch)
2014-12-04 15:57 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch II (2.68 KB, patch)
2014-12-04 15:58 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 2014-11-27 06:21:22 PST
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.
Comment 1 Andreas Kling 2014-11-27 14:23:17 PST
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.
Comment 2 Alberto Garcia 2014-11-28 00:52:24 PST
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
Comment 3 Andreas Kling 2014-11-28 04:08:37 PST
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?
Comment 4 Alberto Garcia 2014-11-28 04:52:50 PST
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 ".
Comment 5 Michael Catanzaro 2014-12-03 15:03:54 PST
(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.
Comment 6 Gustavo Noronha (kov) 2014-12-04 02:23:45 PST
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.
Comment 7 Andreas Kling 2014-12-04 12:00:52 PST
FYI I am actively working on this bug.
Comment 8 Andreas Kling 2014-12-04 14:09:16 PST
Created attachment 242587 [details]
Patch
Comment 9 Geoffrey Garen 2014-12-04 14:29:27 PST
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.
Comment 10 Geoffrey Garen 2014-12-04 14:43:13 PST
Seems like we need to do something about our other 2 StringImpl=>JSString maps, since they are technically not safe.
Comment 11 Andreas Kling 2014-12-04 15:57:17 PST
Created attachment 242593 [details]
Patch II

Less scary solution.
Comment 12 Andreas Kling 2014-12-04 15:58:02 PST
Created attachment 242594 [details]
Patch II
Comment 13 Geoffrey Garen 2014-12-04 15:59:06 PST
Comment on attachment 242594 [details]
Patch II

r=me -- even better!
Comment 14 WebKit Commit Bot 2014-12-04 16:55:01 PST
Comment on attachment 242594 [details]
Patch II

Clearing flags on attachment: 242594

Committed r176824: <http://trac.webkit.org/changeset/176824>
Comment 15 WebKit Commit Bot 2014-12-04 16:55:08 PST
All reviewed patches have been landed.  Closing bug.