Currently width shorthand and height shorthand is not generated by makeprop.pl and the same css property names are already placed in CSSPropertyNames.in. So with enabled device adaptation, crash occurs when visiting webpage using @viewport rule because there is no shorthand property(width and height) for corresponding longhand property. This patch prevents the crash.
Created attachment 259836 [details] Patch
Created attachment 259837 [details] Patch
For your reference, this crash occurs as assertion fail in debug build. The callstack is as below. (gdb) bt #0 0x00007f094663a0aa in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 #1 0x00007f094c32fd56 in WebCore::indexOfShorthandForLonghand ( shorthandID=WebCore::CSSPropertyWidth, shorthands=...) at ../../Source/WebCore/css/StylePropertyShorthand.cpp:83 #2 0x00007f094d4b2f7c in WebCore::CSSParser::addProperty ( this=0x7ffe219d72e0, propId=WebCore::CSSPropertyMinWidth, value=..., important=false, implicit=false) at ../../Source/WebCore/css/CSSParser.cpp:1621 #3 0x00007f094d4e2ecc in WebCore::CSSParser::parseViewportProperty ( this=0x7ffe219d72e0, propId=WebCore::CSSPropertyMinWidth, important=false) at ../../Source/WebCore/css/CSSParser.cpp:12785 #4 0x00007f094d4e2fed in WebCore::CSSParser::parseViewportShorthand ( this=0x7ffe219d72e0, propId=WebCore::CSSPropertyWidth, first=WebCore::CSSPropertyMinWidth, second=WebCore::CSSPropertyMaxWidth, important=false) at ../../Source/WebCore/css/CSSParser.cpp:12802 #5 0x00007f094d4e2d19 in WebCore::CSSParser::parseViewportProperty ( this=0x7ffe219d72e0, propId=WebCore::CSSPropertyWidth, important=false) at ../../Source/WebCore/css/CSSParser.cpp:12755 #6 0x00007f094d4b3fe4 in WebCore::CSSParser::parseValue (this=0x7ffe219d72e0, propId=WebCore::CSSPropertyWidth, important=false) at ../../Source/WebCore/css/CSSParser.cpp:1899 #7 0x00007f094d978884 in cssyyparse (parser=0x7ffe219d72e0) at /home/joonghunpark/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/CSSGrammar.y:1320 #8 0x00007f094d4aed11 in WebCore::CSSParser::parseSheet (this=0x7ffe219d72e0, sheet=0x7f08c03f9820, string=..., textPosition=..., ruleSourceDataResult=0x0, logErrors=true) at ../../Source/WebCore/css/CSSParser.cpp:455 #9 0x00007f094d589449 in WebCore::StyleSheetContents::parseAuthorStyleSheet ( this=0x7f08c03f9820, cachedStyleSheet=0x7f08c29d3940, securityOrigin=0x7f092f38f348) at ../../Source/WebCore/css/StyleSheetContents.cpp:315 #10 0x00007f094c59bf47 in WebCore::HTMLLinkElement::setCSSStyleSheet ( this=0x7f08c3ad5190, href=..., baseURL=..., charset=..., cachedStyleSheet=0x7f08c29d3940) at ../../Source/WebCore/html/HTMLLinkElement.cpp:349 #11 0x00007f094c82e3f1 in WebCore::CachedCSSStyleSheet::checkNotify ( this=0x7f08c29d3940) at ../../Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:113 #12 0x00007f094c82e27b in WebCore::CachedCSSStyleSheet::finishLoading ( this=0x7f08c29d3940, data=0x7f08c1bcc0c0) at ../../Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:101 #13 0x00007f094c7ff8fa in WebCore::SubresourceLoader::didFinishLoading ( this=0x7f08c0018000, finishTime=0) at ../../Source/WebCore/loader/SubresourceLoader.cpp:374 #14 0x00007f094c7fa1b7 in WebCore::ResourceLoader::didFinishLoading ( this=0x7f08c0018000, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:613 #15 0x00007f094cff034a in WebCore::readCallback (asyncResult=0x1f65d20, data=0x7f08c2b77dc0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1340 #16 0x00007f0942de23d6 in async_ready_callback_wrapper ( source_object=0x1f71510, res=0x1f65d20, user_data=0x7f08c2b77dc0) at ginputstream.c:523 #17 0x00007f0942e089b4 in g_task_return_now (task=0x1f65d20) at gtask.c:1077 #18 0x00007f0942e089d9 in complete_in_idle_cb (task=0x1f65d20) at gtask.c:1086 #19 0x00007f09428407ed in g_main_dispatch (context=0x1d6aff0) at gmain.c:3064 #20 g_main_context_dispatch (context=context@entry=0x1d6aff0) at gmain.c:3663 #21 0x00007f09441aee48 in _ecore_glib_select__locked ( ecore_timeout=<optimized out>, efds=0x7ffe219d8b00, wfds=0x7ffe219d8a80, rfds=0x7ffe219d8a00, ecore_fds=<optimized out>, ctx=<optimized out>) at lib/ecore/ecore_glib.c:172 #22 _ecore_glib_select (ecore_fds=<optimized out>, rfds=0x7ffe219d8a00, wfds=0x7ffe219d8a80, efds=0x7ffe219d8b00, ecore_timeout=<optimized out>) at lib/ecore/ecore_glib.c:204 #23 0x00007f09441b2494 in _ecore_main_select (timeout=9.532824124368238e-130) at lib/ecore/ecore_main.c:1459 #24 0x00007f09441b2ed4 in _ecore_main_loop_iterate_internal ( once_only=once_only@entry=0) at lib/ecore/ecore_main.c:1893 #25 0x00007f09441b2fc7 in ecore_main_loop_begin () at lib/ecore/ecore_main.c:983 #26 0x00007f094dc8ee15 in WTF::RunLoop::run () at ../../Source/WTF/wtf/efl/RunLoopEfl.cpp:49 #27 0x00007f094c1044a7 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7ffe219d8f38) at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61 #28 0x00007f094c1040b5 in WebKit::WebProcessMainUnix (argc=2, argv=0x7ffe219d8f38) at ../../Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:161 #29 0x00000000004008ca in main (argc=2, argv=0x7ffe219d8f38) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44
Comment on attachment 259837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259837&action=review > Source/WebCore/ChangeLog:10 > + crash occurs. Any test ?
(In reply to comment #4) > Comment on attachment 259837 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259837&action=review > > > Source/WebCore/ChangeLog:10 > > + crash occurs. > > Any test ? If you visit webpage using @viewport rule with Debug build MiniBrowser Efl, for example, then you will meet the assertion fail, e.g. https://www.wordpress.com.
(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 259837 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=259837&action=review > > > > > Source/WebCore/ChangeLog:10 > > > + crash occurs. > > > > Any test ? > > If you visit webpage using @viewport rule with Debug build MiniBrowser Efl, > for example, then you will meet the assertion fail, > e.g. https://www.wordpress.com. Basic rule is to add a proper test case per a fix, not mention reproduce site.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Comment on attachment 259837 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=259837&action=review > > > > > > > Source/WebCore/ChangeLog:10 > > > > + crash occurs. > > > > > > Any test ? > > > > If you visit webpage using @viewport rule with Debug build MiniBrowser Efl, > > for example, then you will meet the assertion fail, > > e.g. https://www.wordpress.com. > > Basic rule is to add a proper test case per a fix, not mention reproduce > site. Ok, I will add a corresponding test soon. :)
I found there are already tests covering device adaptation in which using @viewport. Below is the result with debug build MiniBrowser EFL before and after this patch is applied. Assertion fails occur when using width and height shorthand. (css3/device-adapt. Total 38 tests) (1) Before this Patch (35 of the tests crash) 3 tests ran as expected, 35 didn't: Regressions: Unexpected crashes (35) css3/device-adapt/opera/cascading-001.xhtml [ Crash ] css3/device-adapt/opera/cascading-002.xhtml [ Crash ] css3/device-adapt/opera/cascading-003.xhtml [ Crash ] css3/device-adapt/opera/cascading-004.xhtml [ Crash ] css3/device-adapt/opera/constrain-001.xhtml [ Crash ] css3/device-adapt/opera/constrain-002.xhtml [ Crash ] css3/device-adapt/opera/constrain-003.xhtml [ Crash ] css3/device-adapt/opera/constrain-004.xhtml [ Crash ] css3/device-adapt/opera/constrain-005.xhtml [ Crash ] css3/device-adapt/opera/constrain-006.xhtml [ Crash ] css3/device-adapt/opera/constrain-007.xhtml [ Crash ] css3/device-adapt/opera/constrain-008.xhtml [ Crash ] css3/device-adapt/opera/constrain-010.xhtml [ Crash ] css3/device-adapt/opera/constrain-011.xhtml [ Crash ] css3/device-adapt/opera/constrain-012.xhtml [ Crash ] css3/device-adapt/opera/constrain-013.xhtml [ Crash ] css3/device-adapt/opera/constrain-014.xhtml [ Crash ] css3/device-adapt/opera/constrain-015.xhtml [ Crash ] css3/device-adapt/opera/constrain-016.xhtml [ Crash ] css3/device-adapt/opera/constrain-017.xhtml [ Crash ] css3/device-adapt/opera/constrain-018.xhtml [ Crash ] css3/device-adapt/opera/constrain-019.xhtml [ Crash ] css3/device-adapt/opera/constrain-020.xhtml [ Crash ] css3/device-adapt/opera/constrain-021.xhtml [ Crash ] css3/device-adapt/opera/constrain-022.xhtml [ Crash ] css3/device-adapt/opera/constrain-024.xhtml [ Crash ] css3/device-adapt/opera/cssom-001.xhtml [ Crash ] css3/device-adapt/opera/orientation-001.xhtml [ Crash ] css3/device-adapt/opera/orientation-002.xhtml [ Crash ] css3/device-adapt/opera/syntax-001.xhtml [ Crash ] css3/device-adapt/opera/syntax-002.xhtml [ Crash ] css3/device-adapt/opera/syntax-003.xhtml [ Crash ] css3/device-adapt/viewport-properties-validation.html [ Crash ] css3/device-adapt/viewport-width-check-window-innerwidth-correct.html [ Crash ] css3/device-adapt/viewport-width-not-affecting-next-page.html [ Crash ] (2) After this Patch (3 of the tests crash) 13 tests ran as expected, 25 didn't: Unexpected flakiness: text-only failures (2) css3/device-adapt/opera/constrain-010.xhtml [ Failure Crash Pass ] css3/device-adapt/opera/constrain-024.xhtml [ Failure Crash Pass ] Regressions: Unexpected text-only failures (20) css3/device-adapt/opera/cascading-002.xhtml [ Failure ] css3/device-adapt/opera/cascading-003.xhtml [ Failure ] css3/device-adapt/opera/cascading-004.xhtml [ Failure ] css3/device-adapt/opera/constrain-001.xhtml [ Failure ] css3/device-adapt/opera/constrain-004.xhtml [ Failure ] css3/device-adapt/opera/constrain-005.xhtml [ Failure ] css3/device-adapt/opera/constrain-008.xhtml [ Failure ] css3/device-adapt/opera/constrain-011.xhtml [ Failure ] css3/device-adapt/opera/constrain-013.xhtml [ Failure ] css3/device-adapt/opera/constrain-014.xhtml [ Failure ] css3/device-adapt/opera/constrain-015.xhtml [ Failure ] css3/device-adapt/opera/constrain-016.xhtml [ Failure ] css3/device-adapt/opera/constrain-017.xhtml [ Failure ] css3/device-adapt/opera/constrain-018.xhtml [ Failure ] css3/device-adapt/opera/constrain-019.xhtml [ Failure ] css3/device-adapt/opera/constrain-020.xhtml [ Failure ] css3/device-adapt/opera/syntax-001.xhtml [ Failure ] css3/device-adapt/opera/syntax-002.xhtml [ Failure ] css3/device-adapt/opera/syntax-003.xhtml [ Failure ] css3/device-adapt/viewport-width-check-window-innerwidth-correct.html [ Failure ] Regressions: Unexpected crashes (3) css3/device-adapt/opera/constrain-006.xhtml [ Crash ] css3/device-adapt/opera/constrain-007.xhtml [ Crash ] css3/device-adapt/viewport-properties-validation.html [ Crash ]
Comment on attachment 259837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259837&action=review As previously discussed, patch needs to include a regression test showing the bug this fixes. > Source/WebCore/css/StylePropertyShorthand.h:98 > + return Vector<StylePropertyShorthand> { widthShorthand() }; Do we need to cite the type here? Does this work? return { widthShorthand() }; > Source/WebCore/css/StylePropertyShorthand.h:106 > + return Vector<StylePropertyShorthand>(); Should just: return { }; > Source/WebCore/css/StylePropertyShorthand.h:109 > +inline Vector<StylePropertyShorthand> matchingCustomShorthandsForLonghand(CSSPropertyID) { return Vector<StylePropertyShorthand>(); } Body should be just be: { return { }; }