Bug 86176

Summary: ietestcenter/css3/valuesandunits/units-000.htm asserts
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: CSSAssignee: Dave Tharp <dtharp>
Status: RESOLVED WORKSFORME    
Severity: Critical CC: bdakin, dglazkov, dtharp, eric.carlson, feature-media-reviews, jberlin, koivisto, macpherson, max.hong.shen, menard, Ms2ger, ossy, rakuco, tomz, webkit.review.bot, zan
Priority: P1 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 90014    
Bug Blocks: 79668, 85307, 85308    
Attachments:
Description Flags
Fix the assertion
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01
none
Retry the chromium test
none
Patch
none
Patch
none
Patch
none
Patch none

Description Csaba Osztrogonác 2012-05-10 22:56:05 PDT
This test introduced in https://trac.webkit.org/changeset/116658
and asserts from the beginning. Here is the crash log from the Qt bot:
(It asserts on Mac Lion too, but there isn't a crash log on it.)
crash log for DumpRenderTree (pid 16682):
STDOUT: <empty>
STDERR: SHOULD NEVER BE REACHED
STDERR: ../../../../Source/WebCore/css/CSSPrimitiveValue.cpp(505) : double WebCore::CSSPrimitiveValue::computeLengthDouble(WebCore::RenderStyle*, WebCore::RenderStyle*, float, bool)
STDERR: 1   0x7f4832737562 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::CSSPrimitiveValue::computeLengthDouble(WebCore::RenderStyle*, WebCore::RenderStyle*, float, bool)+0x244) [0x7f4832737562]
STDERR: 2   0x7f4832737167 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(unsigned int WebCore::CSSPrimitiveValue::computeLength<unsigned int>(WebCore::RenderStyle*, WebCore::RenderStyle*, float, bool)+0x3f) [0x7f4832737167]
STDERR: 3   0x7f48327ab499 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ApplyPropertyComputeLength<unsigned int, &(WebCore::RenderStyle::borderRightWidth() const), &(WebCore::RenderStyle::setBorderRightWidth(unsigned int)), &(WebCore::RenderStyle::initialBorderWidth()), (WebCore::ComputeLengthNormal)0, (WebCore::ComputeLengthThickness)1, (WebCore::ComputeLengthSVGZoom)0>::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*)+0xd6) [0x7f48327ab499]
STDERR: 4   0x7f48327897de /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::PropertyHandler::applyValue(WebCore::StyleResolver*, WebCore::CSSValue*) const+0x74) [0x7f48327897de]
STDERR: 5   0x7f48327fd321 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::StyleResolver::applyProperty(WebCore::CSSPropertyID, WebCore::CSSValue*)+0x263) [0x7f48327fd321]
STDERR: 6   0x7f4832814b30 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void WebCore::StyleResolver::applyProperties<false>(WebCore::StylePropertySet const*, WebCore::StyleRule*, bool, bool, bool)+0x1e6) [0x7f4832814b30]
STDERR: 7   0x7f483280e29e /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(void WebCore::StyleResolver::applyMatchedProperties<false>(WebCore::StyleResolver::MatchResult const&, bool, int, int, bool)+0x1d8) [0x7f483280e29e]
STDERR: 8   0x7f48327fc5e9 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&)+0x3df) [0x7f48327fc5e9]
STDERR: 9   0x7f48327f6b52 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::StyleResolver::styleForElement(WebCore::Element*, WebCore::RenderStyle*, WebCore::StyleSharingBehavior, WebCore::RuleMatchingBehavior, WebCore::RenderRegion*)+0x462) [0x7f48327f6b52]
STDERR: 10  0x7f48328baa43 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Element::styleForRenderer()+0x83) [0x7f48328baa43]
STDERR: 11  0x7f4832911979 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::NodeRendererFactory::createRendererIfNeeded()+0x149) [0x7f4832911979]
STDERR: 12  0x7f48328f42ca /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Node::createRendererIfNeeded()+0x2e) [0x7f48328f42ca]
STDERR: 13  0x7f48328ba35c /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::Element::attach()+0x2c) [0x7f48328ba35c]
STDERR: 14  0x7f4832b248a5 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(+0x25b48a5) [0x7f4832b248a5]
STDERR: 15  0x7f4832b24bb4 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLConstructionSite::executeQueuedTasks()+0x76) [0x7f4832b24bb4]
STDERR: 16  0x7f4832b4d72f /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&)+0x145) [0x7f4832b4d72f]
STDERR: 17  0x7f4832b4d534 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&)+0x66) [0x7f4832b4d534]
STDERR: 18  0x7f4832b2c9ab /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)+0x327) [0x7f4832b2c9ab]
STDERR: 19  0x7f4832b2c271 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)+0xb1) [0x7f4832b2c271]
STDERR: 20  0x7f4832b2d0cb /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&)+0x14b) [0x7f4832b2d0cb]
STDERR: 21  0x7f48328486dc /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long)+0xca) [0x7f48328486dc]
STDERR: 22  0x7f4832ce7c4a /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentWriter::addData(char const*, unsigned long)+0xe2) [0x7f4832ce7c4a]
STDERR: 23  0x7f4832cda527 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentLoader::commitData(char const*, unsigned long)+0x1e1) [0x7f4832cda527]
STDERR: 24  0x7f483245951d /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::FrameLoaderClientQt::committedLoad(WebCore::DocumentLoader*, char const*, int)+0x43) [0x7f483245951d]
STDERR: 25  0x7f4832cda2e3 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentLoader::commitLoad(char const*, int)+0x97) [0x7f4832cda2e3]
STDERR: 26  0x7f4832cda5d1 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::DocumentLoader::receivedData(char const*, int)+0x51) [0x7f4832cda5d1]
STDERR: 27  0x7f4832d14943 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::MainResourceLoader::addData(char const*, int, bool)+0x51) [0x7f4832d14943]
STDERR: 28  0x7f4832d24649 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool)+0x5b) [0x7f4832d24649]
STDERR: 29  0x7f4832d161b2 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool)+0x1b4) [0x7f4832d161b2]
STDERR: 30  0x7f4832d25134 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)+0x90) [0x7f4832d25134]
STDERR: 31  0x7f48331e0231 /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/WebKitBuild/Debug/lib/libQtWebKit.so.4(WebCore::QNetworkReplyHandler::forwardData()+0x18d) [0x7f48331e0231]
Comment 1 Csaba Osztrogonác 2012-05-11 00:14:14 PDT
I skipped it on Qt to make the bot green - https://trac.webkit.org/changeset/116734
Comment 2 Alexey Proskuryakov 2012-05-11 10:16:10 PDT
*** Bug 86208 has been marked as a duplicate of this bug. ***
Comment 3 Yi Shen 2012-05-15 13:37:28 PDT
Created attachment 142042 [details]
Fix the assertion
Comment 4 WebKit Review Bot 2012-05-15 13:42:08 PDT
Attachment 142042 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:229:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSPrimitiveValue.h:330:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 WebKit Review Bot 2012-05-15 17:21:17 PDT
Comment on attachment 142042 [details]
Fix the assertion

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

