bindings: Use CreateDataProperty() instead of ForceSet()

bindings: Use CreateDataProperty() instead of ForceSet()

We use ForceSet() to avoid executing user-provided JS while running
callbacks, but ForceSet() is deprecated because it is difficult to
use properly. Use CreateDataProperty() instead. It is a safer way to
do that.

BUG=475206
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197156

[bashi, 2015-05-29 05:46:32]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org, jsbell@chromium.org

[bashi, 2015-05-29 05:46:33]

PTAL?

I added some TODO comments where I'm not sure how I can replace ForceSet() with
CreateDataProperty(). jochen@, what should I do for these cases?

[jsbell, 2015-05-29 16:39:23]

lgtm on the actual code changes; I didn't review the TODOs closely, though.

[jsbell, 2015-05-29 16:57:21]

https://codereview.chromium.org/1153613007/diff/1/Source/bindings/templates/i...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1153613007/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface_base.cpp:51: static void
{{cpp_class}}ForceSetAttributeOnThis(v8::Local<v8::Name> name,
v8::Local<v8::Value> v8Value, const CallbackInfo& info)
Don't forget to rename this function.

[bashi, 2015-06-02 00:12:54]

jochen@ added DefineOwnProperty. Once it is rolled into chromium, we can replace
all ForceSet with it. I'll do in follow-up CLs.

https://codereview.chromium.org/1167473002

https://codereview.chromium.org/1153613007/diff/1/Source/bindings/templates/i...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1153613007/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface_base.cpp:51: static void
{{cpp_class}}ForceSetAttributeOnThis(v8::Local<v8::Name> name,
v8::Local<v8::Value> v8Value, const CallbackInfo& info)
On 2015/05/29 16:57:21, jsbell wrote:
> Don't forget to rename this function.

Sure. Will rename it.

[jochen (gone - plz use gerrit), 2015-06-02 16:14:48]

there are a bunch of ForceSet() calls where it's not immediately obvious why you
can't replace them?

https://codereview.chromium.org/1153613007/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.cpp
(right):

https://codereview.chromium.org/1153613007/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.cpp:37:
v8::Local<v8::Object>::Cast(info.This())->ForceSet(info.GetIsolate()->GetCurrentContext(),
name, v8Value);
why can't this use CreateDataProperty already now?

[bashi, 2015-06-03 00:16:37]

DefineOwnProperty is rolled in. PTAL?

https://codereview.chromium.org/1153613007/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8MessageEventCustom.cpp (right):

https://codereview.chromium.org/1153613007/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8MessageEventCustom.cpp:95: if
(!v8CallBoolean(info.Holder()->ForceSet(info.GetIsolate()->GetCurrentContext(),
v8AtomicString(info.GetIsolate(), "data"), result, dataAttr))) {
We can't use DefineOwnProperty() here because it recursively calls this callback
and causes stack overflow.

https://codereview.chromium.org/1153613007/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.cpp
(right):

https://codereview.chromium.org/1153613007/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.cpp:37:
v8::Local<v8::Object>::Cast(info.This())->ForceSet(info.GetIsolate()->GetCurrentContext(),
name, v8Value);
On 2015/06/02 16:14:48, jochen wrote:
> why can't this use CreateDataProperty already now?

Done.

https://codereview.chromium.org/1153613007/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1153613007/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:379: if
(!v8CallBoolean(context->Global()->ForceSet(context, v8AtomicString(m_isolate,
"document"), documentWrapper, static_cast<v8::PropertyAttribute>(v8::ReadOnly |
v8::DontDelete))))
We still need ForceSet().

[bashi, 2015-06-03 03:05:31]

https://codereview.chromium.org/1153613007/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1153613007/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:569: auto unused =
object->DefineOwnProperty(info.GetIsolate()->GetCurrentContext(), key, info[2],
v8::DontEnum);
This causes an exception.

[jochen (gone - plz use gerrit), 2015-06-03 10:55:17]

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:378: // global proxy objects.
why?

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:568: //
TODO(bashi): Use DefineOwnProperty() if possible.
what exception does this throw?

[bashi, 2015-06-05 01:10:27]

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/WindowProxy.cpp:378: // global proxy objects.
On 2015/06/03 10:55:17, jochen wrote:
> why?

I hit this assertion.

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/1153613007/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:568: //
TODO(bashi): Use DefineOwnProperty() if possible.
On 2015/06/03 10:55:17, jochen wrote:
> what exception does this throw?

Nevermind; this custom bindings has gone.
https://codereview.chromium.org/1149383005

[jochen (gone - plz use gerrit), 2015-06-05 12:12:57]

jochen@chromium.org changed reviewers:
+ verwaest@chromium.org

[jochen (gone - plz use gerrit), 2015-06-05 12:12:58]

Toon, what should we do about the remaining ForceSet(). Apparently we can't
DefineOwnProperty on the global proxy.

[jochen (gone - plz use gerrit), 2015-06-08 11:36:06]

bashi@ after talking to Toon I still thing defineOwnProperty() *should* work.

Can you post a full stack trace where you've hit the assertion?

[bashi, 2015-06-08 23:38:40]

On 2015/06/08 11:36:06, jochen wrote:
> bashi@ after talking to Toon I still thing defineOwnProperty() *should* work.
> 
> Can you post a full stack trace where you've hit the assertion?

Sure.

--- Stack trace ---
#0  v8::base::OS::Abort () at ../../v8/src/base/platform/platform-posix.cc:229
#1  0x0000000002146edc in V8_Fatal (file=<optimized out>, line=0,
    format=<optimized out>) at ../../v8/src/base/logging.cc:116
