Bug 173998 - Add tests to detect mistakes in backward compatibility when the structured clone algorithm is changed in the future
Summary: Add tests to detect mistakes in backward compatibility when the structured cl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-29 17:20 PDT by Jiewen Tan
Modified: 2017-08-01 13:00 PDT (History)
9 users (show)

See Also:


Attachments
Patch (47.03 KB, patch)
2017-06-30 19:25 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (47.21 KB, patch)
2017-07-10 14:19 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (47.22 KB, patch)
2017-07-11 08:49 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (46.05 KB, patch)
2017-07-12 15:50 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (46.07 KB, patch)
2017-07-12 15:56 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (42.08 KB, patch)
2017-07-26 21:13 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (39.46 KB, patch)
2017-07-28 11:45 PDT, Jiewen Tan
darin: review+
Details | Formatted Diff | Diff
Patch for landing (40.11 KB, patch)
2017-07-31 18:42 PDT, Jiewen Tan
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2017-06-29 17:20:43 PDT
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.
Comment 1 Jiewen Tan 2017-06-29 21:52:56 PDT
Need to tackle the encrypted CryptoKey objects.
Comment 2 Jiewen Tan 2017-06-30 19:25:03 PDT
Created attachment 314349 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2017-07-04 04:32:19 PDT
<rdar://problem/33122081>
Comment 4 Jiewen Tan 2017-07-10 14:19:09 PDT
Created attachment 315027 [details]
Patch
Comment 5 Jiewen Tan 2017-07-11 08:49:34 PDT
Created attachment 315113 [details]
Patch
Comment 6 Jiewen Tan 2017-07-12 15:50:56 PDT
Created attachment 315291 [details]
Patch
Comment 7 Jiewen Tan 2017-07-12 15:56:31 PDT
Created attachment 315294 [details]
Patch
Comment 8 Brady Eidson 2017-07-13 09:14:09 PDT
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 9 Jiewen Tan 2017-07-14 09:10:50 PDT
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.
Comment 10 Brady Eidson 2017-07-14 09:29:09 PDT
(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.
Comment 11 Brady Eidson 2017-07-14 09:29:46 PDT
But you also need the Cocoa API because:

> You need to use the Cocoa API and specify an IndexedDB directory location in the WKWebsiteDataStoreConfiguration.
Comment 12 Jiewen Tan 2017-07-14 09:36:35 PDT
(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.
Comment 13 Jiewen Tan 2017-07-26 21:13:42 PDT
Created attachment 316516 [details]
Patch
Comment 14 Jiewen Tan 2017-07-28 11:45:18 PDT
Created attachment 316661 [details]
Patch
Comment 15 Darin Adler 2017-07-30 10:29:07 PDT
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 16 Jiewen Tan 2017-07-31 18:39:05 PDT
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.
Comment 17 Jiewen Tan 2017-07-31 18:42:34 PDT
Created attachment 316825 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2017-08-01 12:50:35 PDT
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
Comment 19 Jiewen Tan 2017-08-01 13:00:57 PDT
Committed r220109: <http://trac.webkit.org/changeset/220109>