Bug 202904 – Chromium test-case asserts with ASSERTION FAILED: propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect)

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Emilio Cobos Álvarez (:emilio)]

On master (247b0314320d499ae788b6ea993aa1d98e2d607e / r250962), WebKitGTK build.

Running this test-case locally:

 * https://cs.chromium.org/chromium/src/third_party/blink/web_tests/editing/execCommand/justify-right-in-effect-crash.html?rcl=753caf715d8f30f0c673f1b4b36dadfc75c3201f

Asserts like:

ASSERTION FAILED: propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect)
../../Source/WebCore/editing/markup.cpp(366) : void WebCore::StyledMarkupAccumulator::appendStyleNodeOpenTag(WTF::StringBuilder&, WebCore::StyleProperties*, WebCore::Document&, bool)
1   0x7fb15b8863d3 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x9) [0x7fb15b8863d3]
2   0x7fb16752f5f2 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN3WTF15CrashOnOverflow10overflowedEv+0) [0x7fb16752f5f2]
3   0x7fb169b6d396 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore23StyledMarkupAccumulator22appendStyleNodeOpenTagERN3WTF13StringBuilderEPNS_15StylePropertiesERNS_8DocumentEb+0x62) [0x7fb169b6d396]
4   0x7fb169b6d783 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore23StyledMarkupAccumulator10appendTextERN3WTF13StringBuilderERKNS_4TextE+0x147) [0x7fb169b6d783]
5   0x7fb169b27241 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore17MarkupAccumulator20appendNonElementNodeERN3WTF13StringBuilderERKNS_4NodeEPNS1_7HashMapINS1_10AtomStringEPNS1_14AtomStringImplENS1_14AtomStringHashENS1_10HashTraitsIS8_EENSC_ISA_EEEE+0x9d) [0x7fb169b27241]
6   0x7fb169b25533 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore17MarkupAccumulator18startAppendingNodeERKNS_4NodeEPN3WTF7HashMapINS4_10AtomStringEPNS4_14AtomStringImplENS4_14AtomStringHashENS4_10HashTraitsIS6_EENSA_IS8_EEEE+0x7b) [0x7fb169b25533]
7   0x7fb169b6e7c2 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xcc1a7c2) [0x7fb169b6e7c2]
8   0x7fb169b6eae3 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore23StyledMarkupAccumulator29traverseNodesForSerializationEPNS_4NodeES2_NS0_17NodeTraversalModeE+0x275) [0x7fb169b6eae3]
9   0x7fb169b6e64e /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore23StyledMarkupAccumulator14serializeNodesERKNS_8PositionES3_+0x1ee) [0x7fb169b6e64e]
10  0x7fb169b6ff92 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xcc1bf92) [0x7fb169b6ff92]
11  0x7fb169b70631 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore35serializePreservingVisualAppearanceERKNS_5RangeEPN3WTF6VectorIPNS_4NodeELm0ENS3_15CrashOnOverflowELm16EEENS_22AnnotateForInterchangeENS_22ConvertBlocksToInlinesENS_11ResolveURLsE+0x80) [0x7fb169b70631]
12  0x7fb16b118a30 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore20CompositeEditCommand14moveParagraphsERKNS_15VisiblePositionES3_S3_bb+0x614) [0x7fb16b118a30]
13  0x7fb16b117232 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore20CompositeEditCommand42moveParagraphContentsToNewBlockIfNecessaryERKNS_8PositionE+0x4c6) [0x7fb16b117232]
14  0x7fb16b1086c3 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore17ApplyStyleCommand15applyBlockStyleERNS_12EditingStyleE+0x611) [0x7fb16b1086c3]
15  0x7fb16b1080ad /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore17ApplyStyleCommand7doApplyEv+0x157) [0x7fb16b1080ad]
16  0x7fb16b112cd3 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore20CompositeEditCommand5applyEv+0xf5) [0x7fb16b112cd3]
17  0x7fb169ae51f5 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore6Editor19applyParagraphStyleEPNS_15StylePropertiesENS_10EditActionE+0x235) [0x7fb169ae51f5]
18  0x7fb169af5250 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xcba1250) [0x7fb169af5250]
19  0x7fb169af7070 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xcba3070) [0x7fb169af7070]
20  0x7fb169afa62a /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNK7WebCore6Editor7Command7executeERKN3WTF6StringEPNS_5EventE+0xdc) [0x7fb169afa62a]
21  0x7fb1698e5268 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore8Document11execCommandERKN3WTF6StringEbS4_+0x56) [0x7fb1698e5268]
22  0x7fb1685f3694 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xb69f694) [0x7fb1685f3694]
23  0x7fb16860d1b6 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0xb6b91b6) [0x7fb16860d1b6]
24  0x7fb1685f3702 /home/emilio/src/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore38jsDocumentPrototypeFunctionExecCommandEPN3JSC14JSGlobalObjectEPNS0_9CallFrameE+0x23) [0x7fb1685f3702]
25  0x7fb105dfa16b [0x7fb105dfa16b]

