Bug 47699 - Crash in WebCore::ApplyStyleCommand::applyBlockStyle
Summary: Crash in WebCore::ApplyStyleCommand::applyBlockStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-14 16:31 PDT by Ryosuke Niwa
Modified: 2010-10-15 11:23 PDT (History)
8 users (show)

See Also:


Attachments
demo (259 bytes, text/html)
2010-10-14 16:31 PDT, Ryosuke Niwa
no flags Details
fixes the crash (3.50 KB, patch)
2010-10-14 22:55 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-10-14 16:31:07 PDT
Created attachment 70796 [details]
demo

We don't check nullity of start/end in applyBlockStyle and we hit an assert inside highestAncestor.
Comment 1 Ryosuke Niwa 2010-10-14 16:34:35 PDT
Oops, sorry.  This isn't really a security bug.
Comment 2 Ryosuke Niwa 2010-10-14 22:19:13 PDT
http://crbug.com/58832
Comment 3 Ryosuke Niwa 2010-10-14 22:55:35 PDT
Created attachment 70834 [details]
fixes the crash
Comment 4 Darin Adler 2010-10-15 10:49:02 PDT
Comment on attachment 70834 [details]
fixes the crash

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

> WebCore/editing/ApplyStyleCommand.cpp:648
> +    if (visibleStart.isNull() || visibleStart.isOrphan() || visibleEnd.isNull() || visibleEnd.isOrphan())
> +        return;

Makes me wish isOrphan returned true for null. There seem to be very few cases where we check isOrphan and want to handle isNull differently.
Comment 5 Ryosuke Niwa 2010-10-15 11:01:54 PDT
(In reply to comment #4)
> (From update of attachment 70834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70834&action=review
> 
> > WebCore/editing/ApplyStyleCommand.cpp:648
> > +    if (visibleStart.isNull() || visibleStart.isOrphan() || visibleEnd.isNull() || visibleEnd.isOrphan())
> > +        return;
> 
> Makes me wish isOrphan returned true for null. There seem to be very few cases where we check isOrphan and want to handle isNull differently.

We could do that given the following result for isOrphan but we probably want to rename isOrphan to isOrphanOrNull if we made this change.

VisiblePosition.h:     bool isOrphan() const { return m_deepPosition.isOrphan(); }
VisiblePosition.h:     bool isOrphan() const { return m_deepPosition.isOrphan(); }
InsertListCommand.cpp:                 if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
InsertListCommand.cpp:                 if (endOfSelection.isNull() || endOfSelection.isOrphan() || startOfLastParagraph.isNull() || startOfLastParagraph.isOrphan()) {
Position.h:     bool isOrphan() const { return m_anchorNode && !m_anchorNode->inDocument(); }
ApplyStyleCommand.cpp:     if (visibleStart.isNull() || visibleStart.isOrphan() || visibleEnd.isNull() || visibleEnd.isOrphan())
ApplyStyleCommand.cpp:     if (visibleStart.isNull() || visibleStart.isOrphan() || visibleEnd.isNull() || visibleEnd.isOrphan())
ApplyStyleCommand.cpp:             if (nextParagraphStart.isOrphan())
ApplyStyleCommand.cpp:     Position s = start.isNull() || start.isOrphan() ? pushDownStart : start;
ApplyStyleCommand.cpp:     Position e = end.isNull() || end.isOrphan() ? pushDownEnd : end;
Editor.cpp:     if (newSelection.start().isOrphan() || newSelection.end().isOrphan())
Editor.cpp:     if (newSelection.start().isOrphan() || newSelection.end().isOrphan())
VisibleSelection.h:     bool isNonOrphanedRange() const { return isRange() && !start().isOrphan() && !end().isOrphan(); }
VisibleSelection.h:     bool isNonOrphanedRange() const { return isRange() && !start().isOrphan() && !end().isOrphan(); }
VisibleSelection.h:     bool isNonOrphanedCaretOrRange() const { return isCaretOrRange() && !start().isOrphan() && !end().isOrphan(); }
VisibleSelection.h:     bool isNonOrphanedCaretOrRange() const { return isCaretOrRange() && !start().isOrphan() && !end().isOrphan(); }
Comment 6 Ryosuke Niwa 2010-10-15 11:05:53 PDT
Committed r69865: <http://trac.webkit.org/changeset/69865>
Comment 7 WebKit Review Bot 2010-10-15 11:23:46 PDT
http://trac.webkit.org/changeset/69865 might have broken Qt Linux Release
The following tests are not passing:
http/tests/security/mixedContent/insecure-css-in-main-frame.html