RESOLVED FIXED 89562
Regression(r116408): Ctrl-A (select all) on large text file hangs the tab with high CPU usage
https://bugs.webkit.org/show_bug.cgi?id=89562
Summary Regression(r116408): Ctrl-A (select all) on large text file hangs the tab wit...
Alexander Pavlov (apavlov)
Reported 2012-06-20 05:36:18 PDT
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
Attachments
Patch (3.78 KB, patch)
2012-06-20 09:32 PDT, Alexander Pavlov (apavlov)
no flags
Patch (3.99 KB, patch)
2012-06-20 11:01 PDT, Tony Chang
no flags
Patch (4.96 KB, patch)
2012-06-20 12:42 PDT, Jacky Jiang
no flags
Alexander Pavlov (apavlov)
Comment 1 2012-06-20 09:32:10 PDT
Build Bot
Comment 2 2012-06-20 10:01:10 PDT
Alexey Proskuryakov
Comment 3 2012-06-20 10:22:27 PDT
This patch changes two platforms' code. Is that expected to cover all affected platforms?
Alexander Pavlov (apavlov)
Comment 4 2012-06-20 10:24:20 PDT
(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
Comment 5 2012-06-20 10:27:23 PDT
> At least, the offending patch changed exactly these two methods. OK
Tony Chang
Comment 6 2012-06-20 10:38:33 PDT
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)
Comment 7 2012-06-20 10:48:31 PDT
(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
Comment 8 2012-06-20 11:01:08 PDT
Tony Chang
Comment 9 2012-06-20 11:02:48 PDT
(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 10 2012-06-20 12:33:20 PDT
Comment on attachment 148606 [details] Patch Clearing flags on attachment: 148606 Committed r120850: <http://trac.webkit.org/changeset/120850>
WebKit Review Bot
Comment 11 2012-06-20 12:33:25 PDT
All reviewed patches have been landed. Closing bug.
Jacky Jiang
Comment 12 2012-06-20 12:42:21 PDT
Reopening to attach new patch.
Jacky Jiang
Comment 13 2012-06-20 12:42:32 PDT
Jacky Jiang
Comment 14 2012-06-20 12:44:46 PDT
Comment on attachment 148630 [details] Patch Oops, it was uploaded to the wrong bug by the tool... Sorry!
Note You need to log in before you can comment on or make changes to this bug.