[Radar WebKit Bug Importer]

<rdar://problem/56271311>

[Frédéric Wang (:fredw)]

Created attachment 433930 [details]
Patch

Below is the backtrace I have. The assert expects -webkit-text-decorations-in-effect to be removed and the comment even says this should have happened during the wrappingStyleForSerialization call. However, this only happens if annotate is AnnotateForInterchange::YES: https://webkit-search.igalia.com/webkit/source/Source/WebCore/editing/EditingStyle.cpp#1197

As you can see in the backtrace, it's however set to AnnotateForInterchange::NO. Attached is a trivial patch that sets it to AnnotateForInterchange::YES instead, but not sure that's correct. It's however indeed fixing the ASSERT of the testcase.

#0  WTFCrash() () at ../../Source/WTF/wtf/Assertions.cpp:319
#1  0x00007f895c246c3c in CRASH_WITH_INFO(...) () at WTF/Headers/wtf/Assertions.h:744
#2  0x00007f8962fd21cc in WebCore::StyledMarkupAccumulator::appendStyleNodeOpenTag(WTF::StringBuilder&, WebCore::StyleProperties*, WebCore::Document&, bool)
    (this=0x7ffcbe378cd0, out=..., style=0x60b0000de610, document=warning: RTTI symbol not found for class 'WebCore::HTMLDocument'
..., isBlock=false)
    at https://webkit-search.igalia.com/webkit/source/Source/WebCore/editing/markup.cpp#387
#3  0x00007f8962fd2ebc in WebCore::StyledMarkupAccumulator::appendText(WTF::StringBuilder&, WebCore::Text const&) (this=0x7ffcbe378cd0, out=..., text=...)
    at ../../Source/WebCore/editing/markup.cpp:438
#4  0x00007f8962ee71b0 in WebCore::MarkupAccumulator::appendNonElementNode(WTF::StringBuilder&, WebCore::Node const&, WTF::HashMap<WTF::AtomString, WTF::AtomStringImpl*, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomStringImpl*>, WTF::HashTableTraits>*) (this=0x7ffcbe378cd0, result=..., node=..., namespaces=0x0)
    at ../../Source/WebCore/editing/MarkupAccumulator.cpp:551
#5  0x00007f8962ee34fe in WebCore::MarkupAccumulator::startAppendingNode(WebCore::Node const&, WTF::HashMap<WTF::AtomString, WTF::AtomStringImpl*, WTF::DefaultHash<WTF::AtomString>, WTF::HashTraits<WTF::AtomString>, WTF::HashTraits<WTF::AtomStringImpl*>, WTF::HashTableTraits>*)
    (this=0x7ffcbe378cd0, node=..., namespaces=0x0)
    at ../../Source/WebCore/editing/MarkupAccumulator.cpp:254
#6  0x00007f8962fd51e1 in operator()(WebCore::Node&) const (__closure=0x7ffcbe378620, node=...)
    at ../../Source/WebCore/editing/markup.cpp:662
#7  0x00007f8962fd5b80 in WebCore::StyledMarkupAccumulator::traverseNodesForSerialization(WebCore::Node*, WebCore::Node*, WebCore::StyledMarkupAccumulator::NodeTraversalMode)
--Type <RET> for more, q to quit, c to continue without paging--c
   cd0, startNode=0x60b0000dc670, pastEnd=0x60c0002a3e80, traversalMode=WebCore::StyledMarkupAccumulator::NodeTraversalMode::EmitString) at ../../Source/WebCore/editing/markup.cpp:708
