Bug 304549 – REGRESSION(304729@main) [GLIB] Flaky setBackingProviderLayer crash during RenderLayer destruction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Lauro Moura]

Affecting multiple tests in GTK and WPE post-commit bots, which have _assertions disabled_. A local build with assertions enabled (same config as EWS) did not trigger the crash.

fast/events/tabindex-focus-blur-all.html
media/media-source/media-source-istypesupported-case-sensitive.html
imported/w3c/web-platform-tests/css/cssom-view/smooth-scrollIntoView-with-smooth-fragment-scroll.html
jquery/offset.html
media/modern-media-controls/media-documents/media-document-video-with-initial-audio-layout.html

Regression Range (GTK): r304728 (last good) → r304732 (first bad)
Regression Range (WPE): r304726 (last good) → r304743 (first bad)

Common trace in crashes:

#0  WTFCrash()
#1  WebCore::RenderLayer::setBackingProviderLayer(RenderLayer*, OptionSet<UpdateBackingSharingFlags>)
#2  WebCore::clearBackingSharingLayerProviders(InlineWeakKeyListHashSet<RenderLayer>&, RenderLayer const&, OptionSet<UpdateBackingSharingFlags>)
#3  WebCore::RenderLayerBacking::clearBackingSharingLayers(OptionSet<UpdateBackingSharingFlags>)
#4  WebCore::RenderLayerBacking::willBeDestroyed(OptionSet<UpdateBackingSharingFlags>)
#5  WebCore::RenderLayer::clearBacking(OptionSet<UpdateBackingSharingFlags>, bool)
#6  WebCore::RenderLayer::~RenderLayer()
#7  WebCore::RenderLayerModelObject::willBeDestroyed()
#8  WebCore::Document::destroyRenderTree()

It seems InlineWeakPtr is accessing invalid data after the RenderLayer it points to has already been destroyed. Not sure if it's a problem on the InlineWeakPtr itself, or that it's showing some issue on the RenderLayer destruction order.

[Lauro Moura]

Here's a minimal generated test case that behaves differently in a release WPE build. It passes when assertions are enabled, and fails the last assertion when they are disabled.


```
class TestObject : public UniquelyOwned<TestObject> {
    WTF_DEPRECATED_MAKE_FAST_ALLOCATED(TestObject);
public:
    static UniquelyOwnedPtr<TestObject> create()
    {
        return adoptUniquelyOwned(new TestObject);
    }

    int value() const { return m_value; }

private:
    TestObject() = default;
    int m_value { 42 };
};

TEST(WTF_InlineWeakPtr, WeakPtrBehaviorAfterDestruction)
{
    InlineWeakPtr<TestObject> weakPtr;

    {
        auto obj = TestObject::create();
        weakPtr = obj.get();

        EXPECT_NE(weakPtr.get(), nullptr);
        EXPECT_EQ(weakPtr->value(), 42);

        obj = nullptr;
    }

    EXPECT_EQ(weakPtr.get(), nullptr);
}
```

[Lauro Moura]

Pull request: https://github.com/WebKit/WebKit/pull/55753

[EWS]

Test gardening commit 304824@main (07990cdcb7ca): <https://commits.webkit.org/304824@main>

Reviewed commits have been landed. Closing PR #55753 and removing active labels.

[Lauro Moura]

And FTR, the InlineWeakPtr are also failing in the WPE post-commit bot (no assertions):

Unexpected failures (3)
    /TestWTF
        WTF_HashMap.InlineWeakPtr
        WTF_HashSet.InlineWeakPtr
        WTF_ListHashSet.InlineWeakPtr

**FAIL** WTF_HashMap.InlineWeakPtr

../../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:1556
Expected equality of these values:
  map.get(rawObject1)
    Which is: 1
  0



../../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:1557
Value of: map.contains(rawObject1)
  Actual: true
Expected: false



