Bug 127687

Summary: ASSERTION FAILED: !object || (object->isRenderBlock())
Product: WebKit Reporter: Zoltan Horvath <zoltan>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: abucur, commit-queue, esprehn+autocc, glenn, kondapallykalyan, mihnea, WebkitBugTracker, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WTFCrashWithSecurityImplication
none
Patch
none
Patch 2
none
Patch for landing none

Zoltan Horvath
Reported 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)
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
DetailsFormatted DiffDiff
Patch 2 (10.15 KB, patch)
2014-01-30 06:55 PST, Mihnea Ovidenie 		no flags
DetailsFormatted DiffDiff
Patch for landing (10.60 KB, patch)
2014-02-04 00:07 PST, Mihnea Ovidenie 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mihnea Ovidenie
Comment 1 2014-01-29 01:17:26 PST
Created attachment 222558 [details] Patch
Darin Adler
Comment 2 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.
Mihnea Ovidenie
Comment 3 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.
Mihnea Ovidenie
Comment 4 2014-01-30 06:55:13 PST
Created attachment 222667 [details] Patch 2 New patch with an additional test file.
Mihnea Ovidenie
Comment 5 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?
Ryosuke Niwa
Comment 6 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.
Mihnea Ovidenie
Comment 7 2014-02-04 00:07:41 PST
Created attachment 223078 [details] Patch for landing
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2014-02-04 00:49:01 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.