[webkit-changes] [WebKit/WebKit] 0ebf83: Cherry-pick 267815.623 at safari-7617-branch (430d474...

Dan Robson noreply at github.com
Tue Jan 23 13:02:39 PST 2024


  Branch: refs/heads/webkitglib/2.42
  Home:   https://github.com/WebKit/WebKit
  Commit: 0ebf8393b3e433292c2c281576b7578eaf64e671
      https://github.com/WebKit/WebKit/commit/0ebf8393b3e433292c2c281576b7578eaf64e671
  Author: Mark Lam <mark.lam at apple.com>
  Date:   2024-01-23 (Tue, 23 Jan 2024)

  Changed paths:
    A LayoutTests/js/structuredClone/structured-clone-validation-with-big-int-expected.txt
    A LayoutTests/js/structuredClone/structured-clone-validation-with-big-int.html
    M Source/JavaScriptCore/runtime/OptionsList.h
    M Source/WebCore/bindings/js/SerializedScriptValue.cpp

  Log Message:
  -----------
  Cherry-pick 267815.623 at safari-7617-branch (430d474531c0). https://bugs.webkit.org/show_bug.cgi?id=265975

    CloneSerializer/Deserializer's objectPool should match.
    https://bugs.webkit.org/show_bug.cgi?id=265975
    rdar://118868470

    Reviewed by Chris Dumez and Sihui Liu

    CloneSerializer and CloneDeserializer uses m_gcBuffer for multiple purposes:
    1. As an object pool that ObjectReferenceTag refers back to i.e. the order of its
       entries need to be consistent between CloneSerializer and CloneDeserializer.
    2. As a keep alive buffer to protect some objects use in the serialization effort.

    Purpose (2) conflicts with purpose (1), which can lead to bugs.  This patch disambiguates
    between these 2 purposes by introducing m_objectPool for purpose (1), and m_keepAliveBuffer
    for purpose (2).

    Changes made:
    1. Renamed m_objectPool to m_objectPoolMap.
    2. Renamed m_gcBuffer to m_objectPool: for tracking the list of objects that ObjectReferenceTag
       can refer to.
    3. Added m_keepAliveBuffer to CloneSerializer: for keeping miscellaneous objects alive from the GC.

    4. Renamed some method names for clarity:
           CloneSerializer::checkForDuplicate --> CloneSerializer::writeObjectReferenceIfDupe
           CloneSerializer::recordObject --> CloneSerializer::addToObjectPool
           CloneSerializer::startObjectInternal --> CloneSerializer::addToObjectPoolIfNotDupe

    5. Used CloneSerializer::addToObjectPoolIfNotDupe instead of the following:
           CloneSerializer::startObject
           CloneSerializer::startArray
           CloneSerializer::startSet
           CloneSerializer::startMap

       The clients of addToObjectPoolIfNotDupe now indicate what object types (indicated by their
       SerializationTags) they are adding.  This makes it easier to compare the serialization and
       deserialization code and make sure that they are equivalent.

       This enables us to audit the type of object being added and provide a sanity check that
       it's also added on the deserializer side.

    6. Introduced CloneDeserializer::addToObjectPool() so that we can tag which object type (as
       indicated by its SerializationTag) we're adding to the m_objectPool (instead of calling
       appendWithCrashOnOverflow() on it directly to add objects).

       This enables us to audit the type of object being added and provide a sanity check that
       it's also added on the serializer side.

    7. Removed 3 calls to m_gcBuffer.appendWithCrashOnOverflow in the BigIntTag case in
       CloneDeserializer::readBigInt().  This was a bug.

    8. Removed the following calls to m_gcBuffer.appendWithCrashOnOverflow in CloneSerializer::serialize:
       a. redundant adding of the JSMap object.  It was already added by startMap(), now addToObjectPoolIfNotDupe().
       b. keep alive of the JSMapIterator object.  It does not need to be in m_objectPool.
       c. keep alive of a map entry value.
       d. redundant adding of the JSSet object.  It was already added by startSet(), now addToObjectPoolIfNotDupe().
       e. keep alive of the JSSetIterator object.  It does not need to be in m_objectPool.

       These were bugs.

    9. Renamed the mapObjectStartState and setObjectStartState labels in the deserializer to match the
       mapStartState and setStartState labels in the serializer.  This makes it easier to check the
       equivalency of the operations in the two.

    10. Added a validator (see validateSerializedResult()) in the serializer.

       The validator works by running a deserialization pass on the output of the serializer.
       After that, it compares the m_objectPoolTags of the 2 passes, and their entries should
       match.  This ensures that the serializer and deserializer will catch any bugs in the
       serialization / deserialization order of objects.

       a. The validator is only enabled on Debug builds (not built in on Release builds).
       b. The validator is only run when JSC::Options::validateSerializedValue() is true.
       c. The validator is only run when the object graph to be serialized and deserialized
          does not contain any complicated / complex objects.  "complex" here means that
          serialization of such objects cannot be validated this way.
       d. The validator is only run when both serialization and deserialization passes succeeds.

       With this validator, we can now fuzz the serializer / deserializer by creating HTML tests cases
       where we build an object graph and call structureClone() on it.  The HTML test will need to have
       the following comment on its 1st line:

           <!-- webkit-test-runner [ jscOptions=--validateSerializedValue=true ] -->

       This will enable the validator when structured cloning is done on that object graph.

    * LayoutTests/js/structuredClone/structured-clone-validation-with-big-int-expected.txt: Added.
    * LayoutTests/js/structuredClone/structured-clone-validation-with-big-int.html: Added.
    * Source/JavaScriptCore/runtime/OptionsList.h:
    * Source/WebCore/bindings/js/SerializedScriptValue.cpp:
    (WebCore::name):
    (WTF::printInternal):
    (WebCore::canBeAddedToObjectPool):
    (WebCore::CloneBase::objectPoolTags const):
    (WebCore::CloneBase::appendObjectPoolTag):
    (WebCore::CloneSerializer::serialize):
    (WebCore::CloneSerializer::sawComplexCase):
    (WebCore::CloneSerializer::didSeeComplexCases const):
    (WebCore::CloneSerializer::fillTransferMap):
    (WebCore::CloneSerializer::writeObjectReferenceIfDupe):
    (WebCore::CloneSerializer::addToObjectPool):
    (WebCore::CloneSerializer::addToObjectPoolIfNotDupe):
    (WebCore::CloneSerializer::dumpStringObject):
    (WebCore::CloneSerializer::dumpArrayBufferView):
    (WebCore::CloneSerializer::dumpIfTerminal):
    (WebCore::CloneSerializer::writeObjectIndex):
    (WebCore::CloneDeserializer::addToObjectPool):
    (WebCore::CloneDeserializer::readBigInt):
    (WebCore::CloneDeserializer::readTerminal):
    (WebCore::CloneDeserializer::deserialize):
    (WebCore::validateSerializedResult):
    (WebCore::CloneSerializer::checkForDuplicate): Deleted.
    (WebCore::CloneSerializer::recordObject): Deleted.
    (WebCore::CloneSerializer::startObjectInternal): Deleted.
    (WebCore::CloneSerializer::startObject): Deleted.
    (WebCore::CloneSerializer::startArray): Deleted.
    (WebCore::CloneSerializer::startSet): Deleted.
    (WebCore::CloneSerializer::startMap): Deleted.

    Canonical link: https://commits.webkit.org/267815.623@safari-7617-branch


  Commit: 946a67cd427741cf62990a345d579b542782acfb
      https://github.com/WebKit/WebKit/commit/946a67cd427741cf62990a345d579b542782acfb
  Author: Sihui Liu <sihui_liu at apple.com>
  Date:   2024-01-23 (Tue, 23 Jan 2024)

  Changed paths:
    M Source/WebCore/bindings/js/SerializedScriptValue.cpp

  Log Message:
  -----------
  Cherry-pick 267815.625 at safari-7617-branch (aa738c8a36f7). https://bugs.webkit.org/show_bug.cgi?id=266111

    SEED ☂: Video - Playback does not start - [Includes Logs] - Netflix.com
    https://bugs.webkit.org/show_bug.cgi?id=266111
    rdar://118775332

    Reviewed by Chris Dumez and Mark Lam.

    rdar://116034413 changed the serialization format of script value but didn’t update version number. This was later fixed
    by rdar://117020274, which upgraded version number to 15 and made sure data serialized in new format contains new
    version nubmer. The problem is, builds between rdar://116034413 and rdar://117020274 could create serialized script data
    in new format but with old version number 14. These data can be stored persistently in IndexedDB database, and could not
    be deserialized successfully after rdar://117020274 because they have mismatched version number and format.

    To fix this, when we see an error in deserializing data with version nubmer 14, we upgrade the version number and try
    deserializing it again in new format. This patch tries deserialization again with a new deserializer instead of using
    the old deserializer because it's more complicated to revert the internal states on a deserializer that fails.

    * Source/WebCore/bindings/js/SerializedScriptValue.cpp:
    (WebCore::CloneDeserializer::deserialize):
    (WebCore::CloneDeserializer::takeBackingStores):
    (WebCore::CloneDeserializer::takeDetachedOffscreenCanvases):
    (WebCore::CloneDeserializer::takeDetachedRTCDataChannels):
    (WebCore::CloneDeserializer::takeSerializedVideoChunks):
    (WebCore::CloneDeserializer::takeSerializedVideoFrames):
    (WebCore::CloneDeserializer::takeSerializedAudioChunks):
    (WebCore::CloneDeserializer::takeSerializedAudioData):
    (WebCore::CloneDeserializer::version const):
    (WebCore::CloneDeserializer::upgradeVersion):

    Canonical link: https://commits.webkit.org/267815.625@safari-7617-branch


  Commit: 0e5a485821858b7acb5c7be586c177b15ae14a91
      https://github.com/WebKit/WebKit/commit/0e5a485821858b7acb5c7be586c177b15ae14a91
  Author: Dan Robson <dan_robson at apple.com>
  Date:   2024-01-23 (Tue, 23 Jan 2024)

  Changed paths:
    M Source/JavaScriptCore/dfg/DFGGraph.cpp

  Log Message:
  -----------
  [JSC] DFG constant property load should check the validity at the main thread

