Bug 189519 – Make IPC::SharedBufferDataReference a type that decodes into but does not inherit from IPC::DataReference

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Introduce WebCore::DataView, a safer replacement for IPC::DataReference

[Alex Christensen]

Created attachment 349431 [details]
Patch

[Alex Christensen]

Created attachment 349434 [details]
Patch

[Alex Christensen]

Created attachment 349440 [details]
Patch

[Alex Christensen]

Created attachment 349445 [details]
Patch

[Alex Christensen]

Created attachment 349447 [details]
Patch

[Alex Christensen]

Created attachment 349453 [details]
Patch

[Alex Christensen]

Created attachment 349463 [details]
Patch

[Chris Dumez]

Comment on attachment 349463 [details]
Patch

As far as I understand it (please correct me if I am wrong), there is no cost to calling data() / size() on the recipient side. On the recipient side, there is always only 1 segment. Therefore, requiring recipients to iterate over segments even though there is only one seems wrong and confusing.
Recipients that need a buffer may actually iterate unnecessarily over the dataView.

My understanding that is it would only be bad to call data() / size() on sender side, but this is not something that people do.

[Chris Dumez]

Added a few people knowledgeable with our IPC for opinions.

[Chris Dumez]

I think one alternative would be to tweak our IPC code generator to always pass a DataReference to the recipient, even if the .in uses SharedDataReference.

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> I think one alternative would be to tweak our IPC code generator to always
> pass a DataReference to the recipient, even if the .in uses
> SharedDataReference.

* SharedBufferDataReference

[Alex Christensen]

Comment on attachment 349463 [details]
Patch

*.messages.in always uses DataReferences right now.  We create a SharedBufferDataReference as that DataReference right now where we know we are doing nothing but encoding it and don't want to copy the data into one segment.  This leaves us to manually have to know we can't call .data() on some DataReferences but not others.  This is the problem this patch is intended to fix.
"I want to just assume all my data is in one segment" is also the reason we have SharedBuffer::data, which this will also help get rid of.  That's another source of inefficient code.
Re-setting r? hoping Chris will reconsider.

[Chris Dumez]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 349463 [details]
> Patch
> 
> *.messages.in always uses DataReferences right now.  We create a
> SharedBufferDataReference as that DataReference right now where we know we
> are doing nothing but encoding it and don't want to copy the data into one
> segment.  This leaves us to manually have to know we can't call .data() on
> some DataReferences but not others.  This is the problem this patch is
> intended to fix.
> "I want to just assume all my data is in one segment" is also the reason we
> have SharedBuffer::data, which this will also help get rid of.  That's
> another source of inefficient code.
> Re-setting r? hoping Chris will reconsider.

One the recipient side, there is always one segment, right? If so, I disagree that forcing the recipient to iterate over segments is a good idea.

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Alex Christensen from comment #12)
> > Comment on attachment 349463 [details]
> > Patch
> > 
> > *.messages.in always uses DataReferences right now.  We create a
> > SharedBufferDataReference as that DataReference right now where we know we
> > are doing nothing but encoding it and don't want to copy the data into one
> > segment.  This leaves us to manually have to know we can't call .data() on
> > some DataReferences but not others.  This is the problem this patch is
> > intended to fix.
> > "I want to just assume all my data is in one segment" is also the reason we
> > have SharedBuffer::data, which this will also help get rid of.  That's
> > another source of inefficient code.
> > Re-setting r? hoping Chris will reconsider.
> 
> One the recipient side, there is always one segment, right? If so, I
> disagree that forcing the recipient to iterate over segments is a good idea.

As in, if the recipient needs a uint8_t* / length, it will now likely have to construct a Vector and iterate over the dataView, appending to the vector. All this work / extra code would be unnecessary.

[Chris Dumez]

Comment on attachment 349463 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This will help us write better code.

I would argue this forces recipients to write less-efficient code if they need a uint64_t* / length because they'd now have to construct a vector and iterate over the DataView, appending each segment, even though there is always 1.

SharedBufferDataReference is merely an encoding optimization at call sites, and the data() / size() method are already deleted on that class to discourage the sender from accessing the data (which is an unlikely use-case anyway).

[Alex Christensen]

Created attachment 349502 [details]
Patch

[Alex Christensen]

Could someone look into why this latest patch doesn't work on Linux?

[EWS Watchlist]

Comment on attachment 349502 [details]
Patch

Attachment 349502 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9187032

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 349523 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349502 [details]
Patch

Attachment 349502 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9187617

New failing tests:
fast/text/variations/ipc2.html

[EWS Watchlist]

Created attachment 349526 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Catanzaro]

Looks like something is trying to copy an IPC::Attachment, but it is noncopyable:

