Summary: | SerializedScriptValue should use a compact encoding for 8-bit strings. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||
Component: | WebCore JavaScript | Assignee: | Andreas Kling <kling> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | alecflett, buildbot, commit-queue, darin, ggaren, jsbell, kling, rniwa | ||||||||||||
Priority: | P2 | Keywords: | Performance | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Andreas Kling
2015-10-08 14:42:17 PDT
Created attachment 262715 [details]
Patch
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)) !!!! (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! Created attachment 262719 [details]
Patch without a planet-sized security hole
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. Well this broke fast/storage/serialized-script-value.html according to EWS so let's figure out what happened with that first. 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 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
(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 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 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 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. 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 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 on attachment 262788 [details] Patch II: Back to the Compatibility Clearing flags on attachment: 262788 Committed r190838: <http://trac.webkit.org/changeset/190838> All reviewed patches have been landed. Closing bug. |