Bug 156068 - Modern IDB: Dump blobs to disk before storing them in an object store
Summary: Modern IDB: Dump blobs to disk before storing them in an object store
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 143193
  Show dependency tree / graph
 
Reported: 2016-03-31 09:46 PDT by Brady Eidson
Modified: 2016-04-05 09:34 PDT (History)
3 users (show)

See Also:

Attachments
Patch v1 (40.03 KB, patch)
2016-04-04 15:53 PDT, Brady Eidson 		achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (39.78 KB, patch)
2016-04-04 16:39 PDT, Brady Eidson 		beidson: commit-queue+
Details | Formatted Diff | Diff
Patch for landing v2 (39.78 KB, patch)
2016-04-04 21:52 PDT, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-03-31 09:46:27 PDT 
Modern IDB: Dump blobs to disk before storing them in an object store
Comment 1 Brady Eidson 2016-03-31 09:49:14 PDT 
This is a bit complicated due to https://bugs.webkit.org/show_bug.cgi?id=129979, but that change didn't really buy us anything so I'm basically going to be undoing it.
Comment 2 Brady Eidson 2016-04-04 15:53:30 PDT 
Created attachment 275585 [details]
Patch v1
Comment 3 Brady Eidson 2016-04-04 15:54:10 PDT 
This patch dumps Blobs/Files to a temporary file on disk.

That's it - nothing more.

It's required for the actual IDB work going forward, but is a nice reviewable chunk by itself.
Comment 4 WebKit Commit Bot 2016-04-04 15:54:59 PDT 
Attachment 275585 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.cpp:78:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.cpp:244:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.h:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistry.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:2750:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 10 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Alex Christensen 2016-04-04 16:22:40 PDT 
Comment on attachment 275585 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/IDBValue.h:34
> +class IDBValue {
> +public:
> +    IDBValue();
> +};

This is the ideal class.  Please make more like this.

> Source/WebCore/platform/FileSystem.cpp:137
> +    if (!isHandleValid(source))

Probably return false if target is invalid, too.

> Source/WebCore/platform/FileSystem.cpp:141
> +    static int bufferSize = 1 << 20;
> +    Vector<char> buffer(bufferSize);

1MB might be a bit big for a buffer.

> Source/WebCore/platform/FileSystem.cpp:143
> +    ScopeGuard fileCloser([source, buffer]() {

No need to capture buffer.

> Source/WebCore/platform/FileSystem.cpp:145
> +        PlatformFileHandle handle = source;
> +        closeFile(handle);

Is this first line really needed?  We can't just close source?

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:235
> +    static auto& queue = WorkQueue::create("org.webkit.BlobUtility", WorkQueue::Type::Serial, WorkQueue::QOS::Background).leakRef();

NeverDestroyed.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:286
> +                callOnMainThread([completionHandler]() {
> +                    Vector<String> filePaths;
> +                    completionHandler(filePaths);
> +                });

This is redundant.  Lots of calling the same thing on main thread.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:290
> +            for (auto& part : blob.filePathsOrDataBuffers) {

ASSERT(part.first.isNull() || part.second.isEmpty())
Comment 6 Brady Eidson 2016-04-04 16:28:24 PDT 
(In reply to comment #5)
> Comment on attachment 275585 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275585&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBValue.h:34
> > +class IDBValue {
> > +public:
> > +    IDBValue();
> > +};
> 
> This is the ideal class.  Please make more like this.

Will do.

> 
> > Source/WebCore/platform/FileSystem.cpp:137
> > +    if (!isHandleValid(source))
> 
> Probably return false if target is invalid, too.
> 
> > Source/WebCore/platform/FileSystem.cpp:141
> > +    static int bufferSize = 1 << 20;
> > +    Vector<char> buffer(bufferSize);
> 
> 1MB might be a bit big for a buffer.

s/1 << 20/1 << 19/

> > Source/WebCore/platform/FileSystem.cpp:145
> > +        PlatformFileHandle handle = source;
> > +        closeFile(handle);
> 
> Is this first line really needed?  We can't just close source?

Bizarrely yes, because captured arguments lose their const'ness, but closeFile expects an actual const int as its argument.

> > Source/WebCore/platform/network/BlobRegistryImpl.cpp:235
> > +    static auto& queue = WorkQueue::create("org.webkit.BlobUtility", WorkQueue::Type::Serial, WorkQueue::QOS::Background).leakRef();
> 
> NeverDestroyed.

NeverCaughtByTheCompilerAndLinker

> > Source/WebCore/platform/network/BlobRegistryImpl.cpp:286
> > +                callOnMainThread([completionHandler]() {
> > +                    Vector<String> filePaths;
> > +                    completionHandler(filePaths);
> > +                });
> 
> This is redundant.  Lots of calling the same thing on main thread.

Will rework it into the scope guard.
Comment 7 Brady Eidson 2016-04-04 16:39:02 PDT 
Created attachment 275590 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2016-04-04 16:41:34 PDT 
Attachment 275590 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.cpp:78:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.cpp:245:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.h:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistry.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:2750:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 10 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brady Eidson 2016-04-04 21:52:31 PDT 
Created attachment 275636 [details]
Patch for landing v2

Patch for landing again... does style bot really prevent commit queue? That's BS, if so.
Comment 10 Brady Eidson 2016-04-04 21:53:03 PDT 
Going to let style bot complain, *then* mark cq+... just to try it.
Comment 11 WebKit Commit Bot 2016-04-04 21:54:21 PDT 
Attachment 275636 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.cpp:78:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.cpp:245:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.h:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistry.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:2750:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 10 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Commit Bot 2016-04-04 22:47:38 PDT 
Comment on attachment 275636 [details]
Patch for landing v2

Clearing flags on attachment: 275636

Committed r199043: <http://trac.webkit.org/changeset/199043>
Comment 13 Csaba Osztrogonác 2016-04-05 08:58:58 PDT 
(In reply to comment #12)
> Comment on attachment 275636 [details]
> Patch for landing v2
> 
> Clearing flags on attachment: 275636
> 
> Committed r199043: <http://trac.webkit.org/changeset/199043>

FYI: It broke the Apple Mac cmake and the WinCairo build.
See build.webkit.org for details.
Comment 14 Csaba Osztrogonác 2016-04-05 09:00:33 PDT 
and the Apple Windows build too