RESOLVED FIXED 106668
Web Inspector: Make V8 LiveEdit API disabled by default
https://bugs.webkit.org/show_bug.cgi?id=106668
Summary Web Inspector: Make V8 LiveEdit API disabled by default
Peter Rybin
Reported 2013-01-11 08:37:54 PST
LiveEdit is a sophisticated routine in V8, and potentially it may have some vulnerabilities. Enable C-level blocks in V8 core, that won't allow LiveEdit code unless actually called LiveEdit command. Related Chromium bug: https://code.google.com/p/chromium/issues/detail?id=159124
Attachments
Patch (1.36 KB, patch)
2013-01-11 08:41 PST, Peter Rybin
no flags
Patch (2.42 KB, patch)
2013-01-11 09:53 PST, Peter Rybin
no flags
Patch (2.53 KB, patch)
2013-01-18 12:37 PST, Peter Rybin
no flags
Patch (2.57 KB, patch)
2013-01-19 11:07 PST, Peter Rybin
no flags
Peter Rybin
Comment 1 2013-01-11 08:41:59 PST
Peter Rybin
Comment 2 2013-01-11 08:42:36 PST
WebKit Review Bot
Comment 3 2013-01-11 08:45:55 PST
Comment on attachment 182349 [details] Patch Attachment 182349 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15808414
Vsevolod Vlasov
Comment 4 2013-01-11 09:14:01 PST
Some thoughts: 1. I would consider setting live_edit_enabled_ to false in Debugger constructor. It might break other existing v8 embedders though. 2. If we don't want to change v8 API in this sense, I would expect to see this in WebCore v8 bindings, where other setLiveEditEnabled calls reside.
Peter Beverloo (cr-android ews)
Comment 5 2013-01-11 09:25:44 PST
Comment on attachment 182349 [details] Patch Attachment 182349 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15811350
Peter Rybin
Comment 6 2013-01-11 09:53:59 PST
Adam Barth
Comment 7 2013-01-11 10:36:51 PST
Comment on attachment 182358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182358&action=review > Source/WebCore/bindings/v8/V8Initializer.cpp:108 > +// V8 initialization part common for both Document and Worker. This comment doesn't really add anything. Perhaps we should remove it? > Source/WebCore/bindings/v8/V8Initializer.cpp:125 > v8::V8::IgnoreOutOfMemoryException(); This call looks shared by both Document and worker, perhaps we should move it into initializeV8Common as well. (There look to be a couple others that we could share as well.)
Adam Barth
Comment 8 2013-01-11 10:39:27 PST
This seems like something that's better done in V8, but we can do it here if we don't want to change the V8 API in this way.
Peter Rybin
Comment 9 2013-01-14 08:35:16 PST
Non-liveedit-related changes are moved into a separate prerequisite change: https://bugs.webkit.org/show_bug.cgi?id=106790
Peter Rybin
Comment 10 2013-01-18 12:37:50 PST
Peter Rybin
Comment 11 2013-01-18 12:46:49 PST
Moving the default value into V8 is not quite elegant, because it won't spare us from changes anyway, but would require all other V8 embedders to manually change the default value back without any pay-off for them (you only can use enable/disable, if you wrote the entire debug command dispatcher in C++ like WebKit did). All other comments are addresses.
WebKit Review Bot
Comment 12 2013-01-18 23:21:21 PST
Comment on attachment 183529 [details] Patch Rejecting attachment 183529 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/15973076
Yury Semikhatsky
Comment 13 2013-01-18 23:26:01 PST
(In reply to comment #12) > (From update of attachment 183529 [details]) > Rejecting attachment 183529 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue > > /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). > > Full output: http://queues.webkit.org/results/15973076 Peter you'll need to restore "Reviewed by..." line in the ChangeLog in order commit queue to understand it.
Peter Rybin
Comment 14 2013-01-19 11:07:00 PST
Peter Rybin
Comment 15 2013-01-19 11:10:27 PST
> Peter you'll need to restore "Reviewed by..." line in the ChangeLog in order commit queue to understand it. Thanks! My bad. Oh bureaucracy!.. :)
WebKit Review Bot
Comment 16 2013-01-20 10:19:41 PST
Comment on attachment 183629 [details] Patch Clearing flags on attachment: 183629 Committed r140273: <http://trac.webkit.org/changeset/140273>
WebKit Review Bot
Comment 17 2013-01-20 10:19:46 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.