In file included from ../../Source/WebKit/Platform/IPC/Decoder.h:32:0,
                 from ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:28,
                 from ../../Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:28,
                 from ../../Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:29,
                 from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/NetworkConnectionToWebProcessMessageReceiver.cpp:27:
DerivedSources/ForwardingHeaders/wtf/Vector.h: In instantiation of ‘static void WTF::VectorCopier<false, T>::uninitializedCopy(const T*, const T*, U*) [with U = IPC::Attachment; T = IPC::Attachment]’:
DerivedSources/ForwardingHeaders/wtf/Vector.h:265:79:   required from ‘static void WTF::VectorTypeOperations<T>::uninitializedCopy(const T*, const T*, T*) [with T = IPC::Attachment]’
DerivedSources/ForwardingHeaders/wtf/Vector.h:885:42:   required from ‘WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous> >::Vector(const WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous> >&) [with T = IPC::Attachment; long unsigned int inlineCapacity = 0ul; OverflowHandler = WTF::CrashOnOverflow; long unsigned int minCapacity = 16ul]’
../../Source/WebKit/Platform/IPC/Encoder.h:38:7:   required from here
DerivedSources/ForwardingHeaders/wtf/Vector.h:162:13: error: use of deleted function ‘constexpr IPC::Attachment::Attachment(const IPC::Attachment&)’
             new (NotNull, dst) U(*src);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../Source/WebKit/Platform/IPC/Decoder.h:29:0,
                 from ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:28,
                 from ../../Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:28,
                 from ../../Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:29,
                 from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/NetworkConnectionToWebProcessMessageReceiver.cpp:27:
../../Source/WebKit/Platform/IPC/Attachment.h:43:7: note: ‘constexpr IPC::Attachment::Attachment(const IPC::Attachment&)’ is implicitly declared as deleted because ‘IPC::Attachment’ declares a move constructor or move assignment operator
 class Attachment {
       ^~~~~~~~~~

[Chris Dumez]

Comment on attachment 349502 [details]
Patch

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

> Source/WebKit/Platform/IPC/DataReference.h:77
>      virtual ~DataReference() { }

Does this still need to be virtual?

> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39
> +    SharedBufferDataReference(const uint8_t* data, size_t size)

Do we really need this constructor? Or are you only keeping it temporarily to help port the existing code incrementally?

> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:60
> +    Variant<std::pair<const uint8_t*, size_t>, Ref<const WebCore::SharedBuffer>> m_data;

Ditto. Why cannot it be simply a SharedBuffer?

[Chris Dumez]

Comment on attachment 349502 [details]
Patch

We just have to figure out the GTK build failure and the layout test failures but it looks very nice otherwise.

[Chris Dumez]

Comment on attachment 349502 [details]
Patch

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

> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:45
> +            [encoder] (const Ref<const WebCore::SharedBuffer>& buffer) mutable {

I suspect &encoder might fix the build issues. We should not copy the encoder.

> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:51
> +            }, [encoder] (const std::pair<const uint8_t*, size_t>& pair) mutable {

Ditto.

[Alex Christensen]

(In reply to Chris Dumez from comment #23)
> Comment on attachment 349502 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349502&action=review
> 
> > Source/WebKit/Platform/IPC/DataReference.h:77
> >      virtual ~DataReference() { }
> 
> Does this still need to be virtual?
Probably not.

> > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39
> > +    SharedBufferDataReference(const uint8_t* data, size_t size)
> 
> Do we really need this constructor? Or are you only keeping it temporarily
> to help port the existing code incrementally?
> 
> > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:60
> > +    Variant<std::pair<const uint8_t*, size_t>, Ref<const WebCore::SharedBuffer>> m_data;
> 
> Ditto. Why cannot it be simply a SharedBuffer?
Messages::StorageProcess::DidReceiveFetchData is called once from a location that has a SharedBuffer and once from a location that has a pointer/size pair.  I think this approach is more versatile than splitting that into two different calls that do the same thing with different parameter types.

[Alex Christensen]

Created attachment 349555 [details]
Patch

[Alex Christensen]

Created attachment 349558 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Created attachment 349560 [details]
Patch

[Chris Dumez]

Comment on attachment 349560 [details]
Patch

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

r=me

> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39
> +    SharedBufferDataReference(const uint8_t* data, size_t size)

I personally preference would have been to not have this constructor and use 2 separate IPC messages for the specific case that sometimes sends a SharedBuffer, somethings data+size. It looks confusing to me to construct a SharedBufferDataReference from anything else than a SharedBuffer but your call..

[WebKit Commit Bot]

Comment on attachment 349560 [details]
Patch

Clearing flags on attachment: 349560

Committed r235951: <https://trac.webkit.org/changeset/235951>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44394559>