Add the backchannel for Blobs to be received into Blink from the database backend.

Add the backchannel for Blobs to be received into Blink from the database backend.

BUG=108012
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172656

[cmumford, 2014-04-15 16:05:01]

lgtm

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:225: void IDBRequest::handleBlobAcks()
Why do we check for non-zero size in IDBCursor::handleBlobAcks(), but not here?

[jsbell, 2014-04-15 18:29:07]

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:387:
m_request->ackReceivedBlobs(m_blobInfo.get());
I think m_request could be null if called from destructor. This is often racy
since either the cursor's or request's script wrapper could get GC'd last, which
lets the cursor+request break a ref cycle. If the cursor goes first then
m_request is cleared - see checkForReferenceCycle().

(We should probably have a unit test for that.)

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:196:
m_transaction->db()->backend()->ackReceivedBlobs(uuids);
Could just be m_transaction->backendDB()

That said... cmumford has been tracing lifetime issues. It seems plugins can
call into IDB script functions after the backend is dropped, so this may need to
be resilient against that.

It seems like ackReceivedBlobs() should move to IDBDatabase since it doesn't
depend on the internal state of the request, and the IDBDatabase could guard
against null backend.

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/WebIDBCallbacksImpl.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:56: 
Remove this extra blank line

[ericu, 2014-04-16 23:04:13]

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:387:
m_request->ackReceivedBlobs(m_blobInfo.get());
On 2014/04/15 18:29:07, jsbell wrote:
> I think m_request could be null if called from destructor. This is often racy
> since either the cursor's or request's script wrapper could get GC'd last,
which
> lets the cursor+request break a ref cycle. If the cursor goes first then
> m_request is cleared - see checkForReferenceCycle().

Good point; in that case, we'd not only crash, we'd also leak.  I've added a
handleBlobAcks to anywhere we clear m_request, and put an ASSERT here.

> (We should probably have a unit test for that.)

For which?

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:196:
m_transaction->db()->backend()->ackReceivedBlobs(uuids);
On 2014/04/15 18:29:07, jsbell wrote:
> Could just be m_transaction->backendDB()
> 
> That said... cmumford has been tracing lifetime issues. It seems plugins can
> call into IDB script functions after the backend is dropped, so this may need
to
> be resilient against that.
> 
> It seems like ackReceivedBlobs() should move to IDBDatabase since it doesn't
> depend on the internal state of the request, and the IDBDatabase could guard
> against null backend.

Done.

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:225: void IDBRequest::handleBlobAcks()
On 2014/04/15 16:05:02, cmumford wrote:
> Why do we check for non-zero size in IDBCursor::handleBlobAcks(), but not
here?

