Bug 87463 – Assertion failure due to Position::upstream() and downstream() not considering shadow hosts as an atomic node and thus crossing editing boundaries

• Component: HTML Editing
• Status: NEW

[Shinya Kawanaka]

After m_start = m_start.downstream(), m_start might get into editing boundaries.
This bug will cause a lot of crashes editing with Shadow DOM.

[Shinya Kawanaka]

I'm now making a patch. I'll submit it soon.

[Ryosuke Niwa]

(In reply to comment #0)
> After m_start = m_start.downstream(), m_start might get into editing boundaries.

What do you mean by getting into editing boundaries? Are you saying that it erroneously crosses editing boundaries?

[Shinya Kawanaka]

(In reply to comment #2)
> (In reply to comment #0)
> > After m_start = m_start.downstream(), m_start might get into editing boundaries.
> 
> What do you mean by getting into editing boundaries? Are you saying that it erroneously crosses editing boundaries?

Sorry for ambiguous term... Yes, you are right.

[Shinya Kawanaka]

Created attachment 144334 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144334 [details]
Patch

Attachment 144334 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12841255

New failing tests:
editing/execCommand/delete-table-with-empty-contents.html
editing/selection/selection-plugin-clear-crash.html
editing/selection/4895428-4.html
editing/pasteboard/replacement-fragment-remove-unrendered-node-crash.html

[WebKit Review Bot]

Created attachment 144344 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

Comment on attachment 144334 [details]
Patch

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

> Source/WebCore/dom/Position.cpp:93
> +    if (position.anchorType() == Position::PositionIsAfterAnchor) {

In my humble opinion, if this condition is changed to *if ~ else if* or *switch ~ case*, it seems to me "return position;" on line 100, line 110 can be removed.

> Source/WebCore/dom/Position.cpp:116
> +

It looks this is unneeded line.

[Shinya Kawanaka]

Created attachment 144519 [details]
Patch

[Build Bot]

Comment on attachment 144519 [details]
Patch

Attachment 144519 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12840551

[Build Bot]

Comment on attachment 144519 [details]
Patch

Attachment 144519 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12839572

[Early Warning System Bot]

Comment on attachment 144519 [details]
Patch

Attachment 144519 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12849457

[Gyuyoung Kim]

Comment on attachment 144519 [details]
Patch

Attachment 144519 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12849497

[Early Warning System Bot]

Comment on attachment 144519 [details]
Patch

Attachment 144519 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12843524

[WebKit Review Bot]

Comment on attachment 144519 [details]
Patch

Attachment 144519 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12850492

[Ryosuke Niwa]

Comment on attachment 144519 [details]
Patch

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

> Source/WebCore/dom/Position.cpp:80
> +    Node* node = position.deprecatedNode();

Please try not to use deprecatedNode and/or anchorNode in new code unless you're explicitly checking the position's type.
See https://rniwa.com/2011-06-26/position-and-anchor-types/

> Source/WebCore/dom/Position.cpp:85
> +    if (node->parentNode() && node->parentNode()->rendererIsEditable() != node->parentNode()->rendererIsEditable())

When could node->parentNode()->rendererIsEditable() ever be different from node->parentNode()->rendererIsEditable()?
We definitely need a test case that executes this line. r- because of this.

> Source/WebCore/dom/Position.cpp:91
> +static PositionIterator visiblePositionIterator(const Position& position)

I don't understand what this function does. Please make the function name more descriptive of what it does.

> Source/WebCore/dom/Position.cpp:96
> +            return createLegacyEditingPosition(position.anchorNode(), caretMaxOffset(position.anchorNode()));

Please don't instantiate legacy positions. I think you're not fixing the bug properly if you have to instantiate legacy position here.

> Source/WebCore/dom/Position.cpp:109
> +    default:

In most cases, we prefer listing all values of enum in switch statements instead of using the default label.

[Shinya Kawanaka]

Created attachment 145000 [details]
WIP

[WebKit Review Bot]

Comment on attachment 145000 [details]
WIP

Attachment 145000 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12861310

New failing tests:
editing/execCommand/delete-table-with-empty-contents.html
editing/selection/selection-plugin-clear-crash.html
editing/selection/4895428-4.html
editing/shadow/selection-crossing-editing-shadow-boundaries.html

[WebKit Review Bot]

Created attachment 145076 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

Comment on attachment 145000 [details]
WIP

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

> Source/WebCore/dom/Position.cpp:638
> +#if 0

I think it is better to write a comment to explain why below codes is disabled.

For example,
http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp#L135

> Source/WebCore/dom/Position.cpp:765
> +#if 0

ditto.

[Shinya Kawanaka]

(In reply to comment #19)
> (From update of attachment 145000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145000&action=review
> 
> > Source/WebCore/dom/Position.cpp:638
> > +#if 0
> 
> I think it is better to write a comment to explain why below codes is disabled.
> 
> For example,
> http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/QtTapGestureRecognizer.cpp#L135
> 
> > Source/WebCore/dom/Position.cpp:765
> > +#if 0
> 
> ditto.

It's just a work-in-progress patch to use EWS. Sorry if confusing.

[Shinya Kawanaka]

unassigned me for now. This is not only Shadow-related problem. I might attack this later though.