Bug 96818

Summary: Add tests for explicit serialization values
Product: WebKit Reporter: Alec Flett <alecflett>
Component: New BugsAssignee: Alec Flett <alecflett>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bfulgham, dbates, dglazkov, dino, gtk-ews, gustavo, haraken, jsbell, philn, webkit-ews, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102302, 102324, 102337, 102342, 102466    
Bug Blocks: 102230    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch none

Description Alec Flett 2012-09-14 12:09:51 PDT
Add tests for explicit serialization values
Comment 1 Alec Flett 2012-09-14 12:12:56 PDT
Created attachment 164204 [details]
Patch
Comment 2 Alec Flett 2012-09-14 12:16:57 PDT
Note that a subset of the functionality of these tests already exist in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/IDBKeyPathTest.cpp - I am removing them in bug 95409 because of a refactoring - rather than write a bunch of v8-specific tests, I just moved the tests to JS.

Also note that I'm using Uint16Array in the JS. This may seem arbitrary and strange, but it is intentional, to be consistent with the actual encoding style in SerializedScriptValue, which packs bytes into UChars - this will make it easier to debug regressions.
Comment 3 Alec Flett 2012-09-14 12:18:15 PDT
haraken@ - this is the test split out from bug 95409, as discussed in e-mail a few days ago. Mind a quick review? (path for the other bug forthcoming...)
Comment 4 Build Bot 2012-09-14 12:26:17 PDT
Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13854538
Comment 5 Gyuyoung Kim 2012-09-14 12:34:33 PDT
Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13855548
Comment 6 Early Warning System Bot 2012-09-14 12:58:06 PDT
Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13841879
Comment 7 Early Warning System Bot 2012-09-14 12:59:36 PDT
Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13841880
Comment 8 WebKit Review Bot 2012-09-14 13:17:03 PDT
Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13859301

New failing tests:
storage/ssv.html
Comment 9 Build Bot 2012-09-14 13:39:54 PDT
Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13858333
Comment 10 Kentaro Hara 2012-09-14 14:01:29 PDT
Comment on attachment 164204 [details]
Patch

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

The approach looks good. Let's fix build failures. r-ing due to bot failures.

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please describe what your patch is doing.

> Source/WebCore/testing/Internals.cpp:1261
> +    return ArrayBuffer::create(static_cast<const void*>(stringValue.impl()->characters()),
> +                               stringValue.sizeInBytes());

Nit: We don't wrap lines.

> LayoutTests/ChangeLog:13
> +        * storage/resources/ssv.js: Added.

Let's rename ssv.js to serialized-script-value.js, for clarification.

> LayoutTests/storage/resources/ssv.js:1
> +

Nit: remove this line.

> LayoutTests/storage/resources/ssv.js:32
> +    // test deserialization

Nit: this comment is not needed (invalid?).

> LayoutTests/storage/resources/ssv.js:42
> +    // test serialization

Nit: this comment is not needed.

> LayoutTests/storage/resources/ssv.js:60
> +// we only test a few cases of the "old" serialization format because

It looks like no tests are passing values to 'oldFormat'. Is it intended?

> LayoutTests/storage/resources/ssv.js:98
> +testSerialization(undefined, [0x01ff, 0x003f, 0x005f]);
> +testSerialization(true, [0x01ff, 0x003f, 0x0054]);
> +testSerialization(false, [0x01ff, 0x003f, 0x0046]);
> +testSerialization([1,2,3,4,5,6,7,8], [0x01ff, 0x003f, 0x0841, 0x013f, 0x0249, 0x013f, 0x0449, 0x013f, 0x0649, 0x013f, 0x0849, 0x013f, 0x0a49, 0x013f, 0x0c49, 0x013f, 0x0e49, 0x013f, 0x1049, 0x0024, 0x0008]);
> +testSerialization(10, [0x01ff, 0x003f, 0x1449]);
> +testSerialization(-10, [0x01ff, 0x003f, 0x1349]);
> +testSerialization(Math.pow(2,30), [0x01ff, 0x003f, 0x8049, 0x8080, 0x0880]);
> +testSerialization(Math.pow(2,55), [0x01ff, 0x003f, 0x004e, 0x0000, 0x0000, 0x6000, 0x0043]);
> +testSerialization(null, [0x01ff, 0x003f, 0x0030]);
> +testSerialization(/abc/, [0x01ff, 0x003f, 0x0352, 0x6261, 0x0063]);

The coverage of added tests is not so good. Could you improve the coverage? e.g. "", "abc", 1.23, function(){}, etc...

> LayoutTests/storage/resources/ssv.js:112
> +finishJSTest();

Nit: this is not needed.

> LayoutTests/storage/ssv.html:8
> +<p id="description"></p>
> +<div id="console"></div>

Nit: These are not needed.

