Bug 72783 – Get rid of WebCore dependencies from TypedArray implementation types

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Get rid of WebCore dependencies from TypedArray implementation types

[Oliver Hunt]

Created attachment 115922 [details]
Patch

[Oliver Hunt]

Needs V8 side to be implemented and i can't work out how that should be done.

[Adam Barth]

I'm happy to help.  What needs to be done?  Should I apply this patch and try to make it build?

[Oliver Hunt]

(In reply to comment #3)
> I'm happy to help.  What needs to be done?  Should I apply this patch and try to make it build?

Making it build would be easy.  Making it correct less so, it really needs dslomov to do the v8 side i think.

[Adam Barth]

@dslomov: Would you be willing to help Oliver with this patch?

[WebKit Review Bot]

Comment on attachment 115922 [details]
Patch

Attachment 115922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10518300

[Oliver Hunt]

(In reply to comment #6)
> (From update of attachment 115922 [details])
> Attachment 115922 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10518300

Least surprising build failure ever :D

[Dmitry Lomov]

Comment on attachment 115922 [details]
Patch

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

I think it is a reasonable approach. Some nits below. Once they are fixed, I'll be happy to implement a V8 side of these changes.

> Source/WebCore/ChangeLog:19
> +        (WTF::ArrayBufferBinding::poison):

Spec name for this operation is "neuter" (http://dev.w3.org/html5/spec/common-dom-interfaces.html#transferable-objects) - let's keep it!

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:10650
> +		A70E3252147744E7005E2988 /* CodeGeneratorV8.pm */ = {isa = PBXFileReference; lastKnownFileType = text.script.perl; name = CodeGeneratorV8.pm; path = scripts/CodeGeneratorV8.pm; sourceTree = "<group>"; };

CodeGeneratorV8.pm is not used in apple port of webkit - do not reference it from project files.

> Source/WebCore/html/canvas/ArrayBuffer.cpp:141
>          ArrayBufferView* current = m_firstView;

There is no more reason to keep a list of ArrayBufferViews - we should just remove them.

[Dmitry Lomov]

Hmm I have thought on this more, and I really do not see a good way to make this approach work for V8. The problem is, V8 bindings does not really have an equivalent of JS* classes to stick an m_binding member on.

For example, JUint8Array is a generated class that represents JSC object corresponding to Uint8Array. V8 just have a generic v8::JSObject and a bunch of callbacks for this. There is no instances of 'V8Uint8Array'.

So it looks like the better solution would be this:
- expose the list of views for array buffer on ArrayBuffer class
- ArrayBuffer::transfer shouldn't attempt to neuter bindings (it should still neuter views by setting everything to zero)
- let the callers to ArrayBuffer (namely SerializedScriptValue) deal with neutering the bindings, iteration over exposed list of views.

I will prepare a patch along these lines.

[Dmitry Lomov]

Created attachment 116289 [details]
Oliver's patch restructured

Forego trying to sneak any callbacks from WebCore into WTF's ArrayBuffer.
Instead, return list of neutered views from ArrayBuffer::transfer and let clients (WebCore's SerializedScriptValue) deal with neutering bindings as they see fit.

[WebKit Review Bot]

Comment on attachment 116289 [details]
Oliver's patch restructured

Clearing flags on attachment: 116289

Committed r101064: <http://trac.webkit.org/changeset/101064>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?

[Dmitry Lomov]

(In reply to comment #13)
> How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?

The neutering will be handled by a caller to ArrayBuffer::transfer, as in

SerializedScriptValue::create(...)
{
    ...
    Vector<ArrayBufferView*> viewsToNeuter;
    arrayBuffer.transfer(..., viewsToNeuter);
    for(view in viewsToNeuter) {
        get a binding for view and neuter it
    }
    ...
}

SerializedScriptValue lives in WebCore and knows all about bindings and JavaScript, so it is in better position to propagate neutering to bindings.

[Oliver Hunt]

(In reply to comment #14)
> (In reply to comment #13)
> > How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?
> 
> The neutering will be handled by a caller to ArrayBuffer::transfer, as in
> 
> SerializedScriptValue::create(...)
> {
>     ...
>     Vector<ArrayBufferView*> viewsToNeuter;
>     arrayBuffer.transfer(..., viewsToNeuter);
>     for(view in viewsToNeuter) {
>         get a binding for view and neuter it
>     }
>     ...
> }
> 
> SerializedScriptValue lives in WebCore and knows all about bindings and JavaScript, so it is in better position to propagate neutering to bindings.

The thing to be careful of is that you have to neuter all bindings in all worlds that may reference the buffer.

Once again I'll say that i think this is a poorly conceived API, that will result in substantial programmer confusion in the long term.

[Dmitry Lomov]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?
> > 
> > The neutering will be handled by a caller to ArrayBuffer::transfer, as in
> > 
> > SerializedScriptValue::create(...)
> > {
> >     ...
> >     Vector<ArrayBufferView*> viewsToNeuter;
> >     arrayBuffer.transfer(..., viewsToNeuter);
> >     for(view in viewsToNeuter) {
> >         get a binding for view and neuter it
> >     }
> >     ...
> > }
> > 
> > SerializedScriptValue lives in WebCore and knows all about bindings and JavaScript, so it is in better position to propagate neutering to bindings.
> 
> The thing to be careful of is that you have to neuter all bindings in all worlds that may reference the buffer.
> 
> Once again I'll say that i think this is a poorly conceived API, that will result in substantial programmer confusion in the long term.

Which do you think a poorly conceived API? The transfer method on ArrayBuffer or the postMessage transferrable API?

For the transfer method on ArrayBuffer, I think this is the best way forward if you want ArrayBuffer to live in WTF and not depend on WebCore.