Bug 89921 – Fix Blob serialization to enable embedders to persist references.

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Greg Billock]

Fix Blob serialization to enable embedders to persist references.

[Greg Billock]

Created attachment 149391 [details]
Patch

[Michael Nordman]

Comment on attachment 149391 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:610
>              if (obj->inherits(&JSBlob::s_info)) {

What about the CloneSerializer ctor on line 370? How is that used, it accepts a vector of blobURLs as an input argument?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:615
> +                blobRegistry().didSerializeBlob(cloneURL, blob->url());

Should this be a call to ThreadableBlobRegistry? Ditto other callsites?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1481
> +            RefPtr<Blob> blob = Blob::create(KURL(KURL(), url->ustring().impl()));

The 'type' and 'size' params should be in this method call and not in the getJsValue(T*) function call.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1002
> +        blobRegistry().didSerializeBlob(cloneURL, blob->url());

Should this be a call to ThreadableBlobRegistry? Ditto at other callsites in this file.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:201
> +{

This will have to be implemented for native webcore impl to function, is that right? I think its just a call to registryBlobUrl(url, srcUrl).

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:205
> +{

And this remains an empty method body to function properly, is that right?

[Greg Billock]

Comment on attachment 149391 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:610
>>              if (obj->inherits(&JSBlob::s_info)) {
> 
> What about the CloneSerializer ctor on line 370? How is that used, it accepts a vector of blobURLs as an input argument?

This vector of blob urls is currently used by indexed-db to basically forbid blob serialization when they're being used. We need to put the internal url into those locations, since that's the one we're notifying is the clone, but other than that, I don't think we need to worry about that vector.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:615
>> +                blobRegistry().didSerializeBlob(cloneURL, blob->url());
> 
> Should this be a call to ThreadableBlobRegistry? Ditto other callsites?

I need some help here -- I'm not sure. I think that serialization can only happen on the main thread. Am I right about that?

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1481
>> +            RefPtr<Blob> blob = Blob::create(KURL(KURL(), url->ustring().impl()));
> 
> The 'type' and 'size' params should be in this method call and not in the getJsValue(T*) function call.

Oops. Fixed.

>> Source/WebCore/platform/network/BlobRegistryImpl.cpp:201
>> +{
> 
> This will have to be implemented for native webcore impl to function, is that right? I think its just a call to registryBlobUrl(url, srcUrl).

You're right. Filled this in with a forward.

>> Source/WebCore/platform/network/BlobRegistryImpl.cpp:205
>> +{
> 
> And this remains an empty method body to function properly, is that right?

Looked at this more carefully. It would really need to do a registerBlobURL, but it turns out that the details of Blob construction already do this for us. In that context, we would probably prefer to *un*register the url, but it's better to leave that to the SSV value to manage (it could be deserialized again, for example).

[Greg Billock]

Created attachment 149639 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 149639 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13094651

[Early Warning System Bot]

Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13093631

[Gyuyoung Kim]

Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13089646

[Early Warning System Bot]

Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13102551

[WebKit Review Bot]

Comment on attachment 149639 [details]
Patch

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

[Michael Nordman]

> > Should this be a call to ThreadableBlobRegistry? Ditto other callsites?
> 
> I need some help here -- I'm not sure. I think that serialization can only happen on the main thread. Am I right about that?

MessagePorts and IDB related interfaces take SSVs as inputs and outputs, those can be invoked from within worker contexts (bg threads). There's also just worker.postMessage(ssv), i think the far side of that has to deserialize on the bg thread so the resulting things show up in the worker script context, but I'm not sure if that method is directly reachable from within a worker context (did we ever do nested workers?).

[Kinuko Yasuda]

(In reply to comment #12)
> > > Should this be a call to ThreadableBlobRegistry? Ditto other callsites?
> > 
> > I need some help here -- I'm not sure. I think that serialization can only happen on the main thread. Am I right about that?
> 
> MessagePorts and IDB related interfaces take SSVs as inputs and outputs, those can be invoked from within worker contexts (bg threads). There's also just worker.postMessage(ssv), i think the far side of that has to deserialize on the bg thread so the resulting things show up in the worker script context, but I'm not sure if that method is directly reachable from within a worker context (did we ever do nested workers?).

Yes serialization happens on worker thread if, say, postMessage is called on the worker context.

[Build Bot]

Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13095720

[Greg Billock]

(In reply to comment #12)
> > > Should this be a call to ThreadableBlobRegistry? Ditto other callsites?
> > 
> > I need some help here -- I'm not sure. I think that serialization can only happen on the main thread. Am I right about that?
> 
> MessagePorts and IDB related interfaces take SSVs as inputs and outputs, those can be invoked from within worker contexts (bg threads). There's also just worker.postMessage(ssv), i think the far side of that has to deserialize on the bg thread so the resulting things show up in the worker script context, but I'm not sure if that method is directly reachable from within a worker context (did we ever do nested workers?).

grepping the code for wire format usage, the usages we need to worry about are:

1. the History API (HistoryItem::setStateObject)
2. Indexed DB put (IDBObjectStoreBackendInterface::put)
3. Web Messaging
4. Web Intents

I don't think web messaging or web intents will be that hard to account for -- these serializations are intended to be used right away, so we can track them down. The History API may be a bit tougher -- I don't know if there is any allowance for dealing with history in a state-dependent way. The indexed-db use might be harder, but I think that's where they've added the Blob URL checks to make sure blob serializations aren't part of these persisted objects, so we may not need to think about that yet. Who knows the history API to know if that'll be a big obstacle to deal with? Perhaps it should take the indexed-db path of forbidding Blobs...

[Michael Nordman]

Depending solely on a WebCore::SerializedScriptValue instance's deserialize() method being run in the receiving side for proper cleanup is a show stopper i think. There are so many ways in which that may never happen (sending process dies prior to sending the ssv, receiving process is gone by the time the ssv gets there, target thread within receiving process is gone, some other thing represented by a 'routing id' is gone by the time the ssv gets there), and we're left with leaks for which we have no good fix.

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

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1781
> +        blobRegistry().didDeserializeBlob(blob->url());

This doesn't look right. The value of blob->url() will be different then |url| at this point. The create() function will coin yet another new url under the covers. (Too freakin many urls/ids to reason about in this stuff).

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1816
> +        blobRegistry().didDeserializeBlob(file->url());

Ditto file->url() != |url| contained in the SSV.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1838
> +            blobRegistry().didDeserializeBlob(fileList->item(i)->url());

Ditto, the url value passed into didSerialize() and didDeserialize() are different.

> Source/WebCore/fileapi/ThreadableBlobRegistry.cpp:188
> +void ThreadableBlobRegistry::didSerializeBlob(const KURL&)

2nd argument to this method is missing here

[Peter Beverloo (cr-android ews)]

Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13456989

[Eric Seidel (no email)]

Comment on attachment 149639 [details]
Patch

Looks like this breaks the world.

[Greg Billock]

Yeah, this patch is a prototype and I haven't merged it in too long. Michael, shall I split out the parts that we want to use in the end and submit those separately?

[Greg Billock]

Created attachment 163148 [details]
Patch

[WebKit Review Bot]

Attachment 163148 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13799841

[Peter Beverloo (cr-android ews)]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13797891

[Build Bot]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13800870

[Early Warning System Bot]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13802727

[Gyuyoung Kim]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13800882

[WebKit Review Bot]

Comment on attachment 163148 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13805599

[Build Bot]

Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13795957

[Greg Billock]

Getting rid of this pending Michael Nordman's work on blob handling.

[Eric Seidel (no email)]

Comment on attachment 163148 [details]
Patch

Cleared review? from attachment 163148 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).