> LayoutTests/storage/ssv.html:9
> +<script src="resources/ssv.js"></script>

Nit: You can just directly write JavaScript here instead of including ssv.js.
Comment 11 Alec Flett 2012-11-14 10:22:05 PST
Created attachment 174195 [details]
Patch
Comment 12 Alec Flett 2012-11-14 10:23:09 PST
abarth@ - r?

We talked about this some months ago.. finally got around to fixing it using webkit_unit_tests

note that I had to add webkit_test_support to the gyp to allow me to use the internals-injector helper.
Comment 13 kov's GTK+ EWS bot 2012-11-14 10:38:30 PST
Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14834409
Comment 14 Early Warning System Bot 2012-11-14 10:39:29 PST
Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14826683
Comment 15 EFL EWS Bot 2012-11-14 10:56:01 PST
Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14826687
Comment 16 Build Bot 2012-11-14 11:01:39 PST
Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14832438
Comment 17 Adam Barth 2012-11-14 11:29:28 PST
Comment on attachment 174195 [details]
Patch

Woah, I don't think we want to link in the internals object into the unit tests.  If you want to use internals, you should write a LayoutTest (perhaps somewhere in the platform/chromium directory).  If you want to write a unit test, you should just call WebSerializedScriptValue::serialize and WebSerializedScriptValue::fromString directly from the unit test.  Note: You can use WebFrame::executeScriptAndReturnValue to create interesting JavaScript objects to serialize.
Comment 18 Alec Flett 2012-11-14 13:58:01 PST
Created attachment 174253 [details]
Patch
Comment 19 Alec Flett 2012-11-14 13:58:52 PST
I had no idea that there were *tests* in platform/chromium - that changes everything. New patch is much better (and fixes JSC build issues)
Comment 20 Adam Barth 2012-11-14 14:53:03 PST
Comment on attachment 174253 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.h:88
>      String toString();
> +    String toWireString() { return toString(); }

What's the difference between toString and toWireString?  Should we add a comment explaining the difference?

> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:8
> +    expectedV = expectedValues;

expectedV <--- "V" isn't a great part of a variable name.  we prefer complete words in variable names.
Comment 21 Build Bot 2012-11-14 15:25:47 PST
Comment on attachment 174253 [details]
Patch

Attachment 174253 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14837490
Comment 22 Alec Flett 2012-11-14 15:31:06 PST
Comment on attachment 174253 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.h:88
>> +    String toWireString() { return toString(); }
> 
> What's the difference between toString and toWireString?  Should we add a comment explaining the difference?

In theory, there seems to be some intention of a serialization format saved somewhere, vs just passed around in memory. In practice, there is none. filed bug 102291
Comment 23 Alec Flett 2012-11-14 15:45:21 PST
Created attachment 174272 [details]
Patch for landing
Comment 24 WebKit Review Bot 2012-11-14 16:35:11 PST
Comment on attachment 174272 [details]
Patch for landing

Clearing flags on attachment: 174272

Committed r134691: <http://trac.webkit.org/changeset/134691>
Comment 25 WebKit Review Bot 2012-11-14 16:35:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Alec Flett 2012-11-14 17:03:02 PST
fix in bug 102302
Comment 28 WebKit Review Bot 2012-11-14 22:46:09 PST
Re-opened since this is blocked by bug 102337
Comment 29 Daniel Bates 2012-11-14 22:58:02 PST
As of 10:49 pm PST, the Apple Windows Debug build and GTK builds are broken. For completeness, the following are hyperlinks to the stdio from the bots as of r134733.

Apple Windows Debug:
<http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/57958/steps/compile-webkit/logs/stdio>

GTK Linux 32-bit Release:
<http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/28586/steps/compile-webkit/logs/stdio>

GTK Linux 64-bit Debug:
<http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/39171/steps/compile-webkit/logs/stdio>

GTK Linux 64-bit Release:
<http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/31012/steps/compile-webkit/logs/stdio>
Comment 30 Zan Dobersek 2012-11-15 00:32:41 PST
Please don't proceed with landing a patch that will surely cause build breakage.

Here's the patch required to fix the build on the GTK port:

diff --git a/Source/autotools/symbols.filter b/Source/autotools/symbols.filter
index 94d46cc..a3263de 100644
--- a/Source/autotools/symbols.filter
+++ b/Source/autotools/symbols.filter
@@ -210,6 +210,13 @@ _ZTVN7WebCore28InspectorFrontendClientLocal8SettingsE;
 _ZNK7WebCore5Frame15layerTreeAsTextEj;
 _ZN7WebCore9FrameView17setTracksRepaintsEb;
 _ZNK7WebCore5Frame25trackedRepaintRectsAsTextEv;