This webkitglib/2.42 backport commit squashes changes from the following
three Safari commits, which add and then remove a considerable amount of
code. It's much simpler if we take the changes all together as one.

Cherry-pick 267815.671 at safari-7617.2.4.11-branch (61d47f64edd9). https://bugs.webkit.org/show_bug.cgi?id=267134

    Apply patch. rdar://120560604

    	[JSC] DFG constant property load should check the validity at the main thread
    	https://bugs.webkit.org/show_bug.cgi?id=267134
    	rdar://120443399

    	Reviewed by Mark Lam.

    	Consider the following case,

    	    CheckStructure O, S1 | S3
    	    GetByOffset O, offset

    	And S1 -> S2 -> S3 structure transition happens.
    	By changing object concurrently with the compiler, it is possible that we will constant fold the property with O + S2.
    	While we insert watchpoints into S1 and S3, we cannot notice the change of the property in S2.
    	If we change O to S3 before running code, CheckStructure passes and we can use a value loaded from O + S2.

    	1. If S1 and S3 transitions are both already watched by DFG / FTL, then we do not need to care about the issue.
    	   CheckStructure ensures that O is S1 or S3. And both has watchpoints which fires when transition happens.
    	   So, if we are transitioning from S1 to S2 while compiling, it already invalidates the code.
    	2. If there is only one Structure (S1), then we can keep the current optimization by checking this condition at the main thread.
    	   CheckStructure ensures that O is S1. And this means that if the assumption is met at the main thread, then we can continue
    	   using this code safely. To check this condition, we added DesiredObjectProperties, which records JSObject*, offset, value, and structure.
    	   And at the end of compilation, in the main thread, we check this assumption is still met.

    	* Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
    	* Source/JavaScriptCore/Sources.txt:
    	* Source/JavaScriptCore/dfg/DFGDesiredObjectProperties.cpp: Added.
    	(JSC::DFG::DesiredObjectProperties::addLazily):
    	(JSC::DFG::DesiredObjectProperties::areStillValidOnMainThread):
    	* Source/JavaScriptCore/dfg/DFGDesiredObjectProperties.h: Added.
    	* Source/JavaScriptCore/dfg/DFGGraph.cpp:
    	(JSC::DFG::Graph::tryGetConstantProperty):
    	* Source/JavaScriptCore/dfg/DFGPlan.cpp:
    	(JSC::DFG::Plan::cancel):
    	(JSC::DFG::Plan::isStillValidOnMainThread):
    	* Source/JavaScriptCore/dfg/DFGPlan.h:

    	Canonical link: https://commits.webkit.org/272448.7@safari-7618-branch

