Changeset 217429 in webkit


Ignore:
Timestamp:
May 25, 2017, 10:03:13 AM (7 years ago)
Author:
mark.lam@apple.com
Message:

ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself nor handleFire if it's dying shortly.
​https://bugs.webkit.org/show_bug.cgi?id=172548
<rdar://problem/31458393>

Reviewed by Filip Pizlo.

JSTests:

  • stress/regress-172548.patch: Added.

Source/JavaScriptCore:

Consider the following scenario:

  1. A ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1, watches for structure transitions, e.g. structure S2 transitioning to structure S3. In this case, O1 would be installed in S2's watchpoint set.
  2. When the structure transition happens, structure S2 will fire watchpoint O1.
  3. O1's handler will normally re-install itself in the watchpoint set of the new "transitioned to" structure S3.
  4. "Installation" here requires writing into the StructureRareData SD3 of the new structure S3. If SD3 does not exist yet, the installation process will trigger the allocation of StructureRareData SD3.
  5. It is possible that the Structure S1, and StructureRareData SD1 that owns the ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1 is no longer reachable by the GC, and therefore will be collected soon.
  6. The allocation of SD3 in (4) may trigger the sweeping of the StructureRareData SD1. This, in turn, triggers the deletion of the ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1.

After O1 is deleted in (6) and SD3 is allocated in (4), execution continues in
AdaptiveInferredPropertyValueWatchpointBase::fire() where O1 gets installed in
structure S3's watchpoint set. This is obviously incorrect because O1 is already
deleted. The result is that badness happens later when S3's watchpoint set fires
its watchpoints and accesses the deleted O1.

The fix is to enhance AdaptiveInferredPropertyValueWatchpointBase::fire() to
check if "this" is still valid before proceeding to re-install itself or to
invoke its handleFire() method.

ObjectToStringAdaptiveInferredPropertyValueWatchpoint (which extends
AdaptiveInferredPropertyValueWatchpointBase) will override its isValid() method,
and return false its owner StructureRareData is no longer reachable by the GC.
This ensures that it won't be deleted while it's installed to any watchpoint set.

Additional considerations and notes:

  1. In the above, I talked about the ObjectToStringAdaptiveInferredPropertyValueWatchpoint being installed in watchpoint sets. What actually happens is that ObjectToStringAdaptiveInferredPropertyValueWatchpoint has 2 members (m_structureWatchpoint and m_propertyWatchpoint) which may be installed in watchpoint sets. The ObjectToStringAdaptiveInferredPropertyValueWatchpoint is not itself a Watchpoint object.

But for brevity, in the above, I refer to the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
instead of its Watchpoint members. The description of the issue is still
accurate given the life-cycle of the Watchpoint members are embedded in the
enclosing ObjectToStringAdaptiveInferredPropertyValueWatchpoint object, and
hence, they share the same life-cycle.

  1. The top of AdaptiveInferredPropertyValueWatchpointBase::fire() removes its m_structureWatchpoint and m_propertyWatchpoint if they have been added to any watchpoint sets. This is safe to do even if the owner StructureRareData is no longer reachable by the GC.

This is because the only way we can get to AdaptiveInferredPropertyValueWatchpointBase::fire()
is if its Watchpoint members are still installed in some watchpoint set that
fired. This means that the AdaptiveInferredPropertyValueWatchpointBase
instance has not been deleted yet, because its destructor will automatically
remove the Watchpoint members from any watchpoint sets.

  • bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:

(JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
(JSC::AdaptiveInferredPropertyValueWatchpointBase::isValid):

  • bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:
  • heap/FreeList.cpp:

(JSC::FreeList::contains):

  • heap/FreeList.h:
  • heap/HeapCell.h:
  • heap/HeapCellInlines.h:

(JSC::HeapCell::isLive):

  • heap/MarkedAllocator.h:

(JSC::MarkedAllocator::isFreeListedCell):

  • heap/MarkedBlock.h:
  • heap/MarkedBlockInlines.h:

(JSC::MarkedBlock::Handle::isFreeListedCell):

  • runtime/StructureRareData.cpp:

(JSC::ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid):

Location:
trunk
Files:
1 added
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r217314 r217429  
     12017-05-25  Mark Lam  <mark.lam@apple.com>
     2
     3        ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself nor handleFire if it's dying shortly.
     4        https://bugs.webkit.org/show_bug.cgi?id=172548
     5        <rdar://problem/31458393>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        * stress/regress-172548.patch: Added.
     10
    1112017-05-23  Saam Barati  <sbarati@apple.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r217314 r217429  
     12017-05-25  Mark Lam  <mark.lam@apple.com>
     2
     3        ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself nor handleFire if it's dying shortly.
     4        https://bugs.webkit.org/show_bug.cgi?id=172548
     5        <rdar://problem/31458393>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        Consider the following scenario:
     10
     11        1. A ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1, watches for
     12           structure transitions, e.g. structure S2 transitioning to structure S3.
     13           In this case, O1 would be installed in S2's watchpoint set.
     14        2. When the structure transition happens, structure S2 will fire watchpoint O1.
     15        3. O1's handler will normally re-install itself in the watchpoint set of the new
     16           "transitioned to" structure S3.
     17        4. "Installation" here requires writing into the StructureRareData SD3 of the new
     18           structure S3.  If SD3 does not exist yet, the installation process will trigger
     19           the allocation of StructureRareData SD3.
     20        5. It is possible that the Structure S1, and StructureRareData SD1 that owns the
     21           ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1 is no longer reachable
     22           by the GC, and therefore will be collected soon.
     23        6. The allocation of SD3 in (4) may trigger the sweeping of the StructureRareData
     24           SD1.  This, in turn, triggers the deletion of the
     25           ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1.
     26
     27        After O1 is deleted in (6) and SD3 is allocated in (4), execution continues in
     28        AdaptiveInferredPropertyValueWatchpointBase::fire() where O1 gets installed in
     29        structure S3's watchpoint set.  This is obviously incorrect because O1 is already
     30        deleted.  The result is that badness happens later when S3's watchpoint set fires
     31        its watchpoints and accesses the deleted O1.
     32
     33        The fix is to enhance AdaptiveInferredPropertyValueWatchpointBase::fire() to
     34        check if "this" is still valid before proceeding to re-install itself or to
     35        invoke its handleFire() method.
     36
     37        ObjectToStringAdaptiveInferredPropertyValueWatchpoint (which extends
     38        AdaptiveInferredPropertyValueWatchpointBase) will override its isValid() method,
     39        and return false its owner StructureRareData is no longer reachable by the GC.
     40        This ensures that it won't be deleted while it's installed to any watchpoint set.
     41
     42        Additional considerations and notes:
     43        1. In the above, I talked about the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
     44           being installed in watchpoint sets.  What actually happens is that
     45           ObjectToStringAdaptiveInferredPropertyValueWatchpoint has 2 members
     46           (m_structureWatchpoint and m_propertyWatchpoint) which may be installed in
     47           watchpoint sets.  The ObjectToStringAdaptiveInferredPropertyValueWatchpoint is
     48           not itself a Watchpoint object.
     49
     50           But for brevity, in the above, I refer to the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
     51           instead of its Watchpoint members.  The description of the issue is still
     52           accurate given the life-cycle of the Watchpoint members are embedded in the
     53           enclosing ObjectToStringAdaptiveInferredPropertyValueWatchpoint object, and
     54           hence, they share the same life-cycle.
     55
     56        2. The top of AdaptiveInferredPropertyValueWatchpointBase::fire() removes its
     57           m_structureWatchpoint and m_propertyWatchpoint if they have been added to any
     58           watchpoint sets.  This is safe to do even if the owner StructureRareData is no
     59           longer reachable by the GC.
     60
     61           This is because the only way we can get to AdaptiveInferredPropertyValueWatchpointBase::fire()
     62           is if its Watchpoint members are still installed in some watchpoint set that
     63           fired.  This means that the AdaptiveInferredPropertyValueWatchpointBase
     64           instance has not been deleted yet, because its destructor will automatically
     65           remove the Watchpoint members from any watchpoint sets.
     66
     67        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
     68        (JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
     69        (JSC::AdaptiveInferredPropertyValueWatchpointBase::isValid):
     70        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:
     71        * heap/FreeList.cpp:
     72        (JSC::FreeList::contains):
     73        * heap/FreeList.h:
     74        * heap/HeapCell.h:
     75        * heap/HeapCellInlines.h:
     76        (JSC::HeapCell::isLive):
     77        * heap/MarkedAllocator.h:
     78        (JSC::MarkedAllocator::isFreeListedCell):
     79        * heap/MarkedBlock.h:
     80        * heap/MarkedBlockInlines.h:
     81        (JSC::MarkedBlock::Handle::isFreeListedCell):
     82        * runtime/StructureRareData.cpp:
     83        (JSC::ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid):
     84
    1852017-05-23  Saam Barati  <sbarati@apple.com>
    286

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

    r205462 r217429  
    11/*
    2  * Copyright (C) 2015 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
    … …  
    5858        m_propertyWatchpoint.remove();
    5959
     60    if (!isValid())
     61        return;
     62
    6063    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
    6164        install();
    … …  
    6467
    6568    handleFire(detail);
     69}
     70
     71bool AdaptiveInferredPropertyValueWatchpointBase::isValid() const
     72{
     73    return true;
    6674}
    6775

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

    r206525 r217429  
    11/*
    2  * Copyright (C) 2015 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
    … …  
    4747
    4848protected:
     49    virtual bool isValid() const;
    4950    virtual void handleFire(const FireDetail&) = 0;
    5051

  • trunk/Source/JavaScriptCore/heap/FreeList.cpp

    r205462 r217429  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2929namespace JSC {
    3030
     31bool FreeList::contains(const void* target) const
     32{
     33    if (remaining) {
     34        const void* start = (payloadEnd - remaining);
     35        const void* end = payloadEnd;
     36        return (start <= target) && (target < end);
     37    }
     38
     39    FreeCell* candidate = head;
     40    while (candidate) {
     41        if (candidate == target)
     42            return true;
     43        candidate = candidate->next;
     44    }
     45
     46    return false;
     47}
     48
    3149void FreeList::dump(PrintStream& out) const
    3250{

  • trunk/Source/JavaScriptCore/heap/FreeList.h

    r205462 r217429  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    8181        return *this != FreeList();
    8282    }
    83    
     83
     84    bool contains(const void* target) const;
     85
    8486    bool allocationWillFail() const { return !head && !remaining; }
    8587    bool allocationWillSucceed() const { return !allocationWillFail(); }

  • trunk/Source/JavaScriptCore/heap/HeapCell.h

    r213773 r217429  
    4848    void zap() { *reinterpret_cast_ptr<uintptr_t**>(this) = 0; }
    4949    bool isZapped() const { return !*reinterpret_cast_ptr<uintptr_t* const*>(this); }
    50    
     50
     51    bool isLive();
     52
    5153    bool isLargeAllocation() const;
    5254    CellContainer cellContainer() const;

  • trunk/Source/JavaScriptCore/heap/HeapCellInlines.h

    r205666 r217429  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2929#include "HeapCell.h"
    3030#include "LargeAllocation.h"
    31 #include "MarkedBlock.h"
     31#include "MarkedBlockInlines.h"
    3232#include "VM.h"
    3333
    3434namespace JSC {
     35
     36ALWAYS_INLINE bool HeapCell::isLive()
     37{
     38    if (isLargeAllocation())
     39        return largeAllocation().isLive();
     40    auto& markedBlockHandle = markedBlock().handle();
     41    if (markedBlockHandle.isFreeListed())
     42        return !markedBlockHandle.isFreeListedCell(this);
     43    return markedBlockHandle.isLive(this);
     44}
    3545
    3646ALWAYS_INLINE bool HeapCell::isLargeAllocation() const

  • trunk/Source/JavaScriptCore/heap/MarkedAllocator.h

    r213883 r217429  
    157157    Heap* heap() { return m_heap; }
    158158
     159    bool isFreeListedCell(const void* target) const { return m_freeList.contains(target); }
     160
    159161    template<typename Functor> void forEachBlock(const Functor&);
    160162    template<typename Functor> void forEachNotEmptyBlock(const Functor&);

  • trunk/Source/JavaScriptCore/heap/MarkedBlock.h

    r215637 r217429  
    167167        bool isLiveCell(const void*);
    168168
     169        bool isFreeListedCell(const void* target) const;
     170
    169171        bool isNewlyAllocated(const void*);
    170172        void setNewlyAllocated(const void*);

  • trunk/Source/JavaScriptCore/heap/MarkedBlockInlines.h

    r210844 r217429  
    116116        return false;
    117117    return isLive(markingVersion, isMarking, static_cast<const HeapCell*>(p));
     118}
     119
     120inline bool MarkedBlock::Handle::isFreeListedCell(const void* target) const
     121{
     122    ASSERT(isFreeListed());
     123    return m_allocator->isFreeListedCell(target);
    118124}
    119125

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

    r217108 r217429  
    11/*
    2  * Copyright (C) 2013 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
    … …  
    9191
    9292private:
     93    bool isValid() const override;
    9394    void handleFire(const FireDetail&) override;
    9495
    … …  
    213214}
    214215
     216bool ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid() const
     217{
     218    return m_structureRareData->isLive();
     219}
     220
    215221void ObjectToStringAdaptiveInferredPropertyValueWatchpoint::handleFire(const FireDetail& detail)
    216222{
Note: See TracChangeset for help on using the changeset viewer.