Bug 44678 – [v8] More correct and faster error handling when converting v8 objects to various WebCore strings

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[anton muhin]

[v8] More correct and faster error handling when converting v8 objects to various WebCore strings

[anton muhin]

Created attachment 65550 [details]
Patch

[Adam Barth]

Comment on attachment 65550 [details]
Patch

Aside from the comment below, this looks reasonable.  I'm a bit too tired to think it through entirely though, so I'm leaving it r? for now.

WebCore/bindings/v8/V8Binding.cpp:375
 +      static AtomicString lowNumbers[kLowNumbers + 1];
Is this threadsafe?  What if a worker and a normal document both use the number 42?

[anton muhin]

Created attachment 65706 [details]
Patch

[anton muhin]

(In reply to comment #2)
> (From update of attachment 65550 [details])
> Aside from the comment below, this looks reasonable.  I'm a bit too tired to think it through entirely though, so I'm leaving it r? for now.
> 
> WebCore/bindings/v8/V8Binding.cpp:375
>  +      static AtomicString lowNumbers[kLowNumbers + 1];
> Is this threadsafe?  What if a worker and a normal document both use the number 42?

Good catch.  I don't think it's thread safe.  However AFAIK we are still single threaded.  So I've added an ASSERT on top of the function with some clarification.  Do you think it's enough for now?  And are you fine with the wording?

[Adam Barth]

Comment on attachment 65706 [details]
Patch

> WebCore/bindings/v8/V8Binding.cpp:377
> +    // Most numbers used are <= 100. Even if they aren't used there's very little in using the space.
very little cost?  Seems we're missing a word here.

> WebCore/bindings/v8/V8Binding.cpp:379
> +    static AtomicString lowNumbers[kLowNumbers + 1];
Shouldn't we use DECLARE_STATIC_LOCAL here?

> WebCore/bindings/v8/V8Binding.cpp:384
> +            AtomicString valueString = AtomicString(String::number(value));
Is there an advantage to using atomic strings here?  It seems like you're already doing the atomization.

> WebCore/bindings/v8/V8Binding.cpp:397
> +        int32ToWebCoreString(object->Int32Value());
Why don't we use the return value here?  Is this code tested???

> WebCore/bindings/v8/V8Binding.h:218
> +        operator AtomicString() { return toString<AtomicString>(); }
I think we'd be better off if we had explicit conversion operators.  In general, WebKit frown on these implicit operators.

> WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:114
> +    STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, type, args[0]);
Why this particular callsite?  Is this the only custom one?

[anton muhin]

Created attachment 65934 [details]
Patch

[anton muhin]

(In reply to comment #5)
> (From update of attachment 65706 [details])
> > WebCore/bindings/v8/V8Binding.cpp:377
> > +    // Most numbers used are <= 100. Even if they aren't used there's very little in using the space.
> very little cost?  Seems we're missing a word here.

Added 'cost'

> 
> > WebCore/bindings/v8/V8Binding.cpp:379
> > +    static AtomicString lowNumbers[kLowNumbers + 1];
> Shouldn't we use DECLARE_STATIC_LOCAL here?

Adam, the code was copied verbatim.  Could we postpone this DECLARE_STATIC_LOCAL for a next commit?  I'd like to minimize changes to the code which existed before my change.

> 
> > WebCore/bindings/v8/V8Binding.cpp:384
> > +            AtomicString valueString = AtomicString(String::number(value));
> Is there an advantage to using atomic strings here?  It seems like you're already doing the atomization.

I think yes, as AtomicString to String conversion is cheaper then conversion back if I understand it correctly.  Again, I don't change this code.

> 
> > WebCore/bindings/v8/V8Binding.cpp:397
> > +        int32ToWebCoreString(object->Int32Value());
> Why don't we use the return value here?  Is this code tested???

Very good catch, thank you very much.  Alas, it's not obvious how to test it: it's only an optimization: we fall through to heavyweight toString conversion which would do the trick anyway.

> 
> > WebCore/bindings/v8/V8Binding.h:218
> > +        operator AtomicString() { return toString<AtomicString>(); }
> I think we'd be better off if we had explicit conversion operators.  In general, WebKit frown on these implicit operators.

Those operators are crucial to solve the following problem: some WebCore DOM methods take Strings and some AtomicStrings.  Alas, in IDL there is no such information (we used to have AtomicHint).  So we need to find the best conversion method and those C++ operators fit very nicely.  I drafted a solution with metaprogramming (using SFINAE to find out exact argument type), but it's more verbose and rumors have it VC used to be weak in templates with pointers-to-members parameters (just read it, didn't check if it's indeed the case).

> 
> > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:114
> > +    STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, type, args[0]);
> Why this particular callsite?  Is this the only custom one?