#8  0x00007f8962fd4d5e in WebCore::StyledMarkupAccumulator::serializeNodes(WebCore::Position const&, WebCore::Position const&) (this=0x7ffcbe378cd0, start=..., end=...) at https://webkit-search.igalia.com/webkit/source/Source/WebCore/editing/markup.cpp#640
#9  0x00007f8962fd8573 in WebCore::serializePreservingVisualAppearanceInternal(WebCore::Position const&, WebCore::Position const&, WTF::Vector<WebCore::Node*, 0, WTF::CrashOnOverflow, 16, WTF::FastMalloc>*, WebCore::ResolveURLs, WebCore::SerializeComposedTree, WebCore::AnnotateForInterchange, WebCore::ConvertBlocksToInlines, WebCore::StandardFontFamilySerializationMode, WebCore::MSOListMode) (start=..., end=..., nodes=0x0, resolveURLs=WebCore::ResolveURLs::No, serializeComposedTree=WebCore::SerializeComposedTree::No, annotate=WebCore::AnnotateForInterchange::No, convertBlocksToInlines=WebCore::ConvertBlocksToInlines::Yes, standardFontFamilySerializationMode=WebCore::StandardFontFamilySerializationMode::Keep, msoListMode=WebCore::MSOListMode::DoNotPreserve) at ../../Source/WebCore/editing/markup.cpp:918
#10 0x00007f8962fd91e1 in WebCore::serializePreservingVisualAppearance(WebCore::SimpleRange const&, WTF::Vector<WebCore::Node*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>*, WebCore::AnnotateForInterchange, WebCore::ConvertBlocksToInlines, WebCore::ResolveURLs) (range=..., nodes=0x0, annotate=WebCore::AnnotateForInterchange::No, convertBlocksToInlines=WebCore::ConvertBlocksToInlines::Yes, resolveURLs=WebCore::ResolveURLs::No) at ../../Source/WebCore/editing/markup.cpp:980
#11 0x00007f8967146d6f in WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) (this=0x613000089640, startOfParagraphToMove=..., endOfParagraphToMove=..., destination=..., preserveSelection=false, preserveStyle=true) at https://webkit-search.igalia.com/webkit/source/Source/WebCore/editing/CompositeEditCommand.cpp#1470

[Frédéric Wang (:fredw)]

Created attachment 434825 [details]
Patch

Probably not ideal, but this just imports the change from Chromium (see https://codereview.chromium.org/1522063002).

[Darin Adler]

Comment on attachment 434825 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:387
> +    // With AnnotateForInterchange::Yes, wrappingStyleForSerialization should have removed -webkit-text-decorations-in-effect
> +    ASSERT(!shouldAnnotate() || propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));

Changing the assertion is OK, but I think this is sort of missing the big picture. Having a webpage specify "-webkit-text-decorations-in-effect" doesn’t really make logical sense. It’s a property intended for internal use in the engine. The key to this test case is that it specifies that property explicitly in a style sheet.

Might be something to clean up here some day, but in the mean time loosening this assertion is a fine thing to do so we don’t block fuzz testing. Changing the assertion has no effect on WebKit behavior.

[Frédéric Wang (:fredw)]

Comment on attachment 434825 [details]
Patch

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

>> Source/WebCore/editing/markup.cpp:387
>> +    ASSERT(!shouldAnnotate() || propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));
> 
> Changing the assertion is OK, but I think this is sort of missing the big picture. Having a webpage specify "-webkit-text-decorations-in-effect" doesn’t really make logical sense. It’s a property intended for internal use in the engine. The key to this test case is that it specifies that property explicitly in a style sheet.
> 
> Might be something to clean up here some day, but in the mean time loosening this assertion is a fine thing to do so we don’t block fuzz testing. Changing the assertion has no effect on WebKit behavior.

Would it be possible to really make it an internal property, not web-exposed? IIUC, this is something that is now possible in WebKit. I guess the best thing to do is to open a separate bug for that.

[EWS]

Committed r282209 (?): <https://commits.webkit.org/r282209>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434825 [details].

[Darin Adler]

(In reply to Frédéric Wang (:fredw) from comment #5)
> Would it be possible to really make it an internal property, not
> web-exposed? IIUC, this is something that is now possible in WebKit. I guess
> the best thing to do is to open a separate bug for that.

That might be possible. I hope so!

Compatibility with existing iOS and macOS apps that might rely on getting the value of the property might be an issue.