It seems a reasonable optimization to check that there's something to do before
calling out to another class to do that processing.  However, here we've got the
check in ackReceivedBlobs for safety, so other methods in the same file [where
you can just look and see the check's already being done] need not necessarily
bother checking.

However, now that I've moved ackReceivedBlobs over to IDBDatabase, I've
duplicated the check here, out of an excess of caution.

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/WebIDBCallbacksImpl.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:56: 
On 2014/04/15 18:29:07, jsbell wrote:
> Remove this extra blank line

Done.

[jsbell, 2014-04-17 19:00:33]

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/235933013/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:387:
m_request->ackReceivedBlobs(m_blobInfo.get());
On 2014/04/16 23:04:14, ericu wrote:
> > (We should probably have a unit test for that.)
> 
> For which?

For the reference cycle breaking - although oilpan should make it moot.

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:364: m_blobInfo = blobInfo;
If `value` is never accessed, the blobs remain unacked. If the cursor is
iterated, this overwrites m_blobInfo. Doesn't that mean the previous batch of
blobs won't be acked even when the cursor is freed?

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:389: ASSERT(m_request);
Can we add a complementary ASSERT (outside the if), that if m_request is NULL
there are no blobs to ack?

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:390:
m_request->transaction()->db()->ackReceivedBlobs(m_blobInfo.get());
Can use m_transaction

[ericu, 2014-04-17 19:30:14]

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:364: m_blobInfo = blobInfo;
On 2014/04/17 19:00:33, jsbell wrote:
> If `value` is never accessed, the blobs remain unacked. If the cursor is
> iterated, this overwrites m_blobInfo. Doesn't that mean the previous batch of
> blobs won't be acked even when the cursor is freed?

Right, thanks; I've added a handleBlobAcks() call before the assignment to fix
it.  This does feel a little fragile, though.  It seems like what I want is a
custom destruction action on some kind of a scoped object, and then I just have
to remember to destroy it after creating the ScriptValue too.  But that seems
rather heavyweight to write just for this.

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:389: ASSERT(m_request);
On 2014/04/17 19:00:33, jsbell wrote:
> Can we add a complementary ASSERT (outside the if), that if m_request is NULL
> there are no blobs to ack?
> 

Done.

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:390:
m_request->transaction()->db()->ackReceivedBlobs(m_blobInfo.get());
On 2014/04/17 19:00:33, jsbell wrote:
> Can use m_transaction

Done.

[jsbell, 2014-04-17 20:19:23]

lgtm

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/235933013/diff/60001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBCursor.cpp:364: m_blobInfo = blobInfo;
On 2014/04/17 19:30:14, ericu wrote:
> On 2014/04/17 19:00:33, jsbell wrote:
> > If `value` is never accessed, the blobs remain unacked. If the cursor is
> > iterated, this overwrites m_blobInfo. Doesn't that mean the previous batch
of
> > blobs won't be acked even when the cursor is freed?
> 
> Right, thanks; I've added a handleBlobAcks() call before the assignment to fix
> it.  This does feel a little fragile, though.  It seems like what I want is a
> custom destruction action on some kind of a scoped object, and then I just
have
> to remember to destroy it after creating the ScriptValue too.  But that seems
> rather heavyweight to write just for this.

Yeah. See comment on another review about propagating the IndexedDBValue (bits +
blobs) concept around.

[ericu, 2014-04-17 21:17:13]

The CQ bit was checked by ericu@chromium.org

[ericu, 2014-04-17 21:17:21]

The CQ bit was unchecked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-17 21:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/235933013/80001

[ericu, 2014-04-17 21:18:20]

+abarth as owner.

[abarth-chromium, 2014-04-24 00:42:53]

public/ LGTM

(Sorry for the delay.)

[ericu, 2014-04-25 15:01:02]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-25 15:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/235933013/80001

[commit-bot: I haz the power, 2014-04-25 15:01:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 15:01:35]

Failed to apply patch for Source/modules/indexeddb/IDBAny.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/indexeddb/IDBAny.h
  Hunk #2 succeeded at 67 with fuzz 2.
  Hunk #3 FAILED at 80.
  Hunk #4 succeeded at 116 (offset 2 lines).
  Hunk #5 succeeded at 133 with fuzz 2 (offset 2 lines).
  Hunk #6 succeeded at 149 (offset 2 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
Source/modules/indexeddb/IDBAny.h.rej

Patch:       Source/modules/indexeddb/IDBAny.h
Index: Source/modules/indexeddb/IDBAny.h
diff --git a/Source/modules/indexeddb/IDBAny.h
b/Source/modules/indexeddb/IDBAny.h
index
2a94777b4c348f3a277832fc7a93e70796bcfecd..d9bbb2a7ebdc32c4f281092fb31d797e09fd5cb6
100644
--- a/Source/modules/indexeddb/IDBAny.h
+++ b/Source/modules/indexeddb/IDBAny.h
@@ -35,6 +35,12 @@
 #include "wtf/RefPtr.h"
 #include "wtf/text/WTFString.h"
 
+namespace blink {
+
+class WebBlobInfo;
+
+}
+
 namespace WebCore {
 
 class DOMStringList;
@@ -61,6 +67,10 @@ public:
     {
         return adoptRef(new IDBAny(idbObject));
     }
+    static PassRefPtr<IDBAny> create(PassRefPtr<SharedBuffer> value, const
Vector<blink::WebBlobInfo>* blobInfo)
+    {
+        return adoptRef(new IDBAny(value, blobInfo));
+    }
     template<typename T>
     static PassRefPtr<IDBAny> create(PassRefPtr<T> idbObject)
     {
@@ -70,9 +80,9 @@ public:
     {
         return adoptRef(new IDBAny(value));
     }
-    static PassRefPtr<IDBAny> create(PassRefPtr<SharedBuffer> value,
PassRefPtr<IDBKey> key, const IDBKeyPath& keyPath)
+    static PassRefPtr<IDBAny> create(PassRefPtr<SharedBuffer> value, const
Vector<blink::WebBlobInfo>* blobInfo, PassRefPtr<IDBKey> key, const IDBKeyPath&
keyPath)
     {
-        return adoptRef(new IDBAny(value, key, keyPath));
+        return adoptRef(new IDBAny(value, blobInfo, key, keyPath));
     }
     ~IDBAny();
 
@@ -104,6 +114,7 @@ public:
     IDBObjectStore* idbObjectStore() const;
     IDBTransaction* idbTransaction() const;
     SharedBuffer* buffer() const;
+    const Vector<blink::WebBlobInfo>* blobInfo() const;
     int64_t integer() const;
     const String& string() const;
     const IDBKey* key() const;
@@ -120,13 +131,13 @@ private:
     explicit IDBAny(PassRefPtr<IDBKey>);
     explicit IDBAny(const IDBKeyPath&);
     explicit IDBAny(const String&);
-    explicit IDBAny(PassRefPtr<SharedBuffer>);
-    explicit IDBAny(PassRefPtr<SharedBuffer>, PassRefPtr<IDBKey>, const
IDBKeyPath&);
+    IDBAny(PassRefPtr<SharedBuffer>, const Vector<blink::WebBlobInfo>*);
+    IDBAny(PassRefPtr<SharedBuffer>, const Vector<blink::WebBlobInfo>*,
PassRefPtr<IDBKey>, const IDBKeyPath&);
     explicit IDBAny(int64_t);
 
     const Type m_type;
 
-    // Only one of the following should ever be in use at any given time.
+    // Only one of the following should ever be in use at any given time,
except that BufferType uses two and BufferKeyAndKeyPathType uses four.
     const RefPtr<DOMStringList> m_domStringList;
     const RefPtr<IDBCursor> m_idbCursor;
     const RefPtr<IDBDatabase> m_idbDatabase;
@@ -136,6 +147,7 @@ private:
     const RefPtr<IDBKey> m_idbKey;
     const IDBKeyPath m_idbKeyPath;
     const RefPtr<SharedBuffer> m_buffer;
+    const Vector<blink::WebBlobInfo>* m_blobInfo;
     const String m_string;
     const int64_t m_integer;
 };

[ericu, 2014-04-25 17:20:59]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-25 17:21:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/235933013/120001

[commit-bot: I haz the power, 2014-04-25 17:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 17:59:29]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[ericu, 2014-04-25 18:36:40]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-25 18:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/235933013/120001

[commit-bot: I haz the power, 2014-04-25 20:11:15]

Change committed as 172656