WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(3.99 KB, patch)
2012-06-20 11:01 PDT
,
Tony Chang
no flags
Details
Formatted Diff
Diff
Patch
(4.96 KB, patch)
2012-06-20 12:42 PDT
,
Jacky Jiang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alexander Pavlov (apavlov)
Comment 1
2012-06-20 09:32:10 PDT
Created
attachment 148583
[details]
Patch
Build Bot
Comment 2
2012-06-20 10:01:10 PDT
Comment on
attachment 148583
[details]
Patch
Attachment 148583
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/13002247
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
Created
attachment 148606
[details]
Patch
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
Created
attachment 148630
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug