Bug 77295

Summary: V8 idl code generator doesn't handle [CachedAttribute] on SerializedScriptValue attributes.
Product: WebKit Reporter: Pablo Flouret <pf>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, haraken, japhet, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76035    
Attachments:
Description Flags
Proof of concept patch
none
Better patch none

Description Pablo Flouret 2012-01-29 02:02:08 PST
Actually, i looks like the SerializedScriptValue attribute case is hackishly handled for just one attribute of the kind, which is deserialized once in the constructor, with no getter generated for it.
Comment 1 Pablo Flouret 2012-01-29 02:14:26 PST
Created attachment 124462 [details]
Proof of concept patch
Comment 2 Pablo Flouret 2012-01-29 02:19:50 PST
Comment on attachment 124462 [details]
Proof of concept patch

Came up while looking at the history.state stuff, the JSC generator generates code for [CachedAttribute] SerializedScriptValue attributes.

Rough patch as a possible solution, it probably needs more work to handle more [InitializedByConstructor] attributes (and i'm not sure hijacking that extended-attribute is even a good idea), and i haven't looked into the setters case.

I tried going with ForceSet() instead of using hidden attributes but i didn't find a way to reset the cached value in that case (would probably need to set the accessor function again, but it's internal to the v8 generated file), and a way to invalidate the value is needed for the history.state thing.

Ideas/comments welcome.
Comment 3 Pablo Flouret 2012-01-29 02:22:33 PST
(In reply to comment #2)
> (From update of attachment 124462 [details])
> [...]
> I tried going with ForceSet() instead of using hidden attributes but i didn't find a way to reset the cached value in that case (would probably need to set the accessor function again, but it's internal to the v8 generated file), and a way to invalidate the value is needed for the history.state thing.
> 

s/hidden attributes/hidden values/
Comment 4 WebKit Review Bot 2012-01-29 02:39:17 PST
Comment on attachment 124462 [details]
Proof of concept patch

Attachment 124462 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11369253
Comment 5 Kentaro Hara 2012-01-29 15:52:04 PST
Comment on attachment 124462 [details]
Proof of concept patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review

> Source/WebCore/Modules/intents/Intent.idl:34
> +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;

You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later.

Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler.
Comment 6 Pablo Flouret 2012-01-29 17:19:58 PST
(In reply to comment #5)
> (From update of attachment 124462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review
> 
> > Source/WebCore/Modules/intents/Intent.idl:34
> > +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;
> 
> You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later.
> 
> Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler.

Yeah, [InitializedByConstructor] is wrongly used here, that's what i meant when i said i was hijacking the meaning, but i was somehow trying to preserve the behavior introduced by https://bugs.webkit.org/show_bug.cgi?id=36892 and https://bugs.webkit.org/show_bug.cgi?id=75641

What i'm trying to do is something more akin to what the JSC generator does for SerializedScriptValue attributes. It generates getters/setters that deserialize the value when they're called (and cache the deserialized value in a member in the object if [CachedAttribute] is specified), which is not done in the v8 generator.
Instead there's code to deserialize one (and only one) SerializedScriptValue attribute upfront in the constructor and ForceSet:tting it in the object, avoiding the generation and use of getters/setters.
If i understood things correctly this was initially set up like that because MessageEvent needed the data deserialized in the right context (which presumably would not be the correct one if deserialized lazily in a getter?). It looks like the webintents case would be handled correctly.

Do you think we can do something to not special case what MessageEvent needs so that we can generate the getter/setter functions and do everything the same way?
Comment 7 Kentaro Hara 2012-01-29 20:51:38 PST
Comment on attachment 124462 [details]
Proof of concept patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review

>>> Source/WebCore/Modules/intents/Intent.idl:34
>>> +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;
>> 
>> You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later.
>> 
>> Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler.
> 
> Yeah, [InitializedByConstructor] is wrongly used here, that's what i meant when i said i was hijacking the meaning, but i was somehow trying to preserve the behavior introduced by https://bugs.webkit.org/show_bug.cgi?id=36892 and https://bugs.webkit.org/show_bug.cgi?id=75641
> 
> What i'm trying to do is something more akin to what the JSC generator does for SerializedScriptValue attributes. It generates getters/setters that deserialize the value when they're called (and cache the deserialized value in a member in the object if [CachedAttribute] is specified), which is not done in the v8 generator.
> Instead there's code to deserialize one (and only one) SerializedScriptValue attribute upfront in the constructor and ForceSet:tting it in the object, avoiding the generation and use of getters/setters.
> If i understood things correctly this was initially set up like that because MessageEvent needed the data deserialized in the right context (which presumably would not be the correct one if deserialized lazily in a getter?). It looks like the webintents case would be handled correctly.
> 
> Do you think we can do something to not special case what MessageEvent needs so that we can generate the getter/setter functions and do everything the same way?

Ah, I got it. But still I have some questions.

Why is this line not "readonly attribute [CachedAttribute] SerializedScriptValue data"? What you want to do is to make it work, isn't it?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:869
> +        if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"CachedAttribute"}) {
> +            push(@implContentDecls, <<END);
> +    v8::Handle<v8::String> propertyName = v8::String::NewSymbol("${attrName}");
> +    v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(propertyName);
> +    if (!value.IsEmpty())
> +        return value;
> +END
> +        }

This looks fine.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1025
> +        if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"CachedAttribute"}) {
> +            my $getterFunc = $codeGenerator->WK_lcfirst($attribute->signature->name);
> +            push(@implContentDecls, <<END);
> +    SerializedScriptValue* serialized = imp->${getterFunc}();
> +    value = serialized ? serialized->deserialize() : v8::Handle<v8::Value>(v8::Null());
> +    info.Holder()->SetHiddenValue(propertyName, value);
> +    return value;
> +END
> +        } else {
> +            push(@implContentDecls, "    " . ReturnNativeToJSValue($attribute->signature, $result, "    ").";\n");
> +        }

This also looks fine.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1962
> -    # Attributes of type SerializedScriptValue are set in the
> +    # Attributes of type SerializedScriptValue marked as [InitializedByConstructor] are set in the
>      # constructor and don't require callbacks.
> -    return if ($attribute->signature->type eq "SerializedScriptValue");
> +    return if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"InitializedByConstructor"});

But I am not still sure why we need these changes about [InitializedByConstructor]. I am expecting that the changes of two "if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"CachedAttribute"}) { ... }" (i.e. line 862 -- 869 and 1015 -- 1025) would be enough to cache attributes. With the two changes, the attribute will be cached when the getter/setter is called at the first time.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2325
> +            if ($attribute->signature->extendedAttributes->{"InitializedByConstructor"}) {
> +                die "Only one attribute of type SerializedScriptValue [InitializedByConstructor] supported" if $serializedAttribute;
> +                $serializedAttribute = $attribute;
> +                next;
> +            }

Ditto.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:136
> +    { "serializedScriptValueAttr", DontDelete | ReadOnly, (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsTestObjSerializedScriptValueAttr), (intptr_t)0, NoIntrinsic },
> +    { "cachedSerializedScriptValueAttr", DontDelete | ReadOnly, (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsTestObjCachedSerializedScriptValueAttr), (intptr_t)0, NoIntrinsic },

Would you please make a patch with the "right" run-bindings-tests results? You didn't touch CodeGeneratorJS.pm and thus JSTestObj.cpp should not be changed. Maybe you need to rebaseline run-bindings-tests before you make a patch. The run-bindings-tests results in WebKit trunk are sometimes wrong, since people are likely to forget to update the results:-) (The run-bindings-tests results are just for reviews and the build bots do not check their failures.)

> Source/WebCore/bindings/scripts/test/TestObj.idl:155
> +
> +        readonly attribute SerializedScriptValue serializedScriptValueAttr;
> +        readonly attribute [CachedAttribute] SerializedScriptValue cachedSerializedScriptValueAttr;

It might be better to put these tests in TestSerializedScriptValueInterface.idl.

Also, please remove "readonly" in order to test its setter.

> Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:35
> -        readonly attribute SerializedScriptValue value;
> +        readonly attribute [InitializedByConstructor] SerializedScriptValue value;

Where are the test results (i.e. test/V8/V8TestSerializedScriptValueInterface.cpp)?
Comment 8 Kentaro Hara 2012-01-29 20:55:54 PST
(In reply to comment #2)
> (From update of attachment 124462 [details])
> I tried going with ForceSet() instead of using hidden attributes but i didn't find a way to reset the cached value in that case (would probably need to set the accessor function again, but it's internal to the v8 generated file), and a way to invalidate the value is needed for the history.state thing.
> 
> Ideas/comments welcome.

Using GetHiddenValue()/SetHiddenValue() makes sense to me.

(FYI: Rather, I would object to use ForceSet() here. We are planning to move DOM attributes from a DOM object to a JS prototype chain (https://bugs.webkit.org/show_bug.cgi?id=75297), but ForceSet() makes it difficult (or impossible?). I cannot move MessageEvent.data from a DOM object to a JS prototype chain because it uses ForceSet()...)
Comment 9 Pablo Flouret 2012-01-30 13:39:21 PST
(In reply to comment #7)
> (From update of attachment 124462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review
> 
> >>> Source/WebCore/Modules/intents/Intent.idl:34
> >>> +        readonly attribute [InitializedByConstructor] SerializedScriptValue data;
> >> 
> >> You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later.
> >> 
> >> Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler.
> > 
> > Yeah, [InitializedByConstructor] is wrongly used here, that's what i meant when i said i was hijacking the meaning, but i was somehow trying to preserve the behavior introduced by https://bugs.webkit.org/show_bug.cgi?id=36892 and https://bugs.webkit.org/show_bug.cgi?id=75641
> > 
> > What i'm trying to do is something more akin to what the JSC generator does for SerializedScriptValue attributes. It generates getters/setters that deserialize the value when they're called (and cache the deserialized value in a member in the object if [CachedAttribute] is specified), which is not done in the v8 generator.
> > Instead there's code to deserialize one (and only one) SerializedScriptValue attribute upfront in the constructor and ForceSet:tting it in the object, avoiding the generation and use of getters/setters.
> > If i understood things correctly this was initially set up like that because MessageEvent needed the data deserialized in the right context (which presumably would not be the correct one if deserialized lazily in a getter?). It looks like the webintents case would be handled correctly.
> > 
> > Do you think we can do something to not special case what MessageEvent needs so that we can generate the getter/setter functions and do everything the same way?
> 
> Ah, I got it. But still I have some questions.
> 
> Why is this line not "readonly attribute [CachedAttribute] SerializedScriptValue data"? What you want to do is to make it work, isn't it?

I was trying to preserve a behavior that i thought was introduced just for MessageEvent, but that one is actually using custom getter/setters so i'll just get rid of the whole [InitializedByConstructor] faux handling and make a proper patch. Sorry for the misunderstanding.
Comment 10 Pablo Flouret 2012-01-30 15:17:16 PST
Created attachment 124611 [details]
Better patch
Comment 11 WebKit Review Bot 2012-01-30 15:21:35 PST
Attachment 124611 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:76:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:89:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:100:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:113:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:126:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Pablo Flouret 2012-02-02 16:02:54 PST
Ping?
Comment 13 Kentaro Hara 2012-02-02 16:18:35 PST
Comment on attachment 124611 [details]
Better patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124611&action=review

> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:88
> +    info.Holder()->DeleteHiddenValue(v8::String::NewSymbol("cachedValue")); // Invalidate the cached value.

Why don't you set the new value by SetHiddenValue(), instead of invalidating the current value?
Comment 14 Pablo Flouret 2012-02-02 16:34:01 PST
Comment on attachment 124611 [details]
Better patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124611&action=review

>> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:88
>> +    info.Holder()->DeleteHiddenValue(v8::String::NewSymbol("cachedValue")); // Invalidate the cached value.
> 
> Why don't you set the new value by SetHiddenValue(), instead of invalidating the current value?

We'd need to deserialize it, i figured it would be better to do that lazily when it's requested (i think the JSC generator does it like this as well). Also, (using this case as an example) we'd need to assume that v == imp->getCachedValue(), otherwise we'd need to call imp->getCachedValue() before deserializing, and by that point we might as well just let the getter do its job.
Comment 15 Kentaro Hara 2012-02-02 16:38:26 PST
Comment on attachment 124611 [details]
Better patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124611&action=review

Feel free to ping me if I am missing a review.

>>> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:88
>>> +    info.Holder()->DeleteHiddenValue(v8::String::NewSymbol("cachedValue")); // Invalidate the cached value.
>> 
>> Why don't you set the new value by SetHiddenValue(), instead of invalidating the current value?
> 
> We'd need to deserialize it, i figured it would be better to do that lazily when it's requested (i think the JSC generator does it like this as well). Also, (using this case as an example) we'd need to assume that v == imp->getCachedValue(), otherwise we'd need to call imp->getCachedValue() before deserializing, and by that point we might as well just let the getter do its job.

Makes sense!
Comment 16 Kentaro Hara 2012-02-02 16:39:31 PST
Comment on attachment 124611 [details]
Better patch

Let me land this patch manually, to avoid style check errors.
Comment 17 Pablo Flouret 2012-02-02 16:40:21 PST
Thanks!
Comment 18 Kentaro Hara 2012-02-02 16:48:09 PST
Committed r106605: <http://trac.webkit.org/changeset/106605>