Bug 149934 - SerializedScriptValue should use a compact encoding for 8-bit strings.
Summary: SerializedScriptValue should use a compact encoding for 8-bit strings.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2015-10-08 14:42 PDT by Andreas Kling
Modified: 2015-10-10 08:28 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.66 KB, patch)
2015-10-08 14:48 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch without a planet-sized security hole (6.14 KB, patch)
2015-10-08 15:00 PDT, Andreas Kling
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (761.46 KB, application/zip)
2015-10-08 15:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (629.40 KB, application/zip)
2015-10-08 15:33 PDT, Build Bot
no flags Details
Patch II: Back to the Compatibility (24.32 KB, patch)
2015-10-09 13:00 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-10-08 14:42:17 PDT
SerializedScriptValue encodes 8-bit strings as 16-bit strings with zeroed-out high bytes.
This is not ideal.
Comment 1 Andreas Kling 2015-10-08 14:48:54 PDT
Created attachment 262715 [details]
Patch
Comment 2 Darin Adler 2015-10-08 14:58:03 PDT
Comment on attachment 262715 [details]
Patch

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

> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:25
> -(deny default (with partial-symbolication))
> +(allow default (with partial-symbolication))

!!!!
Comment 3 Andreas Kling 2015-10-08 14:59:53 PDT
(In reply to comment #2)
> Comment on attachment 262715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262715&action=review
> 
> > Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:25
> > -(deny default (with partial-symbolication))
> > +(allow default (with partial-symbolication))
> 
> !!!!

Oh wow, that's a brush with danger right there!
Comment 4 Andreas Kling 2015-10-08 15:00:43 PDT
Created attachment 262719 [details]
Patch without a planet-sized security hole
Comment 5 Darin Adler 2015-10-08 15:07:00 PDT
Comment on attachment 262719 [details]
Patch without a planet-sized security hole

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:483
> +        writeLittleEndian<uint8_t>(out, s.is8Bit());

A whole byte. Why not use a String8Tag and String16Tag instead? I guess a byte is no big deal.

Or, why not stop before this when the length is zero?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1001
> +        writeLittleEndian<uint8_t>(m_buffer, str.is8Bit());
> +        if (!length)
> +            return;

Could move the zero length one line earlier if you took my suggestion above.
Comment 6 Andreas Kling 2015-10-08 15:13:26 PDT
Well this broke fast/storage/serialized-script-value.html according to EWS so let's figure out what happened with that first.
Comment 7 Build Bot 2015-10-08 15:21:37 PDT
Comment on attachment 262719 [details]
Patch without a planet-sized security hole

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

New failing tests:
fast/storage/serialized-script-value.html
Comment 8 Build Bot 2015-10-08 15:21:42 PDT
Created attachment 262721 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 9 Andreas Kling 2015-10-08 15:31:04 PDT
(In reply to comment #6)
> Well this broke fast/storage/serialized-script-value.html according to EWS
> so let's figure out what happened with that first.

Oh this test just documents the exact byte-for-byte encoding of these things and needs some rebaselining.
Comment 10 Build Bot 2015-10-08 15:33:30 PDT
Comment on attachment 262719 [details]
Patch without a planet-sized security hole

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

New failing tests:
fast/storage/serialized-script-value.html
Comment 11 Build Bot 2015-10-08 15:33:34 PDT
Created attachment 262722 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 12 Darin Adler 2015-10-08 15:37:43 PDT
Comment on attachment 262719 [details]
Patch without a planet-sized security hole

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1473
>          if (!readLittleEndian(ptr, end, length) || length >= StringPoolTag)
>              return String();

This length >= StringPoolTag check is redundant. The readString function below does a range check on length already. I don’t think we need a second one.
Comment 13 Andreas Kling 2015-10-09 13:00:21 PDT
Created attachment 262788 [details]
Patch II: Back to the Compatibility

To keep compatibility with old serializations, encode the 8bitness in the highest bit of the length u32 instead.
Since the length limit was always 0x7fffffff, this is pretty neat.
Comment 14 Antti Koivisto 2015-10-10 07:16:04 PDT
Comment on attachment 262788 [details]
Patch II: Back to the Compatibility

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

r=me

> Source/WebCore/ChangeLog:15
> +        This patch knocks ~1 MB off of theverge.com, where some ad or tracker or whatever likes to
> +        do a ton of postMessage() business.

:/
Comment 15 WebKit Commit Bot 2015-10-10 08:28:19 PDT
Comment on attachment 262788 [details]
Patch II: Back to the Compatibility

Clearing flags on attachment: 262788

Committed r190838: <http://trac.webkit.org/changeset/190838>
Comment 16 WebKit Commit Bot 2015-10-10 08:28:26 PDT
All reviewed patches have been landed.  Closing bug.