New failing tests:
inspector/profiler/heap-snapshot-comparison-expansion-preserved-when-sorting.html
Comment 6 WebKit Review Bot 2012-05-15 17:21:22 PDT
Created attachment 142102 [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
Comment 7 Antti Koivisto 2012-05-16 01:45:04 PDT
Comment on attachment 142042 [details]
Fix the assertion

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:525
> +    else if (m_primitiveUnitType == CSS_VW)
> +        computedValue = getDoubleValue() * renderView->viewportSize().width();
> +    else if (m_primitiveUnitType == CSS_VH)
> +        computedValue = getDoubleValue() * renderView->viewportSize().height();
> +    else if (m_primitiveUnitType == CSS_VMIN)
> +        computedValue = getDoubleValue() * std::min(renderView->viewportSize().height(), renderView->viewportSize().width());

Why is it ok to dereference renderView without checking? It defaults to null.
Comment 8 Yi Shen 2012-05-16 07:21:09 PDT
Thanks for the comment. An assertion for the renderView added above for vw/vh/vmin.

+        case CSS_VW:
+        case CSS_VH:
+        case CSS_VMIN:
+            ASSERT(renderView);
+            factor = 0.01;      
+            break;

(In reply to comment #7)
> (From update of attachment 142042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142042&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:525
> > +    else if (m_primitiveUnitType == CSS_VW)
> > +        computedValue = getDoubleValue() * renderView->viewportSize().width();
> > +    else if (m_primitiveUnitType == CSS_VH)
> > +        computedValue = getDoubleValue() * renderView->viewportSize().height();
> > +    else if (m_primitiveUnitType == CSS_VMIN)
> > +        computedValue = getDoubleValue() * std::min(renderView->viewportSize().height(), renderView->viewportSize().width());
> 
> Why is it ok to dereference renderView without checking? It defaults to null.
Comment 9 Yi Shen 2012-05-16 07:39:10 PDT
Created attachment 142258 [details]
Retry the chromium test
Comment 10 WebKit Review Bot 2012-05-16 07:43:00 PDT
Attachment 142258 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:229:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSPrimitiveValue.h:330:  The parameter name "renderView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Yi Shen 2012-05-17 11:36:19 PDT
Add Dave Tharp's comment here for discussion.

1.  A fundamental question about the spec: it states: "The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the viewport is changed,
they are scaled accordingly."   Your fix seems to reference the viewport,
which seems to be the size of the viewable area of the page.  The spec's wording ("initial containing block") led me to believe the size should be relative to the parent element.  For example in the units-000.htm test, the parent div is 1 inch wide so the "vw" div should be 100% of 1 inch wide.  Your code makes it as wide as the entire viewport.  I am not sure which is correct at this point...  do you have a good argument as to why vw should be based on the viewport and not parent?

2.  A code issue:  I see you put an assert in to check for valid renderView pointer.  This will only catch a NULL pointer in a debug build.
 Later in the code you de-reference renderView without a check.  This is likely to segfault in a non-debug build.
Comment 12 Yi Shen 2012-05-17 12:26:50 PDT
(In reply to comment #11)
> Add Dave Tharp's comment here for discussion.
> 1.  A fundamental question about the spec: it states: "The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the viewport is changed,
> they are scaled accordingly."   Your fix seems to reference the viewport,
> which seems to be the size of the viewable area of the page.  The spec's wording ("initial containing block") led me to believe the size should be relative to the parent element.  For example in the units-000.htm test, the parent div is 1 inch wide so the "vw" div should be 100% of 1 inch wide.  Your code makes it as wide as the entire viewport.  I am not sure which is correct at this point...  do you have a good argument as to why vw should be based on the viewport and not parent?
hmmm. I checked the existing implementation for vh/vw/vmin in WebCore/css/SytleBuilder.cpp and LengthFunctions.cpp, and it seems they all reference the viewport size. But I aggree that the units-000.htm expects the vm/vw/wmin should be based on the parent, rather than the viewport. 

Dave, feel free to upload your patch if it goes the correct solution :) 

> 2.  A code issue:  I see you put an assert in to check for valid renderView pointer.  This will only catch a NULL pointer in a debug build.
>  Later in the code you de-reference renderView without a check.  This is likely to segfault in a non-debug build.
agree, it should return -1.0 when renderview is NUll. Thanks Antti's comments also :)
Comment 13 Dave Tharp 2012-05-17 16:25:09 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > Add Dave Tharp's comment here for discussion.
> > 1.  A fundamental question about the spec: it states: "The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the viewport is changed,
> > they are scaled accordingly."   Your fix seems to reference the viewport,
> > which seems to be the size of the viewable area of the page.  The spec's wording ("initial containing block") led me to believe the size should be relative to the parent element.  For example in the units-000.htm test, the parent div is 1 inch wide so the "vw" div should be 100% of 1 inch wide.  Your code makes it as wide as the entire viewport.  I am not sure which is correct at this point...  do you have a good argument as to why vw should be based on the viewport and not parent?
> hmmm. I checked the existing implementation for vh/vw/vmin in WebCore/css/SytleBuilder.cpp and LengthFunctions.cpp, and it seems they all reference the viewport size. But I aggree that the units-000.htm expects the vm/vw/wmin should be based on the parent, rather than the viewport. 
> 
> Dave, feel free to upload your patch if it goes the correct solution :) 
> 
> > 2.  A code issue:  I see you put an assert in to check for valid renderView pointer.  This will only catch a NULL pointer in a debug build.
> >  Later in the code you de-reference renderView without a check.  This is likely to segfault in a non-debug build.
> agree, it should return -1.0 when renderview is NUll. Thanks Antti's comments also :)

I'm close to finishing my patch... I found a few stragglers where I have to find the parentStyle to pass down.  I'm running out of time for today, but will post my patch tomorrow morning (PDT) for sure.
Comment 14 Dave Tharp 2012-05-18 15:45:09 PDT
Created attachment 142801 [details]
Patch
Comment 15 Dave Tharp 2012-05-18 15:53:43 PDT
(In reply to comment #14)
> Created an attachment (id=142801) [details]
> Patch
Ok, better late than never, right? :)

I am not sure my approach was the right one, so I'm ready for any comments/criticisms from the reviewers. I touched alot of code, but I have tested on several platforms and all seems to be well.  The original assert is handled, and the IETC units tests pass for the vh, vw and vmin cases (as well as the new tests I wrote for each of these specifically).
Comment 16 Early Warning System Bot 2012-05-18 16:05:33 PDT
Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12723786
Comment 17 Early Warning System Bot 2012-05-18 16:09:10 PDT
Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12725632
Comment 18 WebKit Review Bot 2012-05-18 16:11:23 PDT
Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724604
Comment 19 Build Bot 2012-05-18 16:22:53 PDT
Comment on attachment 142801 [details]
Patch

Attachment 142801 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12719977
Comment 20 Dave Tharp 2012-05-18 16:41:04 PDT
Created attachment 142820 [details]
Patch
Comment 21 Dave Tharp 2012-05-18 16:42:46 PDT
(In reply to comment #20)
> Created an attachment (id=142820) [details]
> Patch
Fixed new instance of usage of computeLength() to include parentStyle pointer.
Comment 22 WebKit Review Bot 2012-05-18 16:43:44 PDT
Attachment 142820 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:99:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. animations/animation-api-1.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3762:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/compiler-script-mapping.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3764:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/cross-frame-access-put.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3766:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. compositing/geometry/object-clip-rects-assertion.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3767:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/appcache/appcache-iframe-manifests.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3768:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/network/async-xhr-json-mime-type.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3769:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/network/network-cachedresources-with-same-urls.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3770:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/inspector/network/network-content-replacement-xhr.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3771:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/escape-form-data-field-names.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3772:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/feed-urls-from-remote.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3773:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. http/tests/security/referrer-policy-redirect-link.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3774:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. platform/chromium/virtual/gpu/fast/canvas/imagedata-contains-uint8clampedarray.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:3775:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. platform/chromium/virtual/gpu/fast/canvas/webgl/webgl-composite-modes-repaint.html  [test/expectations] [5]
Total errors found: 13 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Build Bot 2012-05-18 17:26:50 PDT
Comment on attachment 142820 [details]
Patch

Attachment 142820 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12719995
Comment 24 Dave Tharp 2012-05-21 13:14:17 PDT
Created attachment 143085 [details]
Patch
Comment 25 Dave Tharp 2012-05-21 13:14:54 PDT
(In reply to comment #24)
> Created an attachment (id=143085) [details]
> Patch
Fixes mac build failure.
Comment 26 WebKit Review Bot 2012-05-21 13:17:03 PDT
Attachment 143085 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/efl/test_expectations.txt:6:  fast/dom/shadow/content-element-api.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:7:  fast/dom/shadow/content-element-outside-shadow.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:38:  editing/pasteboard/drag-files-to-editable-element.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:42:  fast/events/drop-handler-should-not-stop-navigate.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:43:  fast/events/drop-with-file-paths.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:44:  fast/files/file-reader-directory-crash.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:119:  fast/events/show-modal-dialog-onblur-onfocus.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:120:  fast/harness/show-modal-dialog.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/efl/test_expectations.txt:286:  fast/dom/title-directionality.html is also in a Skipped file.  [test/expectations] [5]
WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist
WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist
Total errors found: 9 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Build Bot 2012-05-21 14:04:32 PDT
Comment on attachment 143085 [details]
Patch

Attachment 143085 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12731745
Comment 28 Dave Tharp 2012-05-22 16:32:38 PDT
Created attachment 143395 [details]
Patch
Comment 29 Dave Tharp 2012-05-22 16:34:35 PDT
(In reply to comment #28)
> Created an attachment (id=143395) [details]
> Patch
Re-baselined + Yet another attempt to solve the mac build issue. (I don't have a Mac to test on yet :(   )
Comment 30 Dave Tharp 2012-05-24 10:41:55 PDT
Notes on Bug Fix:
- The assert was caused by vw, vh, and vmin not being handled in CSSPrimitiveValue::computeLength().
- Could have fixed the assert by stubbing the handling of vw, vh and vmin, but both Yi Shen and myself decided to go ahead and implement these units correctly in CSSPrimitiveValue::computeLength().
- There is some debate on what "correctly" is in this context :)  Yi's approach bases vw, vh, and vmin on the viewport size.  My approach bases these units on the size of the containing parent. The spec ( http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths ) says : "The viewport-percentage lengths are relative to the size of the initial containing block". Also, the test that highlights this issue ( http://samples.msdn.microsoft.com/ietestcenter/css3/css_harness.htm?type=values&url=units-000 ) appears to be  written to expect the vw, vh and vmin uints to be relative to the parent.
- I ended up having to push parentStyle down into CSSPrimitiveValue::computeLength() (added a method argument). This ended up blooming out and touching more code than I would have hoped for.  If there is a better way to get parentStyle in CSSPrimitiveValue::computeLength(), I am listening :)
Comment 31 Jessie Berlin 2012-05-24 18:43:09 PDT
(In reply to comment #30)
> Notes on Bug Fix:
> - The assert was caused by vw, vh, and vmin not being handled in CSSPrimitiveValue::computeLength().
> - Could have fixed the assert by stubbing the handling of vw, vh and vmin, but both Yi Shen and myself decided to go ahead and implement these units correctly in CSSPrimitiveValue::computeLength().
> - There is some debate on what "correctly" is in this context :)  Yi's approach bases vw, vh, and vmin on the viewport size.  My approach bases these units on the size of the containing parent. The spec ( http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths ) says : "The viewport-percentage lengths are relative to the size of the initial containing block". Also, the test that highlights this issue ( http://samples.msdn.microsoft.com/ietestcenter/css3/css_harness.htm?type=values&url=units-000 ) appears to be  written to expect the vw, vh and vmin uints to be relative to the parent.
> - I ended up having to push parentStyle down into CSSPrimitiveValue::computeLength() (added a method argument). This ended up blooming out and touching more code than I would have hoped for.  If there is a better way to get parentStyle in CSSPrimitiveValue::computeLength(), I am listening :)

Note: this test also asserts on Windows. I have added it to the Skipped list for now (see http://trac.webkit.org/changeset/118458), but please take it off the Skipped list when you fix this bug.
Comment 32 Zan Dobersek 2012-06-05 01:58:39 PDT
*** Bug 88311 has been marked as a duplicate of this bug. ***
Comment 33 Beth Dakin 2012-06-12 12:06:10 PDT
Here are a few thoughts.

It is a bit of a shame to pass around that extra parentStyle pointer so much. But I suppose there is already a lot of that. We could consider passing an IntSize instead to represent the parentSize. I'm not totally sure that's worth it though because then the call-sites will be messier, and we'll be incurring the overhead of creating that object in a bunch of cases where it's not needed.

I do have a higher-level question though. I'm still a little confused reading through this bug about why we're adding support for these units. Does supporting them fix the assertion? Do we want to support them because we think they are valuable? Or supported in other browsers? Just generally wondering how to assess the value of something that also creates some overhead with all the RenderStyle passing.

Antti would be a good person to weigh in here, too.
Comment 34 Dave Tharp 2012-06-12 13:36:15 PDT
(In reply to comment #33)
> Here are a few thoughts.
> 
> It is a bit of a shame to pass around that extra parentStyle pointer so much. But I suppose there is already a lot of that. We could consider passing an IntSize instead to represent the parentSize. I'm not totally sure that's worth it though because then the call-sites will be messier, and we'll be incurring the overhead of creating that object in a bunch of cases where it's not needed.
> 
> I do have a higher-level question though. I'm still a little confused reading through this bug about why we're adding support for these units. Does supporting them fix the assertion? Do we want to support them because we think they are valuable? Or supported in other browsers? Just generally wondering how to assess the value of something that also creates some overhead with all the RenderStyle passing.
> 
> Antti would be a good person to weigh in here, too.

It appears that the only other browser that supports these units is IE10. The reason I decided to attempt an implementation is that running this test : http://samples.msdn.microsoft.com/ietestcenter/css3/valuesandunits/units-000.htm causes an assertion in debug builds. In general I hate when html causes code to break :)  Yes, I could have just worked around the actual assertion, but the test itself would remain in failure state, so implementing the units seemed to be the right thing to do. So do directly answer your question: Yes, adding support for these units does fix the assertion.  But, I could fix the assertion by simply stubbing the case for the units out... but that seemed like a weak fix (yi shen took the same approach, but ended up with a different solution)
I agree that the overall value of adding support for these units is debatable, given the amount of code I had to touch.  I don't think the use of them in the wild is high. However, they are useful in my opinion.  I guess that value judgement is up to you reviewers :)
Regarding Antii, I did try to get him to look at this when I first posted it. His response was something like "this is a HUGE patch. I'm not going to look at it now".  I didn't want to ask him if that meant he queued it or he was just punting it...  so I waited a few days to see if he picked it up. So after a few days I looked for another reviewer :)
Comment 35 Zan Dobersek 2012-06-13 06:36:53 PDT
*** Bug 86875 has been marked as a duplicate of this bug. ***
Comment 36 Beth Dakin 2012-06-20 16:21:49 PDT
Comment on attachment 143395 [details]
Patch

Seems like these units would be nice to have, so I am going to go ahead and r+. The added complexity is not ideal, but there is no obvious way around that.
Comment 37 WebKit Review Bot 2012-06-25 11:51:36 PDT
Comment on attachment 143395 [details]
Patch

Rejecting attachment 143395 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
fl/TestExpectations.rej
patching file LayoutTests/platform/mac/Skipped
Hunk #1 FAILED at 834.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/Skipped.rej
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 2428.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Beth Dakin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13094201
Comment 38 Tom Zakrajsek 2012-06-26 13:23:43 PDT
Committed r121285: <http://trac.webkit.org/changeset/121285>
Comment 39 Darin Adler 2012-06-26 13:28:54 PDT
Comment on attachment 143395 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        lengths are "relative to the size of the initial containing block", which I read to be
> +        the size of the parent element. Since parentStyle was not available in computeLengthDouble,

But “size of the initial containing block” is not the same thing as “size of the parent element”. The definition of initial containing block is here <http://www.w3.org/TR/CSS21/visudet.html#containing-block-details>. It’s the “size of the containing block in which the root element lives”.
Comment 40 Dave Tharp 2012-06-26 14:12:58 PDT
(In reply to comment #39)
> (From update of attachment 143395 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143395&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        lengths are "relative to the size of the initial containing block", which I read to be
> > +        the size of the parent element. Since parentStyle was not available in computeLengthDouble,
> 
> But “size of the initial containing block” is not the same thing as “size of the parent element”. The definition of initial containing block is here <http://www.w3.org/TR/CSS21/visudet.html#containing-block-details>. It’s the “size of the containing block in which the root element lives”.

Ah.  Ok, rolling the patch out and will re-visit the implementation. Thanks!
Comment 41 WebKit Review Bot 2012-06-26 14:18:57 PDT
Re-opened since this is blocked by 90014
Comment 42 Csaba Osztrogonác 2012-07-31 03:05:55 PDT
Comment on attachment 143395 [details]
Patch

It is obsoloete, because it was rolled out by http://trac.webkit.org/changeset/121289
Comment 43 Csaba Osztrogonác 2012-11-21 01:03:23 PST
The bug is still valid. Is anybody working on it?
Comment 44 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-02-21 05:16:21 PST
Appears to have been fixed in bug 109229.