Bug 127687 - ASSERTION FAILED: !object || (object->isRenderBlock())
Summary: ASSERTION FAILED: !object || (object->isRenderBlock())
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:
Depends on:
Blocks:
 
Reported: 2014-01-27 10:34 PST by Zoltan Horvath
Modified: 2014-02-04 00:49 PST (History)
8 users (show)

See Also:


Attachments
WTFCrashWithSecurityImplication (3.33 KB, text/plain)
2014-01-27 10:34 PST, Zoltan Horvath
no flags Details
Patch (6.17 KB, patch)
2014-01-29 01:17 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (10.15 KB, patch)
2014-01-30 06:55 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (10.60 KB, patch)
2014-02-04 00:07 PST, 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 Zoltan Horvath 2014-01-27 10:34:29 PST
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)
Comment 1 Mihnea Ovidenie 2014-01-29 01:17:26 PST
Created attachment 222558 [details]
Patch
Comment 2 Darin Adler 2014-01-29 10:10:07 PST
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.
Comment 3 Mihnea Ovidenie 2014-01-30 06:48:47 PST
(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.
Comment 4 Mihnea Ovidenie 2014-01-30 06:55:13 PST
Created attachment 222667 [details]
Patch 2

New patch with an additional test file.
Comment 5 Mihnea Ovidenie 2014-01-31 10:43:12 PST
Darin, i hope the new patch responds to your question, would you mind taking a new look at it?
Comment 6 Ryosuke Niwa 2014-02-03 20:47:08 PST
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.
Comment 7 Mihnea Ovidenie 2014-02-04 00:07:41 PST
Created attachment 223078 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2014-02-04 00:48:58 PST
Comment on attachment 223078 [details]
Patch for landing

Clearing flags on attachment: 223078

Committed r163368: <http://trac.webkit.org/changeset/163368>
Comment 9 WebKit Commit Bot 2014-02-04 00:49:01 PST
All reviewed patches have been landed.  Closing bug.