Bug 89562 – Regression(r116408): Ctrl-A (select all) on large text file hangs the tab with high CPU usage

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

What steps will reproduce the problem?
1. Go to https://easylist-downloads.adblockplus.org/dutchadblocklist+easylist.txt or any other long *.txt URL
2. Press ctrl-A

What is the expected result?
Text gets selected

What happens instead?
Tab uses large amounts of CPU. Text doesn't get selected (at least not within a few minutes)

Please provide any additional information below. Attach a screenshot if possible.
-Worked fine in Chrome 20.
-Works fine for small text files.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=129650

[Alexander Pavlov (apavlov)]

Created attachment 148583 [details]
Patch

[Build Bot]

Comment on attachment 148583 [details]
Patch

Attachment 148583 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13002247

[Alexey Proskuryakov]

This patch changes two platforms' code. Is that expected to cover all affected platforms?

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> This patch changes two platforms' code. Is that expected to cover all affected platforms?

I believe we only have two Windows ports. At least, the offending patch changed exactly these two methods.

[Alexey Proskuryakov]

> At least, the offending patch changed exactly these two methods.

OK

[Tony Chang]

Comment on attachment 148583 [details]
Patch

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

> Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:65
> +        ++index;
>      }

Nit: I would just make this a for loop.

> Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:66
> +    str.swap(result.toString());

Looks like this doesn't compile, I bet you can't swap without holding a reference. I'll try and upload a fix.

[Alexander Pavlov (apavlov)]

(In reply to comment #6)
> (From update of attachment 148583 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148583&action=review
> 
> > Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:65
> > +        ++index;
> >      }
> 
> Nit: I would just make this a for loop.

Oops, should have changed it... Will fix.

> > Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:66
> > +    str.swap(result.toString());
> 
> Looks like this doesn't compile, I bet you can't swap without holding a reference. I'll try and upload a fix.

ANSI ISO/IEC 14882 section 13.3.3.1.4 allows binding a temporary to a const reference. As a data point, this built fine for me using MSVS 2008.

[Tony Chang]

Created attachment 148606 [details]
Patch

[Tony Chang]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 148583 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=148583&action=review
> > 
> > > Source/WebCore/platform/chromium/ClipboardUtilitiesChromium.cpp:66
> > > +    str.swap(result.toString());
> > 
> > Looks like this doesn't compile, I bet you can't swap without holding a reference. I'll try and upload a fix.
> 
> ANSI ISO/IEC 14882 section 13.3.3.1.4 allows binding a temporary to a const reference. As a data point, this built fine for me using MSVS 2008.

It turns out that we can just assign the value since WTF::String is internally ref counted.  I verified that this works for me on Linux (removed some of the #ifs to test).

[WebKit Review Bot]

Comment on attachment 148606 [details]
Patch

Clearing flags on attachment: 148606

Committed r120850: <http://trac.webkit.org/changeset/120850>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jacky Jiang]

Reopening to attach new patch.

[Jacky Jiang]

Created attachment 148630 [details]
Patch

[Jacky Jiang]

Comment on attachment 148630 [details]
Patch

Oops, it was uploaded to the wrong bug by the tool... Sorry!