Bug 148419

Summary: [CSS DeviceAdaptation] Fix crash which occurs at visiting webpage using @viewport CSS rule when device adaptation is enabled
Product: WebKit Reporter: Joonghun Park <jh718.park>
Component: CSSAssignee: Joonghun Park <jh718.park>
Status: RESOLVED INVALID    
Severity: Normal CC: benjamin, darin, gyuyoung.kim, ossy
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review-

Description Joonghun Park 2015-08-24 22:41:26 PDT 
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.
Comment 1 Joonghun Park 2015-08-25 03:32:15 PDT 
Created attachment 259836 [details]
Patch
Comment 2 Joonghun Park 2015-08-25 03:43:41 PDT 
Created attachment 259837 [details]
Patch
Comment 3 Joonghun Park 2015-08-26 17:15:01 PDT 
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 4 Gyuyoung Kim 2015-08-26 18:53:18 PDT 
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 ?
Comment 5 Joonghun Park 2015-08-26 18:58:08 PDT 
(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.
Comment 6 Gyuyoung Kim 2015-08-26 19:51:09 PDT 
(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.
Comment 7 Joonghun Park 2015-08-26 20:51:11 PDT 
(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. :)
Comment 8 Joonghun Park 2015-08-27 17:04:31 PDT 
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 9 Darin Adler 2015-09-01 10:53:04 PDT 
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 { }; }