IndexedDB: Avoid ScriptValue copies in IDBAny
Created attachment 190569 [details] Patch
With this patch, only 2 IDB tests crash as a result of bug 110206. As this bug does result in a useful cleanup in IDB, we should land it anyway. The cases that now crash are: storage/indexeddb/readonly.html storage/indexeddb/keyrange.html Strangely they both crash accessing keyRange.lower With the patch in bug 110206, we do not crash at all.
jsbell / dgrogan - comments?
Comment on attachment 190569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190569&action=review > Source/WebCore/Modules/indexeddb/IDBAny.cpp:114 > +const ScriptValue& IDBAny::scriptValue() I don't pretend to know what's going on in the rest of this patch, but returning a reference seems scary. You've ensured that it won't be problematic?
Comment on attachment 190569 [details] Patch Attachment 190569 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16758178
yes, (in fact thats the key line in this whole patch) - in fact the only caller (in V8IDBAnyCustom.cpp) does this: impl->scriptValue().v8Value() This is generally ok as long as the scope of the owning object is longer than the return value - such as the above when the return value is a temporary.
Comment on attachment 190569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190569&action=review Seems okay. You might want to wait to land this until after https://bugs.webkit.org/show_bug.cgi?id=110206 lands so that it's easier to verify that bug's fix is in and behaving as expected. > Source/WebCore/Modules/indexeddb/IDBAny.cpp:48 > + return adoptRef(new IDBAny((IDBAny*)0)); A little wonky, but hidden inside the implementation. Seems reasonable. > Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:60 > + Remove this extra whitespace. > Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:61 > +} Looks like compile fails - need ASSERT_UNREACHED() / return v8::Undefined();
Comment on attachment 190569 [details] Patch Attachment 190569 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16802097
Comment on attachment 190569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190569&action=review >> Source/WebCore/Modules/indexeddb/IDBAny.cpp:48 >> + return adoptRef(new IDBAny((IDBAny*)0)); > > A little wonky, but hidden inside the implementation. Seems reasonable. Per discussion (and for the record), maybe have a constructor with signature: IDBAny(IDBAny::Type type) : m_type(type), m_integer(0) { ASSERT(type == NullType || type == UndefinedType); } .. and remove the IDBAny() and IDBAny(IDBAny*) contructors. Then createNull() would be: return adoptRef(new IDBAny(NullType)); and createInvalid() would be: return adoptRef(new IDBAny(UndefinedType));
See also wkbug.com/100292 - IMHO, in the medium term we should be striving to eliminate IDBAny in general, but that requires pushing more smarts into ScriptValue which should wait for wkbug.com/110206 and any other pending refactors. So no harm in doing this now.
Created attachment 190589 [details] Patch
Created attachment 190590 [details] Patch
Comment on attachment 190590 [details] Patch Attachment 190590 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16758226
Comment on attachment 190590 [details] Patch Attachment 190590 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16758230
Created attachment 190597 [details] Patch
abarth@ - r? Now that bug 110206 is fixed, this is less of an issue, but it's still a good code cleanup and does reduce some totally unnecessary, if minor, copies
Comment on attachment 190597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190597&action=review > Source/WebCore/Modules/indexeddb/IDBAny.h:122 > + IDBAny(Type); > + IDBAny(PassRefPtr<DOMStringList>); > + IDBAny(PassRefPtr<IDBCursor>); > + IDBAny(PassRefPtr<IDBCursorWithValue>); > + IDBAny(PassRefPtr<IDBDatabase>); > + IDBAny(PassRefPtr<IDBFactory>); > + IDBAny(PassRefPtr<IDBIndex>); > + IDBAny(PassRefPtr<IDBObjectStore>); > + IDBAny(PassRefPtr<IDBTransaction>); > + IDBAny(const IDBKeyPath&); > + IDBAny(const String&); > + IDBAny(const ScriptValue&); > + IDBAny(int64_t); Should these be marked explicit? > Source/WebCore/Modules/indexeddb/IDBAny.h:127 > + const RefPtr<DOMStringList> m_domStringList; const RefPtr is sort of unusual in WebCore, but I guess it's fine.
Created attachment 191044 [details] Patch for landing
Comment on attachment 190597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190597&action=review >> Source/WebCore/Modules/indexeddb/IDBAny.h:122 >> + IDBAny(int64_t); > > Should these be marked explicit? yes they should. done. >> Source/WebCore/Modules/indexeddb/IDBAny.h:127 >> + const RefPtr<DOMStringList> m_domStringList; > > const RefPtr is sort of unusual in WebCore, but I guess it's fine. We've started using it recently - it's actually really nice and works just as you want it to - like a const pointer.
Comment on attachment 191044 [details] Patch for landing Clearing flags on attachment: 191044 Committed r144517: <http://trac.webkit.org/changeset/144517>
All reviewed patches have been landed. Closing bug.