Bug 136918 - [CSS Regions] Assertion failure and null dereference crash when using animations and regions
Summary: [CSS Regions] Assertion failure and null dereference crash when using animati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords: AdobeTracked, InRadar
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2014-09-18 04:40 PDT by Radu Stavila
Modified: 2015-02-13 17:30 PST (History)
9 users (show)

See Also:


Attachments
Test-case (1.19 KB, text/html)
2014-09-18 04:40 PDT, Radu Stavila
no flags Details
Patch (8.86 KB, patch)
2014-09-19 01:14 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (9.42 KB, patch)
2014-09-21 23:54 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 2014-09-18 04:40:04 PDT
Created attachment 238305 [details]
Test-case

The attached test-case causes Webkit Nightly to crash. iOS8 Safari is also affected. The assertion is in RenderFlowThread::cachedRegionForCompositedLayer:

    ASSERT(m_layerToRegionMap);
    RenderNamedFlowFragment* namedFlowFragment = m_layerToRegionMap->get(&childLayer);

In this scenario, m_layerToRegionMap is null.
Comment 1 Mihnea Ovidenie 2014-09-18 06:52:52 PDT
The fix for https://bugs.webkit.org/show_bug.cgi?id=129371 remove the check for m_layerToRegionMap causing the reported crash with the stack trace:

ASSERTION FAILED: m_layerToRegionMap
/Users/mihnea/WebKit/Source/WebCore/rendering/RenderFlowThread.cpp(254) : WebCore::RenderNamedFlowFragment *WebCore::RenderFlowThread::cachedRegionForCompositedLayer(WebCore::RenderLayer &) const
1   0x10ffacc50 WTFCrash
2   0x112a6148b WebCore::RenderFlowThread::cachedRegionForCompositedLayer(WebCore::RenderLayer&) const
3   0x112af8fe3 WebCore::RenderLayerBacking::adjustAncestorCompositingBoundsForFlowThread(WebCore::LayoutRect&, WebCore::RenderLayer const*) const
4   0x112af433f WebCore::RenderLayerBacking::updateGeometry()
5   0x112adc1b1 WebCore::RenderLayer::styleChanged(WebCore::StyleDifference, WebCore::RenderStyle const*)
6   0x112b26264 WebCore::RenderLayerModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
7   0x1129e8986 WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
8   0x112b9250d WebCore::RenderReplaced::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
9   0x112aa4e0d WebCore::RenderImage::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
10  0x112a41da1 WebCore::RenderElement::setStyle(WTF::PassRef<WebCore::RenderStyle>)
11  0x112f37b3e WebCore::RenderElement::setAnimatableStyle(WTF::PassRef<WebCore::RenderStyle>)
12  0x112f349ef WebCore::Style::resolveLocal(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
13  0x112f32250 WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
14  0x112f324ae WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
15  0x112f324ae WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
16  0x112f32108 WebCore::Style::resolveTree(WebCore::Document&, WebCore::Style::Change)
17  0x11189f2b6 WebCore::Document::recalcStyle(WebCore::Style::Change)
18  0x11189b84f WebCore::Document::updateStyleIfNeeded()
19  0x1113c1e04 WebCore::AnimationControllerPrivate::fireEventsAndUpdateStyle()
20  0x1113c1406 WebCore::AnimationControllerPrivate::animationTimerFired(WebCore::Timer<WebCore::AnimationControllerPrivate>&)
Comment 2 Mihnea Ovidenie 2014-09-18 08:16:03 PDT
RenderLayer::styleChanged has a FIXME along the code path followed in this case:
....
else if (isComposited()) {
        // FIXME: updating geometry here is potentially harmful, because layout is not up-to-date.
        backing()->updateGeometry();
        backing()->updateAfterDescendents();
}
....

In our case, since we did not update the layout, the maps between the regions and the layers are not up-to-date (null), which causes the crash.
Comment 3 Darin Adler 2014-09-18 10:22:50 PDT
(In reply to comment #2)
>         backing()->updateAfterDescendents();

A side note: That code also has a spelling error since descendants is spelled with an a, not with three eā€™s.
Comment 4 Mihnea Ovidenie 2014-09-19 01:14:29 PDT
Created attachment 238355 [details]
Patch
Comment 5 Andrei Bucur 2014-09-19 06:56:22 PDT
Comment on attachment 238355 [details]
Patch

r=me
Comment 6 Andrei Bucur 2014-09-19 06:56:27 PDT
Comment on attachment 238355 [details]
Patch

r=me
Comment 7 Simon Fraser (smfr) 2014-09-19 08:48:43 PDT
Comment on attachment 238355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238355&action=review

> Source/WebCore/rendering/RenderFlowThread.cpp:257
> +        return 0;

This should return nullptr.

> Source/WebCore/rendering/RenderLayer.cpp:6542
> -        backing()->updateAfterDescendents();
> +        backing()->updateAfterDescendants();

Thank you!
Comment 8 Mihnea Ovidenie 2014-09-21 23:54:16 PDT
Created attachment 238453 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2014-09-22 00:08:54 PDT
Comment on attachment 238453 [details]
Patch for landing

Clearing flags on attachment: 238453

Committed r173806: <http://trac.webkit.org/changeset/173806>
Comment 10 WebKit Commit Bot 2014-09-22 00:09:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2015-02-13 17:30:03 PST
<rdar://problem/19835953>