Bug 173998 – Add tests to detect mistakes in backward compatibility when the structured clone algorithm is changed in the future

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

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.

[Jiewen Tan]

Need to tackle the encrypted CryptoKey objects.

[Jiewen Tan]

Created attachment 314349 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/33122081>

[Jiewen Tan]

Created attachment 315027 [details]
Patch

[Jiewen Tan]

Created attachment 315113 [details]
Patch

[Jiewen Tan]

Created attachment 315291 [details]
Patch

[Jiewen Tan]

Created attachment 315294 [details]
Patch

[Brady Eidson]

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

[Jiewen Tan]

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.

[Brady Eidson]

(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.

[Brady Eidson]

But you also need the Cocoa API because:

> You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration.

[Jiewen Tan]

(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.

[Jiewen Tan]

Created attachment 316516 [details]
Patch

[Jiewen Tan]

Created attachment 316661 [details]
Patch

[Darin Adler]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 316825 [details]
Patch for landing

[WebKit Commit Bot]

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

[Jiewen Tan]

Committed r220109: <http://trac.webkit.org/changeset/220109>