Bug 54102 – IndexedDB: Update the semantics of put()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

IndexedDB: Update the semantics of put()

[Hans Wennborg]

Created attachment 81811 [details]
Patch

[Hans Wennborg]

As usual, I'm a bit uncertain about the PassRefPtr stuff. In the case of the selectKeyForPut function, I figured it should take all parameters as raw pointers since it doesn't take ownership of anything, and it returns a PassRefPtr because the result is expected to go in a RefPtr. Does that make sense?

[Jeremy Orlow]

Comment on attachment 81811 [details]
Patch

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

r=me

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> +        // FIXME: Generate auto increment key, and inject it through the key path.

Can you do this in a follow up patch?

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> +    RefPtr<SerializedScriptValue> value = prpValue;

The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)

Does stuff below reference prpValue or value?  If it's the former, then you have a bug.

[Jeremy Orlow]

(In reply to comment #2)
> As usual, I'm a bit uncertain about the PassRefPtr stuff. In the case of the selectKeyForPut function, I figured it should take all parameters as raw pointers since it doesn't take ownership of anything, and it returns a PassRefPtr because the result is expected to go in a RefPtr. Does that make sense?

Yup, that's perfect.

[Jeremy Orlow]

(In reply to comment #3)
> (From update of attachment 81811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review
> 
> r=me
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> > +        // FIXME: Generate auto increment key, and inject it through the key path.
> 
> Can you do this in a follow up patch?
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> > +    RefPtr<SerializedScriptValue> value = prpValue;
> 
> The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)
> 
> Does stuff below reference prpValue or value?  If it's the former, then you have a bug.

To be clear: the reason for this is that when a PassRefPtr is assigned to another PassRefPtr or a RefPt, it gives up its reference and is zeroed out.  But if you just do a .get() or a -> that doesn't happen.

[Hans Wennborg]

Committed r78187: <http://trac.webkit.org/changeset/78187>

[Hans Wennborg]

(In reply to comment #3)
> (From update of attachment 81811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review
> 
> r=me
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> > +        // FIXME: Generate auto increment key, and inject it through the key path.
> 
> Can you do this in a follow up patch?
Will do.

> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> > +    RefPtr<SerializedScriptValue> value = prpValue;
> 
> The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)
Hmm, we just do -> and .get() on value, so we could get rid of that RefPtr.

But this makes me wonder why so many of the parameters to putInternal() are PassRefPtr, even though we don't take ownership. Is this because of the mechanism with transaction and callback tasks?

> 
> Does stuff below reference prpValue or value?  If it's the former, then you have a bug.
Just value.

[Jeremy Orlow]

(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 81811 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> > > +        // FIXME: Generate auto increment key, and inject it through the key path.
> > 
> > Can you do this in a follow up patch?
> Will do.
> 
> > 
> > > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> > > +    RefPtr<SerializedScriptValue> value = prpValue;
> > 
> > The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)
> Hmm, we just do -> and .get() on value, so we could get rid of that RefPtr.
> 
> But this makes me wonder why so many of the parameters to putInternal() are PassRefPtr, even though we don't take ownership. Is this because of the mechanism with transaction and callback tasks?

Hmm.  Actually, that might be it.  Maybe see if you can remove them and report back?

> > 
> > Does stuff below reference prpValue or value?  If it's the former, then you have a bug.
> Just value.