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)
Created attachment 222558 [details] Patch
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.
(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.
Created attachment 222667 [details] Patch 2 New patch with an additional test file.
Darin, i hope the new patch responds to your question, would you mind taking a new look at it?
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.
Created attachment 223078 [details] Patch for landing
Comment on attachment 223078 [details] Patch for landing Clearing flags on attachment: 223078 Committed r163368: <http://trac.webkit.org/changeset/163368>
All reviewed patches have been landed. Closing bug.