Bug 96818 – Add tests for explicit serialization values

• Component: New Bugs
• Status: RESOLVED FIXED

[Alec Flett]

Add tests for explicit serialization values

[Alec Flett]

Created attachment 164204 [details]
Patch

[Alec Flett]

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.

[Alec Flett]

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

[Build Bot]

Comment on attachment 164204 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 164204 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 164204 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 164204 [details]
Patch

Attachment 164204 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13841880

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 164204 [details]
Patch

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

[Kentaro Hara]

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.

[Alec Flett]

Created attachment 174195 [details]
Patch

[Alec Flett]

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.

[kov's GTK+ EWS bot]

Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14834409

[Early Warning System Bot]

Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14826683

[EFL EWS Bot]

Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14826687

[Build Bot]

Comment on attachment 174195 [details]
Patch

Attachment 174195 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14832438

[Adam Barth]

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.

[Alec Flett]

Created attachment 174253 [details]
Patch

[Alec Flett]

I had no idea that there were *tests* in platform/chromium - that changes everything. New patch is much better (and fixes JSC build issues)

[Adam Barth]

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.

[Build Bot]

Comment on attachment 174253 [details]
Patch

Attachment 174253 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14837490

[Alec Flett]

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

[Alec Flett]

Created attachment 174272 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174272 [details]
Patch for landing

Clearing flags on attachment: 174272

Committed r134691: <http://trac.webkit.org/changeset/134691>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

This has broken the Apple Windows build.
http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/39479/steps/compile-webkit/logs/stdio

[Alec Flett]

fix in bug 102302

[WebKit Review Bot]

Re-opened since this is blocked by bug 102337

[Daniel Bates]

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>

[Zan Dobersek]

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*;

[Daniel Bates]

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)

[Alec Flett]

Created attachment 174479 [details]
Patch

[Alec Flett]

Comment on attachment 174479 [details]
Patch

Running this through EWS again while I try to run as many local builds as I can.

[Alec Flett]

Created attachment 174540 [details]
Patch

[Alec Flett]

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'

[WebKit Review Bot]

Comment on attachment 174540 [details]
Patch

Clearing flags on attachment: 174540

Committed r134865: <http://trac.webkit.org/changeset/134865>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

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

[WebKit Review Bot]

Re-opened since this is blocked by bug 102466

[Daniel Bates]

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

[Brent Fulgham]

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.

[Alec Flett]

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.

[WebKit Review Bot]

Comment on attachment 174540 [details]
Patch

Clearing flags on attachment: 174540

Committed r135022: <http://trac.webkit.org/changeset/135022>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

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.