../../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:1558
Expected equality of these values:
  map.find(rawObject1)
    Which is: 16-byte object <D0-01 00-75 F1-7F 00-00 10-02 00-75 F1-7F 00-00>
  map.end()
    Which is: 16-byte object <10-02 00-75 F1-7F 00-00 10-02 00-75 F1-7F 00-00>



../../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:1559
Expected equality of these values:
  map.begin()
    Which is: 16-byte object <D0-01 00-75 F1-7F 00-00 10-02 00-75 F1-7F 00-00>
  map.end()
    Which is: 16-byte object <10-02 00-75 F1-7F 00-00 10-02 00-75 F1-7F 00-00>



../../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:1564
Expected equality of these values:
  0u
    Which is: 0
  map.size()
    Which is: 1



../../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:1569
Expected: (map.size()) < (16u), actual: 129 vs 16

[Lauro Moura]

Pull request: https://github.com/WebKit/WebKit/pull/55757

[EWS]

Test gardening commit 304829@main (fdb4331beffb): <https://commits.webkit.org/304829@main>

Reviewed commits have been landed. Closing PR #55757 and removing active labels.

[Nikolas Zimmermann]

Pull request: https://github.com/WebKit/WebKit/pull/55800

[EWS]

Test gardening commit 304874@main (8532258871ba): <https://commits.webkit.org/304874@main>

Reviewed commits have been landed. Closing PR #55800 and removing active labels.

[Fujii Hironori]

History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=media%2Fmodern-media-controls%2Fmedia-documents%2Fmedia-document-video-iphone-sizing.html&test=media%2Fmodern-media-controls%2Fmedia-documents%2Fmedia-document-video-with-initial-audio-layout.html&test=jquery%2Foffset.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcssom-view%2Fsmooth-scrollIntoView-with-smooth-fragment-scroll.html&test=media%2Fmedia-source%2Fmedia-source-istypesupported-case-sensitive.html&test=fast%2Fevents%2Ftabindex-focus-blur-all.html&platform=GTK&platform=WPE&style=release

[Fujii Hironori]

As far as I tested, this is reproducible with clang++ 18 but with g++ 13 on my PC.

[Fujii Hironori]

Oh, my bad. the above comment was wrong.
As far as I tested, this is reproducible with g++ 13, but with clang++ 18 on my PC. This seems a GCC specific bug.

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/55944

[EWS]

Committed 305035@main (c37c47affa46): <https://commits.webkit.org/305035@main>

Reviewed commits have been landed. Closing PR #55944 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/167398825>

[Claudio Saavedra]

(In reply to Lauro Moura from comment #1)
> Here's a minimal generated test case that behaves differently in a release
> WPE build. It passes when assertions are enabled, and fails the last
> assertion when they are disabled.
> 
> 
> ```
> class TestObject : public UniquelyOwned<TestObject> {
>     WTF_DEPRECATED_MAKE_FAST_ALLOCATED(TestObject);
> public:
>     static UniquelyOwnedPtr<TestObject> create()
>     {
>         return adoptUniquelyOwned(new TestObject);
>     }
> 
>     int value() const { return m_value; }
> 
> private:
>     TestObject() = default;
>     int m_value { 42 };
> };
> 
> TEST(WTF_InlineWeakPtr, WeakPtrBehaviorAfterDestruction)
> {
>     InlineWeakPtr<TestObject> weakPtr;
> 
>     {
>         auto obj = TestObject::create();
>         weakPtr = obj.get();
> 
>         EXPECT_NE(weakPtr.get(), nullptr);
>         EXPECT_EQ(weakPtr->value(), 42);
> 
>         obj = nullptr;
>     }
> 
>     EXPECT_EQ(weakPtr.get(), nullptr);
> }
> ```

Wouldn't it make sense to make this into an API test to prevent regressions?

[Fujii Hironori]

We already have API tests discovering the bug, for example WTF_HashMap.InlineWeakPtr.
The reason why our EWS couldn't detect this bug was we have only release build EWS with ASSERT_ENABLED .