Cherry-pick 31601205b6f3. https://bugs.webkit.org/show_bug.cgi?id=267134

    [JSC] DFG constant property load should check the validity at the main thread
    https://bugs.webkit.org/show_bug.cgi?id=267134
    rdar://120443399

    Reviewed by Mark Lam.

    Consider the following case,

        CheckStructure O, S1 | S3
        GetByOffset O, offset

    And S1 -> S2 -> S3 structure transition happens.
    By changing object concurrently with the compiler, it is possible that we will constant fold the property with O + S2.
    While we insert watchpoints into S1 and S3, we cannot notice the change of the property in S2.
    If we change O to S3 before running code, CheckStructure passes and we can use a value loaded from O + S2.

    1. If S1 and S3 transitions are both already watched by DFG / FTL, then we do not need to care about the issue.
       CheckStructure ensures that O is S1 or S3. And both has watchpoints which fires when transition happens.
       So, if we are transitioning from S1 to S2 while compiling, it already invalidates the code.
    2. If there is only one Structure (S1), then we can keep the current optimization by checking this condition at the main thread.
       CheckStructure ensures that O is S1. And this means that if the assumption is met at the main thread, then we can continue
       using this code safely. To check this condition, we added DesiredObjectProperties, which records JSObject*, offset, value, and structure.
       And at the end of compilation, in the main thread, we check this assumption is still met.

    * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
    * Source/JavaScriptCore/Sources.txt:
    * Source/JavaScriptCore/dfg/DFGDesiredObjectProperties.cpp: Added.
    (JSC::DFG::DesiredObjectProperties::addLazily):
    (JSC::DFG::DesiredObjectProperties::areStillValidOnMainThread):
    * Source/JavaScriptCore/dfg/DFGDesiredObjectProperties.h: Added.
    * Source/JavaScriptCore/dfg/DFGGraph.cpp:
    (JSC::DFG::Graph::tryGetConstantProperty):
    * Source/JavaScriptCore/dfg/DFGPlan.cpp:
    (JSC::DFG::Plan::cancel):
    (JSC::DFG::Plan::isStillValidOnMainThread):
    * Source/JavaScriptCore/dfg/DFGPlan.h:

    Canonical link: https://commits.webkit.org/272448.7@safari-7618-branch

    Canonical link: https://commits.webkit.org/267815.672@safari-7617.2.4.11-branch

