Bug 90182 – ASSERT_NOT_REACHED is reached when adjusting selection across editable boundaries

• Component: HTML Editing
• Status: RESOLVED INVALID

[Sean Wang]

How to reproduce:
Use setBase() or setExtend across editable boundaries. 

Analysis:
In VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries(), When setting VisibleSelection's m_base/m_extend and validating, ASSERT_NOT_REACHED() is reached since firstEditablePositionAfterPositionInRoot() and lastEditablePositionBeforePositionInRoot() may return Null position.
"
VisiblePosition firstEditablePositionAfterPositionInRoot(const Position& position, Node* highestRoot)
VisiblePosition lastEditablePositionBeforePositionInRoot(const Position& position, Node* highestRoot)
"
When the position's anchor node is equal to the highestRoot, the two functions return Null positions.

This issue was found from BlackBerry SelectionHandler.

The backtrace:
"
Thread [3] (Suspended: Signal 'SIGSEGV' received. Description: Segmentation
fault.)    
    16
WebCore::VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries()
    15 WebCore::VisibleSelection::validate()
    14 WebCore::VisibleSelection::setBase()
    13 BlackBerry::WebKit::SelectionHandler::setSelection()
    12 BlackBerry::WebKit::WebPage::setSelection()
    11 BlackBerry::Platform::MethodDelegate2<void
(BlackBerry::WebKit::WebPage::*)(BlackBerry::Platform::IntPoint const&,
BlackBerry::Platform::IntPoint const&), BlackBerry::WebKit::WebPage,
BlackBerry::Platform::IntPoint, BlackBerry::Platform::IntPoint>::execute()
    10 BlackBerry::Platform::ExecutableMessage::execute()
    9 BlackBerry::Platform::MessageClient::executeMessage()
    8 BlackBerry::Platform::MessageClient::coalesceMessage()
    7 BlackBerry::Platform::MessageClient::receivePendingMessage()
    6 BlackBerry::Platform::MessageClient::processNextMessage()
    5 BlackBerry::Platform::MessageClient::exec()
    4 WebKitThread::exec()
    3 BlackBerry::Platform::MessageClient::run()
    2 timer_settime()
    1 <symbol is not available> 0x00000000    
"
A simple patch will be attached.

[Sean Wang]

Created attachment 149967 [details]
patch

If the issue is valid and the patch makes sense, I will try to add a LayoutTestCase.

[Ryosuke Niwa]

Comment on attachment 149967 [details]
patch

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

This looks sane to me.

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description bug after the bug URL.

[Ryosuke Niwa]

Comment on attachment 149967 [details]
patch

Oh oops, I didn't mean to r+ just yet. Can we add a test for this?

[Sean Wang]

Created attachment 150522 [details]
Patch

It hard to write a reproducible test case for this bug, but the test case in the commit log of this patch should already include this case.

[Ryosuke Niwa]

(In reply to comment #4)
> Created an attachment (id=150522) [details]
> Patch
> 
> It hard to write a reproducible test case for this bug, but the test case in the commit log of this patch should already include this case.

What do you mean by that?

[Sean Wang]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=150522) [details] [details]
> > Patch
> > 
> > It hard to write a reproducible test case for this bug, but the test case in the commit log of this patch should already include this case.
> 
> What do you mean by that?

Could I use the existed test case for this patch?  I'v already write it in the commit log.
"Test: editing/selection/select-out-of-editable.html"

[Ryosuke Niwa]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Created an attachment (id=150522) [details] [details] [details]
> > > Patch
> > > 
> > > It hard to write a reproducible test case for this bug, but the test case in the commit log of this patch should already include this case.
> > 
> > What do you mean by that?
> 
> Could I use the existed test case for this patch?  I'v already write it in the commit log.
> "Test: editing/selection/select-out-of-editable.html"

You're hitting this assertion when you run the test on your port? That sounds red-herring to me. Maybe you have a bug in your port's code.

[Sean Wang]

(In reply to comment #7)

> 
> You're hitting this assertion when you run the test on your port? That sounds red-herring to me. Maybe you have a bug in your port's code.

I hit the assertion in my port by manually testing. But it had disappeared today from my port. So I can't tell the result of this testcase with my porting. But I reproduced it manually like this testcase.

Do you accept this patch if there is not a test case? Should I remove it from the commit log?

[Ryosuke Niwa]

(In reply to comment #8)
> I hit the assertion in my port by manually testing. But it had disappeared today from my port. So I can't tell the result of this testcase with my porting. But I reproduced it manually like this testcase.
> 
> Do you accept this patch if there is not a test case? Should I remove it from the commit log?

It's not clear to me why we would need to make this change if the assertion failure is not reproducible. I've recently fixed comparePositions in http://trac.webkit.org/changeset/121303 so it might be related to do that.

[Sean Wang]

(In reply to comment #9)
> It's not clear to me why we would need to make this change if the assertion failure is not reproducible. I've recently fixed comparePositions in http://trac.webkit.org/changeset/121303 so it might be related to do that.

I have not updated to your change, and there is another one may relate to this bug: 0077f663bd01c590268cb1f5be97ef9d2bf34005

Because the problem in our port's code has gone and it is not reproducible now, I would like to close this bug as invalid.