Changeset 243420 in webkit


Ignore:
Timestamp:
Mar 23, 2019, 9:15:56 PM (6 years ago)
Author:
mark.lam@apple.com
Message:

Rolling out r243032 and r243071 because the fix is incorrect.
​https://bugs.webkit.org/show_bug.cgi?id=195892
<rdar://problem/48981239>

Not reviewed.

JSTests:

  • stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js: Removed.

Source/JavaScriptCore:

The fix is incorrect: it relies on being able to determine liveness of an object
in an ObjectPropertyCondition based on the state of the object's MarkedBit.
However, there's no guarantee that GC has run and that the MarkedBit is already
set even if the object is live. As a result, we may not re-install adaptive
watchpoints based on presumed dead objects which are actually live.

I'm rolling this out, and will implement a more comprehensive fix to handle
watchpoint liveness later.

  • bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:

(JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):

  • bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:

(JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):

  • bytecode/ObjectPropertyCondition.cpp:

(JSC::ObjectPropertyCondition::dumpInContext const):

  • bytecode/StructureStubClearingWatchpoint.cpp:

(JSC::StructureStubClearingWatchpoint::fireInternal):

  • dfg/DFGAdaptiveStructureWatchpoint.cpp:

(JSC::DFG::AdaptiveStructureWatchpoint::fireInternal):

  • runtime/StructureRareData.cpp:

(JSC::ObjectToStringAdaptiveStructureWatchpoint::fireInternal):

LayoutTests:

  • platform/mac/TestExpectations:
Location:
trunk
Files:
1 deleted
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r243391 r243420  
     12019-03-23  Mark Lam  <mark.lam@apple.com>
     2
     3        Rolling out r243032 and r243071 because the fix is incorrect.
     4        https://bugs.webkit.org/show_bug.cgi?id=195892
     5        <rdar://problem/48981239>
     6
     7        Not reviewed.
     8
     9        * stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js: Removed.
     10
    1112019-03-22  Mark Lam  <mark.lam@apple.com>
    212

  • trunk/LayoutTests/ChangeLog

    r243419 r243420  
     12019-03-23  Mark Lam  <mark.lam@apple.com>
     2
     3        Rolling out r243032 and r243071 because the fix is incorrect.
     4        https://bugs.webkit.org/show_bug.cgi?id=195892
     5        <rdar://problem/48981239>
     6
     7        Not reviewed.
     8
     9        * platform/mac/TestExpectations:
     10
    1112019-03-23  Justin Fan  <justin_fan@apple.com>
    212

  • trunk/LayoutTests/platform/mac/TestExpectations

    r243372 r243420  
    10701070webkit.org/b/153894 [ Debug ] inspector/model/color.html [ Pass Timeout ]
    10711071webkit.org/b/148636 inspector/model/parse-script-syntax-tree.html [ Pass Timeout ]
    1072 webkit.org/b/148636 inspector/model/remote-object.html [ Pass Timeout Failure ]
     1072webkit.org/b/148636 inspector/model/remote-object.html [ Pass Timeout ]
    10731073webkit.org/b/167265 inspector/network/client-blocked-load.html [ Pass Timeout ]
    10741074webkit.org/b/148636 inspector/page/main-frame-resource.html [ Pass Timeout ]

  • trunk/Source/JavaScriptCore/ChangeLog

    r243418 r243420  
     12019-03-23  Mark Lam  <mark.lam@apple.com>
     2
     3        Rolling out r243032 and r243071 because the fix is incorrect.
     4        https://bugs.webkit.org/show_bug.cgi?id=195892
     5        <rdar://problem/48981239>
     6
     7        Not reviewed.
     8
     9        The fix is incorrect: it relies on being able to determine liveness of an object
     10        in an ObjectPropertyCondition based on the state of the object's MarkedBit.
     11        However, there's no guarantee that GC has run and that the MarkedBit is already
     12        set even if the object is live.  As a result, we may not re-install adaptive
     13        watchpoints based on presumed dead objects which are actually live.
     14
     15        I'm rolling this out, and will implement a more comprehensive fix to handle
     16        watchpoint liveness later.
     17
     18        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
     19        (JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
     20        * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:
     21        (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):
     22        * bytecode/ObjectPropertyCondition.cpp:
     23        (JSC::ObjectPropertyCondition::dumpInContext const):
     24        * bytecode/StructureStubClearingWatchpoint.cpp:
     25        (JSC::StructureStubClearingWatchpoint::fireInternal):
     26        * dfg/DFGAdaptiveStructureWatchpoint.cpp:
     27        (JSC::DFG::AdaptiveStructureWatchpoint::fireInternal):
     28        * runtime/StructureRareData.cpp:
     29        (JSC::ObjectToStringAdaptiveStructureWatchpoint::fireInternal):
     30
    1312019-03-23  Keith Miller  <keith_miller@apple.com>
    232

  • trunk/Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp

    r243032 r243420  
    11/*
    2  * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    6363        return;
    6464
    65     // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
    66     // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
    67     if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
     65    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
    6866        install(vm);
    6967        return;

  • trunk/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp

    r243032 r243420  
    5050void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireDetail&)
    5151{
    52     // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
    53     // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
    54     if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
     52    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
    5553        install(vm);
    5654        return;

  • trunk/Source/JavaScriptCore/bytecode/ObjectPropertyCondition.cpp

    r243032 r243420  
    11/*
    2  * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2015 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    3838        return;
    3939    }
    40 
    41     // FIXME: The m_key.isStillLive() check should not be needed if the watchpoint using this
    42     // condition was removed when m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
    43     if (!isStillLive()) {
    44         out.print("<not live>");
    45         return;
    46     }
    47 
     40   
    4841    out.print("<", inContext(JSValue(m_object), context), ": ", inContext(m_condition, context), ">");
    4942}

  • trunk/Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp

    r243032 r243420  
    11/*
    2  * Copyright (C) 2012-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2012, 2015-2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    3737void StructureStubClearingWatchpoint::fireInternal(VM& vm, const FireDetail&)
    3838{
    39     // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
    40     // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
    41     if (!m_key.isStillLive() || !m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
     39    if (!m_key || !m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
    4240        // This will implicitly cause my own demise: stub reset removes all watchpoints.
    4341        // That works, because deleting a watchpoint removes it from the set's list, and

  • trunk/Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp

    r243032 r243420  
    11/*
    2  * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2015 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    5353void AdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireDetail& detail)
    5454{
    55     // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
    56     // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
    57     if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
     55    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
    5856        install(vm);
    5957        return;

  • trunk/Source/JavaScriptCore/runtime/StructureRareData.cpp

    r243032 r243420  
    11/*
    2  * Copyright (C) 2013-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    192192        return;
    193193
    194     // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
    195     // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
    196     if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
     194    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
    197195        install(vm);
    198196        return;
Note: See TracChangeset for help on using the changeset viewer.