#2  0x0000000001d4056a in v8::internal::JSObject::MigrateFastToSlow (
    object=..., new_map=..., expected_additional_properties=0)
    at ../../v8/src/objects.cc:4429
#3  0x0000000001d3e45b in v8::internal::JSObject::MigrateToMap (object=...,
    new_map=..., expected_additional_properties=0)
    at ../../v8/src/objects.cc:1881
#4  0x0000000001d53fe8 in v8::internal::JSObject::SetPropertyCallback (
    object=..., name=..., structure=..., attributes=FROZEN)
    at ../../v8/src/objects.cc:6352
#5  0x0000000001d4f729 in v8::internal::JSObject::SetOwnPropertyIgnoreAttributes
    (object=..., name=..., value=..., attributes=FROZEN,
    handling=<optimized out>) at ../../v8/src/objects.cc:4210
#6  0x0000000001e10613 in __RT_impl_Runtime_DefineDataPropertyUnchecked (
    isolate=<optimized out>, args=...)
    at ../../v8/src/runtime/runtime-object.cc:1384
#7  v8::internal::Runtime_DefineDataPropertyUnchecked (
    args_length=<optimized out>, args_object=<optimized out>,
    isolate=0x920c6e38020) at ../../v8/src/runtime/runtime-object.cc:1362

--- V8 stack trace ---
Security context: 0xbeda03a43d9 <JS Object>
    1: DefineObjectProperty(aka DefineObjectProperty) [native v8natives.js:577]
(this=0xbeda0304179 <undefined>,H=0xb770e004391 <JS Global
Object>,U=0xbeda0383119 <String[8]: doc\
ument>,D=0xb770e0d9509 <a PropertyDescriptor with map
0x6619cc077a9>,X=0xbeda0304251 <false>)
    2: DefineOwnProperty(aka DefineOwnProperty) [native v8natives.js:693]
(this=0xbeda0304179 <undefined>,H=0xb770e004391 <JS Global
Object>,U=0xbeda0383119 <String[8]: document>\
,D=0xb770e0d9509 <a PropertyDescriptor with map 0x6619cc077a9>,X=0xbeda0304251
<false>)
    3: DefineOwnPropertyFromAPI(aka DefineOwnPropertyFromAPI) [native
v8natives.js:697] (this=0xbeda0304179 <undefined>,H=0xb770e004391 <JS Global
Object>,U=0xbeda0383119 <String\
[8]: document>,L=0xb770e0d9229 <an HTMLDocument with map
0x6619cc07179>,D=0xb770e0d9331 <JS Array[3]>)

