Bug 127687 – ASSERTION FAILED: !object || (object->isRenderBlock())

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Zoltan Horvath]

Created attachment 222336 [details]
WTFCrashWithSecurityImplication

I attached the recent backtrace for the assertion (WTFCrashWithSecurityImplication),

How to reproduce:
1. Open: https://bug-103324-attachments.webkit.org/attachment.cgi?id=176114 (test case of bug #103324)
4. Mouse click outside of the video's area, but inside the region's area (inside red border)

[Mihnea Ovidenie]

Created attachment 222558 [details]
Patch

[Darin Adler]

Comment on attachment 222558 [details]
Patch

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

> Source/WebCore/rendering/RenderRegion.cpp:110
>      if (!isValid() || !m_flowThread->firstChild()) // checking for empty region blocks.

Do we still need this null check on the first child? Why?

> Source/WebCore/rendering/RenderRegion.cpp:114
> -    return toRenderBlock(m_flowThread->firstChild())->positionForPoint(mapRegionPointIntoFlowThreadCoordinates(point));
> +    LayoutPoint mappedPoint = mapRegionPointIntoFlowThreadCoordinates(point);
> +    return m_flowThread->positionForPoint(mappedPoint);

I understand why we are using m_flowThread instead of the first child, but not why this line of code is being broken into two lines of code.

[Mihnea Ovidenie]

(In reply to comment #2)
> (From update of attachment 222558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222558&action=review
> 
> > Source/WebCore/rendering/RenderRegion.cpp:110
> >      if (!isValid() || !m_flowThread->firstChild()) // checking for empty region blocks.
> 
> Do we still need this null check on the first child? Why?
> 

The null check on the first child is used to check if the region is attached to a flow thread that has children to be rendered inside the region. If the flow thread does not have such children, we then let the region behave like a normal element without any children.
The new patch will add also a test for this situation - it fails if we take the condition out.

> > Source/WebCore/rendering/RenderRegion.cpp:114
> > -    return toRenderBlock(m_flowThread->firstChild())->positionForPoint(mapRegionPointIntoFlowThreadCoordinates(point));
> > +    LayoutPoint mappedPoint = mapRegionPointIntoFlowThreadCoordinates(point);
> > +    return m_flowThread->positionForPoint(mappedPoint);
> 
> I understand why we are using m_flowThread instead of the first child, but not why this line of code is being broken into two lines of code.
It should not be broken, you are right.

[Mihnea Ovidenie]

Created attachment 222667 [details]
Patch 2

New patch with an additional test file.

[Mihnea Ovidenie]

Darin, i hope the new patch responds to your question, would you mind taking a new look at it?

[Ryosuke Niwa]

Comment on attachment 222667 [details]
Patch 2

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

> LayoutTests/fast/regions/selection/position-for-point-inline-content-node.html:20
> +    <script>
> +        onMouseUpLogSelectionAndFocus("selected-content", "focus-node", "focus-offset");
> +    </script>
> +    <script>

Why do we need two script elements?

> LayoutTests/fast/regions/selection/selection-ended-in-empty-region.html:20
> +    <script>
> +        onMouseUpLogSelectionAndFocus("selected-content", "focus-node", "focus-offset");
> +    </script>
> +    <script>

Ditto.

[Mihnea Ovidenie]

Created attachment 223078 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 223078 [details]
Patch for landing

Clearing flags on attachment: 223078

Committed r163368: <http://trac.webkit.org/changeset/163368>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.