Bug 106668 – Web Inspector: Make V8 LiveEdit API disabled by default

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Peter Rybin]

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

[Peter Rybin]

Created attachment 182349 [details]
Patch

[Peter Rybin]

This is a follow-up for https://bugs.webkit.org/show_bug.cgi?id=104039

[WebKit Review Bot]

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]

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 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]

Created attachment 182358 [details]
Patch

[Adam Barth]

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]

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]

Non-liveedit-related changes are moved into a separate prerequisite change:
https://bugs.webkit.org/show_bug.cgi?id=106790

[Peter Rybin]

Created attachment 183529 [details]
Patch

[Peter Rybin]

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 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]

(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]

Created attachment 183629 [details]
Patch

[Peter Rybin]

> 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 on attachment 183629 [details]
Patch

Clearing flags on attachment: 183629

Committed r140273: <http://trac.webkit.org/changeset/140273>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.