Cherry-pick a8b53bc4d7b3. https://bugs.webkit.org/show_bug.cgi?id=267134

    [JSC] Remove DFGDesiredObjectProperties
    https://bugs.webkit.org/show_bug.cgi?id=267134
    rdar://120443399

    Reviewed by Mark Lam.

    When we limit the structure only one, there is no way to change the property without firing
    property replacement watchpoint while keeping object's structure as specified. So removing DFGDesiredObjectProperties.

    * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
    * Source/JavaScriptCore/Sources.txt:
    * Source/JavaScriptCore/dfg/DFGDesiredObjectProperties.cpp: Removed.
    * Source/JavaScriptCore/dfg/DFGDesiredObjectProperties.h: Removed.
    * Source/JavaScriptCore/dfg/DFGGraph.cpp:
    (JSC::DFG::Graph::tryGetConstantProperty):
    * Source/JavaScriptCore/dfg/DFGPlan.cpp:
    (JSC::DFG::Plan::cancel):
    (JSC::DFG::Plan::isStillValidOnMainThread):
    * Source/JavaScriptCore/dfg/DFGPlan.h:

    Canonical link: https://commits.webkit.org/272448.8@safari-7618-branch

    Canonical link: https://commits.webkit.org/267815.673@safari-7617.2.4.11-branch


Compare: https://github.com/WebKit/WebKit/compare/cf330016d67a...0e5a48582185


More information about the webkit-changes mailing list