+_ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPN3WTF11ArrayBufferE;
+_ZN7WebCore13toArrayBufferEN3JSC7JSValueE;
+_ZN7WebCore21SerializedScriptValue6createEPN3JSC9ExecStateENS1_7JSValueEPN3WTF6VectorINS5_6RefPtrINS_11MessagePortEEELm1EEEPNS6_INS7_INS5_11ArrayBufferEEELm1EEENS_22SerializationErrorModeE;
+_ZN7WebCore21SerializedScriptValue6createERKN3WTF6StringE;
+_ZN7WebCore21SerializedScriptValue8toStringEv;
+_ZN7WebCore21SerializedScriptValue11deserializeEPN3JSC9ExecStateEPNS1_14JSGlobalObjectEPN3WTF6VectorINS6_6RefPtrINS_11MessagePortEEELm1EEENS_22SerializationErrorModeE;
+_ZN7WebCore21SerializedScriptValueD1Ev;
 
 local:
 _Z*;
Comment 31 Daniel Bates 2012-11-15 00:33:38 PST
I decided to roll out this change in <http://trac.webkit.org/changeset/134747> (https://bugs.webkit.org/show_bug.cgi?id=102342) because the Apple Windows Debug build and GTK builds have been broken for over 5 hours. We should look to resolve the build failures offline and then re-land this change.

I made an attempt to fix the Apple Windows Debug and GTK builds in <https://bugs.webkit.org/show_bug.cgi?id=102337>. This attempt didn't fix the build breakage and broke the Apple Windows Release build.

For completeness, I reverted the following changesets (in order):

http://trac.webkit.org/changeset/134691 (this bug)
http://trac.webkit.org/changeset/134703 (Bug #102302)
http://trac.webkit.org/changeset/134715
http://trac.webkit.org/changeset/134716
http://trac.webkit.org/changeset/134733 (Bug #102324)
Comment 32 Alec Flett 2012-11-15 10:11:14 PST
Created attachment 174479 [details]
Patch
Comment 33 Alec Flett 2012-11-15 10:12:04 PST
Comment on attachment 174479 [details]
Patch

Running this through EWS again while I try to run as many local builds as I can.
Comment 34 Alec Flett 2012-11-15 16:11:04 PST
Created attachment 174540 [details]
Patch
Comment 35 Alec Flett 2012-11-15 16:49:39 PST
Comment on attachment 174540 [details]
Patch

ok, this patch contains all of the subsequent patches, and it builds & runs on gtk, and last night I had the windows bots green. 

Here goes nothin'
Comment 36 WebKit Review Bot 2012-11-15 17:08:01 PST
Comment on attachment 174540 [details]
Patch

Clearing flags on attachment: 174540

Committed r134865: <http://trac.webkit.org/changeset/134865>
Comment 37 WebKit Review Bot 2012-11-15 17:08:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Daniel Bates 2012-11-15 19:02:18 PST
(In reply to comment #36)
> (From update of attachment 174540 [details])
> Clearing flags on attachment: 174540
> 
> Committed r134865: <http://trac.webkit.org/changeset/134865>

The Apple Windows Debug build is failing after this change:
<http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/58018/steps/compile-webkit/logs/stdio>.
Comment 39 WebKit Review Bot 2012-11-15 23:47:52 PST
Re-opened since this is blocked by bug 102466
Comment 40 Daniel Bates 2012-11-16 00:18:43 PST
(In reply to comment #39)
> Re-opened since this is blocked by bug 102466

As aforementioned in comment 38, this change broke the Apple Windows Debug build. I'm unclear how to resolve this breakage (*) and attempts to contact Alec Frett (alecf) and Adam Barth (abarth) on IRC were unsuccessful. Therefore, I decided to roll out this change so that we can fix the build offline.

(*) I tried to fix identical build breakage yesterday. See comment 31 for more details.
Comment 41 Brent Fulgham 2012-11-16 16:54:08 PST
The 2012-11-15 16:11 PST patch applies cleanly and builds on my Windows system in Debug mode. I'm building in Release mode and will see if that works as well.
Comment 42 Alec Flett 2012-11-16 17:28:24 PST
Comment on attachment 174540 [details]
Patch

ok, so bfulgham tried the build himself, and didn't get an error, so we're thinking the windows buildbot may have just been flakey at the time. Retrying a landing.
Comment 43 WebKit Review Bot 2012-11-16 17:53:38 PST
Comment on attachment 174540 [details]
Patch

Clearing flags on attachment: 174540

Committed r135022: <http://trac.webkit.org/changeset/135022>
Comment 44 WebKit Review Bot 2012-11-16 17:53:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 45 Brent Fulgham 2012-11-16 18:04:01 PST
My release machine finished building successfully as well.  If we continue to see Apple build failures, we might need someone at Apple to force it to regenerate the various files.  These don't always get cleaned up properly from build-to-build if you don't manually clobber the directory.

There is a trick they used to do (touching one of the IDL files), but I don't remember the details.