Full Text Bug Listing

Csaba Osztrogonác

Reported 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]

Csaba Osztrogonác

Comment 1 2012-05-11 00:14:14 PDT

I skipped it on Qt to make the bot green - https://trac.webkit.org/changeset/116734

Alexey Proskuryakov

Comment 2 2012-05-11 10:16:10 PDT

*** Bug 86208 has been marked as a duplicate of this bug. ***

Yi Shen

Comment 3 2012-05-15 13:37:28 PDT

Created attachment 142042 [details] Fix the assertion

WebKit Review Bot

Comment 4 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.

WebKit Review Bot

Comment 5 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

WebKit Review Bot

Comment 6 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

Antti Koivisto

Comment 7 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.

Yi Shen

Comment 8 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.

Yi Shen

Comment 9 2012-05-16 07:39:10 PDT

Created attachment 142258 [details] Retry the chromium test

WebKit Review Bot

Comment 10 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.

Yi Shen

Comment 11 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.

Yi Shen

Comment 12 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 :)

Dave Tharp

Comment 13 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.

Dave Tharp

Comment 14 2012-05-18 15:45:09 PDT

Created attachment 142801 [details] Patch

Dave Tharp

Comment 15 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).

Early Warning System Bot

Comment 16 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

Early Warning System Bot

Comment 17 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

WebKit Review Bot

Comment 18 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

Build Bot

Comment 19 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

Dave Tharp

Comment 20 2012-05-18 16:41:04 PDT

Created attachment 142820 [details] Patch

Dave Tharp

Comment 21 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.

WebKit Review Bot

Comment 22 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.

Build Bot

Comment 23 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

Dave Tharp

Comment 24 2012-05-21 13:14:17 PDT

Created attachment 143085 [details] Patch

Dave Tharp

Comment 25 2012-05-21 13:14:54 PDT

(In reply to comment #24) > Created an attachment (id=143085) [details] > Patch Fixes mac build failure.

WebKit Review Bot

Comment 26 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.

Build Bot

Comment 27 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

Dave Tharp

Comment 28 2012-05-22 16:32:38 PDT

Created attachment 143395 [details] Patch

Dave Tharp

Comment 29 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 :( )

Dave Tharp

Comment 30 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 :)

Jessie Berlin

Comment 31 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.

Zan Dobersek

Comment 32 2012-06-05 01:58:39 PDT

*** Bug 88311 has been marked as a duplicate of this bug. ***

Beth Dakin

Comment 33 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.

Dave Tharp

Comment 34 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 :)

Zan Dobersek

Comment 35 2012-06-13 06:36:53 PDT

*** Bug 86875 has been marked as a duplicate of this bug. ***

Beth Dakin

Comment 36 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.

WebKit Review Bot

Comment 37 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

Tom Zakrajsek

Comment 38 2012-06-26 13:23:43 PDT

Committed r121285: <http://trac.webkit.org/changeset/121285>

Darin Adler

Comment 39 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”.

Dave Tharp

Comment 40 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!

WebKit Review Bot

Comment 41 2012-06-26 14:18:57 PDT

Re-opened since this is blocked by 90014

Csaba Osztrogonác

Comment 42 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

Csaba Osztrogonác

Comment 43 2012-11-21 01:03:23 PST

The bug is still valid. Is anybody working on it?

Ms2ger (he/him; ⌚ UTC+1/+2)

Comment 44 2018-02-21 05:16:21 PST

Appears to have been fixed in bug 109229.

Attachments
Fix the assertion (16.27 KB, patch) 2012-05-15 13:37 PDT, Yi Shen	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-01 (543.11 KB, application/zip) 2012-05-15 17:21 PDT, WebKit Review Bot	no flags	Details
Retry the chromium test (16.27 KB, patch) 2012-05-16 07:39 PDT, Yi Shen	no flags	Details Formatted Diff Diff
Patch (81.74 KB, patch) 2012-05-18 15:45 PDT, Dave Tharp	no flags	Details Formatted Diff Diff
Patch (82.46 KB, patch) 2012-05-18 16:41 PDT, Dave Tharp	no flags	Details Formatted Diff Diff
Patch (83.48 KB, patch) 2012-05-21 13:14 PDT, Dave Tharp	no flags	Details Formatted Diff Diff
Patch (83.62 KB, patch) 2012-05-22 16:32 PDT, Dave Tharp	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Bug 86176