Bug 231235 - ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
Summary: ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-05 09:54 PDT by Chris Dumez
Modified: 2023-01-23 09:06 PST (History)
5 users (show)

See Also:


Attachments
Patch (10.27 KB, patch)
2021-10-05 10:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2021-10-05 11:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.91 KB, patch)
2021-10-05 14:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-10-05 09:54:43 PDT
ASSERTION FAILED: m_callback->hasCallback()
./page/IntersectionObserver.cpp(278) : void WebCore::IntersectionObserver::notify()
1   0x4744dd069 WTFCrash
2   0x48611b24b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x489e238fb WebCore::IntersectionObserver::notify()
4   0x489046a39 WebCore::Document::updateIntersectionObservations()
5   0x489e80f59 WebCore::Page::updateRendering()::$_27::operator()(WebCore::Document&) const
6   0x489e80ef1 WTF::Detail::CallableWrapper<WebCore::Page::updateRendering()::$_27, void, WebCore::Document&>::call(WebCore::Document&)
7   0x488f9b647 WTF::Function<void (WebCore::Document&)>::operator()(WebCore::Document&) const
8   0x489e3c286 WebCore::Page::forEachDocument(WTF::Function<void (WebCore::Document&)> const&) const
9   0x489e433b9 WebCore::Page::updateRendering()::$_21::operator()(WebCore::RenderingUpdateStep, WTF::Function<void (WebCore::Document&)> const&) const
10  0x489e42ffa WebCore::Page::updateRendering()
11  0x45c6e2b61 WebKit::WebPage::updateRendering()
12  0x45bef992d WebKit::TiledCoreAnimationDrawingArea::updateRendering(WebKit::TiledCoreAnimationDrawingArea::UpdateRenderingType)
13  0x45bef9898 WebKit::TiledCoreAnimationDrawingArea::forceRepaint()
14  0x45c6e1502 WebKit::WebPage::forceRepaintWithoutCallback()
15  0x45c2d8d3d WKBundlePageForceRepaint
16  0x454c2a62e WTR::InjectedBundlePage::dump()
Comment 1 Chris Dumez 2021-10-05 09:54:57 PDT
<rdar://80837616>
Comment 2 Chris Dumez 2021-10-05 10:05:43 PDT
Created attachment 440227 [details]
Patch
Comment 3 Chris Dumez 2021-10-05 11:41:42 PDT
Created attachment 440240 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-10-05 13:00:15 PDT
Does Resize Observer need the same fix?
Comment 5 Chris Dumez 2021-10-05 13:04:43 PDT
(In reply to Simon Fraser (smfr) from comment #4)
> Does Resize Observer need the same fix?

I'll check but I doubt we'd expect a resize observation to ever be sent for a detached element?
Comment 6 Ryosuke Niwa 2021-10-05 13:36:49 PDT
(In reply to Chris Dumez from comment #5)
> (In reply to Simon Fraser (smfr) from comment #4)
> > Does Resize Observer need the same fix?
> 
> I'll check but I doubt we'd expect a resize observation to ever be sent for
> a detached element?

I think we do for the initial observation. Also, right after the element got detached from the document because then the element becomes (0, 0) instead of whatever size it used to have. Come to think of it, wouldn't intersection observer do the same thing?

If a node was in the document and intersection observer notified of its current position, wouldn't removing the node result in another notification?
Comment 7 Chris Dumez 2021-10-05 14:12:05 PDT
(In reply to Ryosuke Niwa from comment #6)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Simon Fraser (smfr) from comment #4)
> > > Does Resize Observer need the same fix?
> > 
> > I'll check but I doubt we'd expect a resize observation to ever be sent for
> > a detached element?
> 
> I think we do for the initial observation. Also, right after the element got
> detached from the document because then the element becomes (0, 0) instead
> of whatever size it used to have. Come to think of it, wouldn't intersection
> observer do the same thing?
> 
> If a node was in the document and intersection observer notified of its
> current position, wouldn't removing the node result in another notification?

I am adding a test to cover the case where the target is initially visible and then we remove it from the document (it does trigger an observation notification in the intersection handler case).

I will need to test the resize observer too. It may well have the exact same bug based on your comments.
Comment 8 Chris Dumez 2021-10-05 14:22:00 PDT
(In reply to Ryosuke Niwa from comment #6)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Simon Fraser (smfr) from comment #4)
> > > Does Resize Observer need the same fix?
> > 
> > I'll check but I doubt we'd expect a resize observation to ever be sent for
> > a detached element?
> 
> I think we do for the initial observation. Also, right after the element got
> detached from the document because then the element becomes (0, 0) instead
> of whatever size it used to have. Come to think of it, wouldn't intersection
> observer do the same thing?

Based on my local testing, the same test does not crash for the resize observer. We don't dispatch an initial observation at all it seems when the element is initially disconnected.
Comment 9 Chris Dumez 2021-10-05 14:33:51 PDT
Created attachment 440270 [details]
Patch
Comment 10 Chris Dumez 2021-10-05 14:38:31 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Ryosuke Niwa from comment #6)
> > (In reply to Chris Dumez from comment #5)
> > > (In reply to Simon Fraser (smfr) from comment #4)
> > > > Does Resize Observer need the same fix?
> > > 
> > > I'll check but I doubt we'd expect a resize observation to ever be sent for
> > > a detached element?
> > 
> > I think we do for the initial observation. Also, right after the element got
> > detached from the document because then the element becomes (0, 0) instead
> > of whatever size it used to have. Come to think of it, wouldn't intersection
> > observer do the same thing?
> 
> Based on my local testing, the same test does not crash for the resize
> observer. We don't dispatch an initial observation at all it seems when the
> element is initially disconnected.

I'll add some corresponding resize observer tests in a separate patch. I could not get them to hit the assertion though.
Comment 11 Simon Fraser (smfr) 2021-10-05 16:04:56 PDT
(In reply to Chris Dumez from comment #8)

> Based on my local testing, the same test does not crash for the resize
> observer. We don't dispatch an initial observation at all it seems when the
> element is initially disconnected.

That would make a nice layout/wpt test.
Comment 12 EWS 2021-10-05 17:16:40 PDT
Committed r283590 (?): <https://commits.webkit.org/r283590>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440270 [details].