Bug 89921

Summary: Fix Blob serialization to enable embedders to persist references.
Product: WebKit Reporter: Greg Billock <gbillock>
Component: WebCore Misc.Assignee: Greg Billock <gbillock>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, dglazkov, fishd, gns, gtk-ews, haraken, jamesr, japhet, jochen, jsbell, kinuko, michaeln, peter+ews, philn, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Greg Billock 2012-06-25 16:38:49 PDT
Fix Blob serialization to enable embedders to persist references.
Comment 1 Greg Billock 2012-06-25 16:42:48 PDT
Created attachment 149391 [details]
Patch
Comment 2 Michael Nordman 2012-06-25 18:19:43 PDT
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?
Comment 3 Greg Billock 2012-06-26 17:07:24 PDT
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).
Comment 4 Greg Billock 2012-06-26 17:09:30 PDT
Created attachment 149639 [details]
Patch
Comment 5 WebKit Review Bot 2012-06-26 17:13:00 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 6 WebKit Review Bot 2012-06-26 17:13:56 PDT
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.
Comment 7 Build Bot 2012-06-26 17:41:43 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13094651
Comment 8 Early Warning System Bot 2012-06-26 17:54:26 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13093631
Comment 9 Gyuyoung Kim 2012-06-26 18:00:50 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13089646
Comment 10 Early Warning System Bot 2012-06-26 18:01:13 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13102551
Comment 11 WebKit Review Bot 2012-06-26 19:09:31 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13094669
Comment 12 Michael Nordman 2012-06-26 19:16:24 PDT
> > 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?).
Comment 13 Kinuko Yasuda 2012-06-26 20:50:30 PDT
(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.
Comment 14 Build Bot 2012-06-27 02:42:41 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13095720
Comment 15 Greg Billock 2012-07-02 11:31:59 PDT
(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...
Comment 16 Michael Nordman 2012-07-10 17:52:48 PDT
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
Comment 17 Peter Beverloo (cr-android ews) 2012-08-09 13:13:09 PDT
Comment on attachment 149639 [details]
Patch

Attachment 149639 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13456989
Comment 18 Eric Seidel (no email) 2012-08-12 03:53:35 PDT
Comment on attachment 149639 [details]
Patch

Looks like this breaks the world.
Comment 19 Greg Billock 2012-08-13 08:08:55 PDT
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?
Comment 20 Greg Billock 2012-09-10 09:27:18 PDT
Created attachment 163148 [details]
Patch
Comment 21 WebKit Review Bot 2012-09-10 09:31:29 PDT
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.
Comment 22 Early Warning System Bot 2012-09-10 09:42:48 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13799841
Comment 23 Peter Beverloo (cr-android ews) 2012-09-10 09:43:08 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13797891
Comment 24 Build Bot 2012-09-10 09:46:43 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13800870
Comment 25 Early Warning System Bot 2012-09-10 09:53:41 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13802727
Comment 26 Gyuyoung Kim 2012-09-10 10:29:10 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13800882
Comment 27 WebKit Review Bot 2012-09-10 11:10:13 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13805598
Comment 28 kov's GTK+ EWS bot 2012-09-10 11:12:55 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13805599
Comment 29 Build Bot 2012-09-10 11:58:42 PDT
Comment on attachment 163148 [details]
Patch

Attachment 163148 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13795957
Comment 30 Greg Billock 2012-10-18 11:39:26 PDT
Getting rid of this pending Michael Nordman's work on blob handling.
Comment 31 Eric Seidel (no email) 2013-01-04 00:49:50 PST
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).