We should have a test that ensures IndexedDB will always be able to read from an existing good shaped database, and reconstruct the expect values.
Need to tackle the encrypted CryptoKey objects.
Created attachment 314349 [details] Patch
<rdar://problem/33122081>
Created attachment 315027 [details] Patch
Created attachment 315113 [details] Patch
Created attachment 315291 [details] Patch
Created attachment 315294 [details] Patch
Comment on attachment 315294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315294&action=review In general, please re-write using the Cocoa API, then I'll look closer at the HTML. > Source/WebCore/ChangeLog:3 > + IndexedDB: ensure its backward compatibility This is not really descriptive. I'd say something like "Ensure backward compatibility in the serialization format of structured clones." > Source/WebCore/bindings/js/SerializedScriptValue.cpp:120 > +// Who edits this list, please teach IndexedDB.BackwardCompatibility API test to learn the new stuff. "When making changes to this list please cover your new type(s) in the API test "IndexedDB.BackwardCompatibility" > Tools/ChangeLog:8 > + A test is composed to ensure IndexedDB's backward compatibility. The way it works is to Again, this is not really about "IndexedDB backward compatibility" as it is about "backward compatibility in the serialization format of structured clones" > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IndexedDBBackwardCompatibility.mm:73 > + // Somehow ~/Library/WebKit/TestWebKitAPI/WebsiteData/IndexedDB/ doesn't work on my machine. @Brady any idea? > + NSURL *targetURL = [NSURL fileURLWithPath:[@"~/Library/WebKit/Databases/___IndexedDB/file__0/backward_compatibility/" stringByExpandingTildeInPath]]; > + [[NSFileManager defaultManager] createDirectoryAtURL:targetURL withIntermediateDirectories:YES attributes:nil error:nil]; You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration. (See comment below on other reasons to not use the C API) > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IndexedDBBackwardCompatibility.mm:86 > + WKPageNavigationClientV0 navigationClient; > + memset(&navigationClient, 0, sizeof(navigationClient)); > + navigationClient.base.version = 0; > + navigationClient.decidePolicyForNavigationAction = decidePolicyForNavigationAction; > + navigationClient.decidePolicyForNavigationResponse = decidePolicyForNavigationResponse; > + navigationClient.copyWebCryptoMasterKey = copyWebCryptoMasterKey; The Cocoa API is the future, and the C-API will eventually be removed. Writing this test using the C-API means it will just have to be re-written later. Please re-write now using the Cocoa API
Comment on attachment 315294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315294&action=review >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IndexedDBBackwardCompatibility.mm:73 >> + [[NSFileManager defaultManager] createDirectoryAtURL:targetURL withIntermediateDirectories:YES attributes:nil error:nil]; > > You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration. > > (See comment below on other reasons to not use the C API) The reason why I didn't use the Cocoa API is copyWebCryptoMasterKey is not available in Cocoa API.
(In reply to Jiewen Tan from comment #9) > Comment on attachment 315294 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315294&action=review > > >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IndexedDBBackwardCompatibility.mm:73 > >> + [[NSFileManager defaultManager] createDirectoryAtURL:targetURL withIntermediateDirectories:YES attributes:nil error:nil]; > > > > You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration. > > > > (See comment below on other reasons to not use the C API) > > The reason why I didn't use the Cocoa API is copyWebCryptoMasterKey is not > available in Cocoa API. Please expose it as SPI. We really need to be abandoning the C API and filling in all the holes in the Cocoa API.
But you also need the Cocoa API because: > You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration.
(In reply to Brady Eidson from comment #10) > (In reply to Jiewen Tan from comment #9) > > Comment on attachment 315294 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315294&action=review > > > > >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IndexedDBBackwardCompatibility.mm:73 > > >> + [[NSFileManager defaultManager] createDirectoryAtURL:targetURL withIntermediateDirectories:YES attributes:nil error:nil]; > > > > > > You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration. > > > > > > (See comment below on other reasons to not use the C API) > > > > The reason why I didn't use the Cocoa API is copyWebCryptoMasterKey is not > > available in Cocoa API. > > Please expose it as SPI. We really need to be abandoning the C API and > filling in all the holes in the Cocoa API. Okay.
Created attachment 316516 [details] Patch
Created attachment 316661 [details] Patch
Comment on attachment 316661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316661&action=review Great idea. Do these tests cover enough? > Source/WebCore/ChangeLog:3 > + Ensure backward compatibility in the serialization format of structured clones It’s great to add a few backward compatibility tests and add a comment reminding people not to break compatibility and pointing them at the tests. However, I personally would not call that “ensuring backward compatibility”. I would write something more like “add tests to help us detect mistakes in backward compatibility in the future when we change the structured clone algorithm, since it’s used for data stored in persistent databases”. > Source/WebCore/ChangeLog:8 > + No changes of behavior. Don’t need this comment. It’s clear that just adding a comment does not change behavior. Please just delete this line. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:120 > +// When making changes to this list please cover your new type(s) in the API test "IndexedDB.BackwardCompatibility" This comment is good but insufficient. After all, it’s not just changes to this list that can break compatibility, but rather any changes at all. Also, the key thing to mention is that some serialized values are stored in a persistent database, which is *why* we need compatibility. I also am unclear on how we will handle migration. Do we only need to be able to read old serialized data, or will we ever share an IndexedDB database between an older and newer version of WebKit? If we might be sharing one, then we need compatibility in both directions. Not only do we need to be able to read old data, but the old version needs to be able to safely read data from the new version without crashing and has to have some strategy for dealing with data it does not understand. This comment about the tag is the smaller issue. Really any change to any of the serialization code in this entire file has to consider compatibility. Or maybe not. If types in IndexedDB are limited, then only a part of the code in this file depends on that. It would be good to draw a clear distinction between the part that has to be compatible and the part that does not.
Comment on attachment 316661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316661&action=review Thanks Darin for r+ my patch. >> Source/WebCore/ChangeLog:3 >> + Ensure backward compatibility in the serialization format of structured clones > > It’s great to add a few backward compatibility tests and add a comment reminding people not to break compatibility and pointing them at the tests. > > However, I personally would not call that “ensuring backward compatibility”. > > I would write something more like “add tests to help us detect mistakes in backward compatibility in the future when we change the structured clone algorithm, since it’s used for data stored in persistent databases”. Fixed. >> Source/WebCore/ChangeLog:8 >> + No changes of behavior. > > Don’t need this comment. It’s clear that just adding a comment does not change behavior. Please just delete this line. Deleted. >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:120 >> +// When making changes to this list please cover your new type(s) in the API test "IndexedDB.BackwardCompatibility" > > This comment is good but insufficient. > > After all, it’s not just changes to this list that can break compatibility, but rather any changes at all. > > Also, the key thing to mention is that some serialized values are stored in a persistent database, which is *why* we need compatibility. I also am unclear on how we will handle migration. Do we only need to be able to read old serialized data, or will we ever share an IndexedDB database between an older and newer version of WebKit? If we might be sharing one, then we need compatibility in both directions. Not only do we need to be able to read old data, but the old version needs to be able to safely read data from the new version without crashing and has to have some strategy for dealing with data it does not understand. > > This comment about the tag is the smaller issue. Really any change to any of the serialization code in this entire file has to consider compatibility. > > Or maybe not. If types in IndexedDB are limited, then only a part of the code in this file depends on that. It would be good to draw a clear distinction between the part that has to be compatible and the part that does not. Only enums after this comment need to be protected. Surprisedly, some of them cannot be stored in the indexedDB. Those are FileList, ObjectReference, MessagePortReference, ArrayBufferView, ArrayBufferTransfer, NonMapProperties, NonSetProperties, SharedArrayBuffer, WasmModule, DOMPointReadOnly, DOMPoint, DOMRectReadOnly, DOMRect, DOMMatrixReadOnly, DOMMatrix, DOMQuad and Error. CryptoKey objects are not included as well as they need new API for encrypting/decrypting the internal key data slot. I added the above sentences to the ChangeLog as well.
Created attachment 316825 [details] Patch for landing
Comment on attachment 316825 [details] Patch for landing Rejecting attachment 316825 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 316825, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/4235513
Committed r220109: <http://trac.webkit.org/changeset/220109>