--- V8 Entry point ---
#0  0x0000000001b57337 in v8::internal::Invoke (is_construct=<optimized out>,
    function=..., receiver=..., argc=<optimized out>, args=<optimized out>)
    at ../../v8/src/execution.cc:128
#1  0x0000000001a77226 in CallV8HeapFunction (recv=..., argc=4,
    isolate=<optimized out>, name=<optimized out>, argv=<optimized out>)
    at ../../v8/src/api.cc:2292
#2  v8::Object::DefineOwnProperty (this=<optimized out>, context=..., key=...,
    value=..., attributes=<optimized out>) at ../../v8/src/api.cc:3619
#3  0x0000000003627ba5 in blink::WindowProxy::updateDocumentProperty (
    this=0x920c6af53a0)
    at ../../third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:379
#4  0x0000000003627193 in blink::WindowProxy::updateDocument (
    this=0x920c6af53a0)
    at ../../third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:442
#5  0x00000000036264b8 in blink::WindowProxy::initialize (this=0x920c6af53a0)
    at ../../third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:244
#6  0x00000000036261d0 in blink::WindowProxy::initializeIfNeeded (
    this=0x920c6af53a0)
    at ../../third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:207
#7  0x00000000035b228a in blink::ScriptController::windowProxy (
    this=0x920c6af5480, world=...)
    at ../../third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:211
#8  0x0000000002f27f2f in blink::LocalFrame::windowProxy (this=0x2e95cfe44610,
    world=...) at ../../third_party/WebKit/Source/core/frame/LocalFrame.cpp:233
#9  0x00000000035e43ba in blink::toV8Context (frame=0x2e95cfe44610, world=...)
    at ../../third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:777
#10 0x000000000215a79a in blink::WebLocalFrameImpl::mainWorldScriptContext (
    this=0x2e95cfe50010)
    at ../../third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:942
#11 0x00000000052d6580 in blink::WebTestingSupport::resetInternalsObject (
    frame=0x2e95cfe50010)
    at ../../third_party/WebKit/Source/web/WebTestingSupport.cpp:46
#12 0x00000000004b6531 in content::BlinkTestRunner::Reset (this=0x920c64a7e20)
    at ../../content/shell/renderer/layout_test/blink_test_runner.cc:766
#13 0x000000000047cb01 in
content::LayoutTestContentRendererClient::RenderViewCreated (this=0x920c60b0e60,
render_view=0x920c63d3428)
    at
../../content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:103
#14 0x0000000004bb5efc in content::RenderViewImpl::Initialize (
    this=0x920c63d3020, params=..., compositor_deps=0x920c6342210,
    was_created_by_renderer=false)
    at ../../content/renderer/render_view_impl.cc:816
...

[jochen (gone - plz use gerrit), 2015-06-09 08:11:33]

Ah, I guess what happens is that "document" is already defined in Window.idl as
an unforgeable read-only attribute, so what we can't overwrite it.

Maybe "document" should not be in window.idl since we'll programmatically set it
(or it should be marked as not set by generated bindings or something)

[Toon Verwaest, 2015-06-11 19:23:26]

I've fixed the issue underlying the assert, so you will be able to try this
again in the future. I tried it locally by making ForceSet do the right thing,
but there are still some issues in blink. E.g., MessageEventCustom>>data relies
on accessors + ForceSet to lazily initialize itself. This won't work anymore...

After the changes I want to do, the accessor will stay there forever; on
ForceSet to non-writable I just remove the setter (if writable the setter
stays). It should instead cache the value as a hidden property, and the getter
should retrieve that value.

I think there were also worker-typed-array-related failures, iirc.

[jochen (gone - plz use gerrit), 2015-06-12 09:49:31]

lgtm

[bashi, 2015-06-16 03:13:36]

I'm going to land this. We will address WindowProxy issue separately.

[bashi, 2015-06-16 03:13:44]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-06-16 03:13:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1153613007/#ps120001
(title: "Fix uninitialized error")

[commit-bot: I haz the power, 2015-06-16 03:13:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153613007/120001

[commit-bot: I haz the power, 2015-06-16 03:20:33]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197156