Yes.  I double checked and found another one.  I'll have to check again before I submit it.  BTW, if in review you could ask people to use the macro when converting parameters, that would be most appreciated.

Thank you very much for a next round of comments.

[Adam Barth]

Comment on attachment 65934 [details]
Patch

Thanks.

> WebCore/bindings/scripts/CodeGeneratorV8.pm:3333
> +      my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK";
four-space indent.

[anton muhin]

Created attachment 66018 [details]
Patch

[anton muhin]

(In reply to comment #8)
> (From update of attachment 65934 [details])
> Thanks.
> 
> > WebCore/bindings/scripts/CodeGeneratorV8.pm:3333
> > +      my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK";
> four-space indent.

Sorry, Adam, what do you mean?  Is it Perl statement which requires additional indentation or generated C++ string?  Apparently both are fine: perl's at right indent and C++ doesn't require indentation---it's indented at call site.

Anyway, if I manage to submit it before you reply, I'll fix it in a separate change.

And many thanks for review.

[Adam Barth]

Comment on attachment 66018 [details]
Patch

> WebCore/bindings/scripts/CodeGeneratorV8.pm:3339
> +    if ($signature->type eq "DOMString") {
> +      my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK";
> +      $macro .= "_$suffix" if $suffix;
> +      return "$macro($nativeType, $variableName, $value);"
> +    } else {
> +      # Don't know how to properly check for conversion exceptions when $parameter->type is "DOMUserData"
> +      return "$nativeType $variableName($value, true);";
> +    }
Notice that the contents of the if are only indented with two spaces instead of four.

[WebKit Commit Bot]

Comment on attachment 66018 [details]
Patch

Clearing flags on attachment: 66018

Committed r66521: <http://trac.webkit.org/changeset/66521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66521 might have broken Chromium Linux Release

[Tony Chang]

(In reply to comment #14)
> http://trac.webkit.org/changeset/66521 might have broken Chromium Linux Release

This broke the chromium compile:

third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp: In static member function ‘static v8::Handle<v8::Value> WebCore::V8DeviceOrientationEvent::initDeviceOrientationEventCallback(const v8::Arguments&)’:
third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp:72:error: expected primary-expression before ‘,’ token
third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp:72:error: ‘type’ was not declared in this scope
third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp:72:error: ‘STRING_TO_V8PARAMETER_EXCEPTION_BLOCK’ was not declared in this scope
make: *** [out/Release/obj.target/webcore_remaining/third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.o] Error 1

[Dumitru Daniliuc]

Looks like you didn't update the custom bindings.

[Tony Chang]

(In reply to comment #16)
> Looks like you didn't update the custom bindings.

I have a compile fix coming.

[Tony Chang]

http://trac.webkit.org/changeset/66530

[anton muhin]

(In reply to comment #11)
> (From update of attachment 66018 [details])
> > WebCore/bindings/scripts/CodeGeneratorV8.pm:3339
> > +    if ($signature->type eq "DOMString") {
> > +      my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK";
> > +      $macro .= "_$suffix" if $suffix;
> > +      return "$macro($nativeType, $variableName, $value);"
> > +    } else {
> > +      # Don't know how to properly check for conversion exceptions when $parameter->type is "DOMUserData"
> > +      return "$nativeType $variableName($value, true);";
> > +    }
> Notice that the contents of the if are only indented with two spaces instead of four.

Sorry, didn't notice that.  Fixing ASAP.

[anton muhin]

(In reply to comment #18)
> http://trac.webkit.org/changeset/66530

Thank you very much, Tony.

[anton muhin]

Created attachment 66209 [details]
Patch

[Tony Chang]

(In reply to comment #21)
> Created an attachment (id=66209) [details]
> Patch

It's ok to commit whitespace changes like this without review.

[anton muhin]

Comment on attachment 66209 [details]
Patch

thanks a lot, Tony

[anton muhin]

to kick cq.

[WebKit Commit Bot]

Comment on attachment 66209 [details]
Patch

Clearing flags on attachment: 66209

Committed r66664: <http://trac.webkit.org/changeset/66664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.