Bug 90182 - ASSERT_NOT_REACHED is reached when adjusting selection across editable boundaries
Summary: ASSERT_NOT_REACHED is reached when adjusting selection across editable bounda...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-28 09:14 PDT by Sean Wang
Modified: 2012-07-19 16:15 PDT (History)
7 users (show)

See Also:


Attachments
patch (3.07 KB, patch)
2012-06-28 10:16 PDT, Sean Wang
no flags Details | Formatted Diff | Diff
Patch (3.09 KB, patch)
2012-07-02 20:31 PDT, Sean Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sean Wang 2012-06-28 09:14:33 PDT
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.
Comment 1 Sean Wang 2012-06-28 10:16:49 PDT
Created attachment 149967 [details]
patch

If the issue is valid and the patch makes sense, I will try to add a LayoutTestCase.
Comment 2 Ryosuke Niwa 2012-06-28 12:25:05 PDT
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.
Comment 3 Ryosuke Niwa 2012-06-28 12:25:57 PDT
Comment on attachment 149967 [details]
patch

Oh oops, I didn't mean to r+ just yet. Can we add a test for this?
Comment 4 Sean Wang 2012-07-02 20:31:11 PDT
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.
Comment 5 Ryosuke Niwa 2012-07-02 20:37:03 PDT
(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?
Comment 6 Sean Wang 2012-07-02 21:06:30 PDT
(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"
Comment 7 Ryosuke Niwa 2012-07-02 21:15:26 PDT
(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.
Comment 8 Sean Wang 2012-07-02 21:31:15 PDT
(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?
Comment 9 Ryosuke Niwa 2012-07-02 21:36:11 PDT
(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.
Comment 10 Sean Wang 2012-07-02 23:35:52 PDT
(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.