Summary: | ietestcenter/css3/valuesandunits/units-000.htm asserts | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||||||||
Component: | CSS | Assignee: | 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
Csaba Osztrogonác
2012-05-10 22:56:05 PDT
I skipped it on Qt to make the bot green - https://trac.webkit.org/changeset/116734 *** Bug 86208 has been marked as a duplicate of this bug. *** Created attachment 142042 [details]
Fix the assertion
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 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 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 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. 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. Created attachment 142258 [details]
Retry the chromium test
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.
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. (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 :) (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. Created attachment 142801 [details]
Patch
(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 on attachment 142801 [details] Patch Attachment 142801 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12723786 Comment on attachment 142801 [details] Patch Attachment 142801 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12725632 Comment on attachment 142801 [details] Patch Attachment 142801 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12724604 Comment on attachment 142801 [details] Patch Attachment 142801 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12719977 Created attachment 142820 [details]
Patch
(In reply to comment #20) > Created an attachment (id=142820) [details] > Patch Fixed new instance of usage of computeLength() to include parentStyle pointer. 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 on attachment 142820 [details] Patch Attachment 142820 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12719995 Created attachment 143085 [details]
Patch
(In reply to comment #24) > Created an attachment (id=143085) [details] > Patch Fixes mac build failure. 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 on attachment 143085 [details] Patch Attachment 143085 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12731745 Created attachment 143395 [details]
Patch
(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 :( ) 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 :) (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. *** Bug 88311 has been marked as a duplicate of this bug. *** 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. (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 :) *** Bug 86875 has been marked as a duplicate of this bug. *** 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 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 Committed r121285: <http://trac.webkit.org/changeset/121285> 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”. (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! Re-opened since this is blocked by 90014 Comment on attachment 143395 [details] Patch It is obsoloete, because it was rolled out by http://trac.webkit.org/changeset/121289 The bug is still valid. Is anybody working on it? Appears to have been fixed in bug 109229. |