DEBUG Demonstrate new functionality #4

jugglinmike · 2018-08-23T16:58:39Z

No description provided.

chromedriver doesn't allow changing Object.prototype to add enumerable properties, but this test requires setting some values on Object.prototype. When Object.prototype.a is set to: {b: {c: 'on proto'}} chromedriver fails with: JavascriptErrorException: javascript error (500): Maximum call stack size exceeded (Session info: chrome=72.0.3626.121) Remote-end stacktrace: #0 0x563ff3a32a59 <unknown> #1 0x563ff39cb7f3 <unknown> #2 0x563ff38fcd7c <unknown> #3 0x563ff38ff78c <unknown> #4 0x563ff38ff5f7 <unknown> #5 0x563ff38ffbe7 <unknown> #6 0x563ff38fff1b <unknown> #7 0x563ff38a3f7a <unknown> #8 0x563ff3899bf2 <unknown> #9 0x563ff38a37b7 <unknown> #10 0x563ff3899ac3 <unknown> #11 0x563ff38782d2 <unknown> #12 0x563ff3879112 <unknown> #13 0x563ff39fe865 <unknown> web-platform-tests#14 0x563ff39ff32b <unknown> web-platform-tests#15 0x563ff39ff70c <unknown> web-platform-tests#16 0x563ff39d940a <unknown> web-platform-tests#17 0x563ff39ff997 <unknown> web-platform-tests#18 0x563ff39e9947 <unknown> web-platform-tests#19 0x563ff3a1a800 <unknown> web-platform-tests#20 0x563ff3a3c8be <unknown> web-platform-tests#21 0x7f3bf4545494 start_thread web-platform-tests#22 0x7f3bf2d58a8f clone Ran 1 tests finished in 2.0 seconds. • 0 ran as expected. 0 tests skipped. • 1 tests had errors unexpectedly Work around this problem by cleaning up the test environment so Object.prototype no longer has the override by the time chromedriver tries to inspect the test result. While here, fix the other tests to use the t.add_cleanup() function so they'll cleanup their test environment in case they exit in some other way besides reaching t.done(). The underlying chromedriver issue is tracked upstream at https://crbug.com/chromedriver/2555. Bug: 934844 Change-Id: Id1b4ab2a908bfbc001e2a2d045eeec3ef01c24d9

This reverts commit 712c3cf3ed8201420acf23f760eaa34be20781cd. Reason for revert: This patch causes webkit-layout-tests failure on WebKit_Linux_Trusty_ASAN bot: https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20Trusty%20ASAN/25720 Unexpected Failures: * external/wpt/css/css-scroll-snap/scroll-snap-type.html * virtual/threaded/external/wpt/css/css-scroll-snap/scroll-snap-type.html STDERR: ==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200023f8d8 at pc 0x5620c924e56d bp 0x7ffde3c56830 sp 0x7ffde3c56828 STDERR: READ of size 8 at 0x61200023f8d8 thread T0 (content_shell) STDERR: #0 0x5620c924e56c in get ./../../base/memory/scoped_refptr.h:212:27 STDERR: #1 0x5620c924e56c in Style ./../../third_party/blink/renderer/core/layout/layout_object.h:1615:0 STDERR: #2 0x5620c924e56c in GetPhysicalSnapType ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:88:0 STDERR: #3 0x5620c924e56c in blink::SnapCoordinator::UpdateSnapContainerData(blink::LayoutBox&) ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:107:0 STDERR: #4 0x5620c924e74b in blink::SnapCoordinator::UpdateAllSnapContainerData() ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:76:5 Original change's description: > Correctly handle scroll-snap-type changes to 'none' > > > Previously when a scroll container's snap type is changed to 'none' its > data was discarded including all of its snap areas. However this is > incorrect. Because while the snap type is 'none', the element is still > a scroll container which per spec [1] means that is should continue to > captures the snap areas in its subtree for whom it is the nearest > ancestor scroll container . The only difference is that it no longer > snaps. > > The fix is that we no longer remove the snap container data just > because is has a 'none' snap type and instead keep it and its snap > areas. But we check the snap type before performing any snap. > > To ensure this does not introduce any performance regression, this CL > also includes an optimization where we avoid re-calculating > snap_container_data when the snap type is 'none'. So keeping these snap > data should not be cheap. > > Note that there is another problem where if the current snap container > is no longer a scroll container (e.g., overflow: scroll => overflow: > visible) we release its snap areas and they become "orphan". But if we > are to do this correctly, we should re-assign these areas to the next > stroller in the chain. Similarly when an element becomes a scroll > container, it can potentially take over snap areas from its parent snap > container. > > > This patch does not address that situation yet but fixes the easier > problem. > > [1] https://drafts.csswg.org/css-scroll-snap/#overview > > Bug: 953575 > Test: > - wpt/css/css-scroll-snap/scroll-snap-type-change.html => Changing snap-type should work correctly > - wpt/css/css-scroll-snap/scroll-snap-type.html => Add a specific test for type 'none' to ensure it does not snap > > Change-Id: Ie493ad68ecba818ed41c0ee103ccf44725ff6e3f > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589899 > Reviewed-by: Majid Valipour <[email protected]> > Reviewed-by: David Bokan <[email protected]> > Commit-Queue: Majid Valipour <[email protected]> > Cr-Commit-Position: refs/heads/master@{#657460} [email protected],[email protected] Change-Id: I3a327f6e342e95d045194d24ceaf49de52b2b921 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 953575 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600437 Reviewed-by: Takashi Sakamoto <[email protected]> Commit-Queue: Takashi Sakamoto <[email protected]> Cr-Commit-Position: refs/heads/master@{#657571}

…iner height See stack trace below. We set the override container logical height to -1 for the initial layout of a flex item so that we compute the correct size for min-height. However, that messes with our cache for definite heights because we would always set it to indefinite in such a case. Instead, just don't cache these values. That way we will later compute the right thing for resolving flex-basis, etc. (FlexNG can't come soon enough...) #0 blink::LayoutBox::ContainingBlockLogicalHeightForPercentageResolution (this=0x3dda8d434198, out_cb=0x7f6e7d42d8c0, out_skipped_auto_height_containing_block=0x0) at ../../third_party/blink/renderer/core/layout/layout_box.cc:3833 #1 0x00007f6ee84ad0a1 in blink::LayoutFlexibleBox::MainAxisLengthIsDefinite (this=0x3dda8d434010, child=..., flex_basis=Length(0%, Percent), add_to_cb=false) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:762 #2 0x00007f6ee84af930 in blink::LayoutFlexibleBox::MainSizeIsDefiniteForPercentageResolution ( this=0x3dda8d434010, child=...) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1125 #3 0x00007f6ee84ad7f5 in blink::LayoutFlexibleBox::UseOverrideLogicalHeightForPerentageResolution ( this=0x3dda8d434010, child=...) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1137 #4 0x00007f6ee83f2b9d in blink::LayoutBlock::AvailableLogicalHeightForPercentageComputation ( this=0x3dda8d434198) at ../../third_party/blink/renderer/core/layout/layout_block.cc:2333 #5 0x00007f6ee845e745 in blink::LayoutBox::ContainingBlockLogicalHeightForPercentageResolution ( this=0x3dda8d4243d0, out_cb=0x0, out_skipped_auto_height_containing_block=0x0) at ../../third_party/blink/renderer/core/layout/layout_box.cc:3830 #6 0x00007f6ee86dcc5c in blink::LayoutBoxUtils::AvailableLogicalHeight (box=..., cb=0x3dda8d434198) at ../../third_party/blink/renderer/core/layout/ng/layout_box_utils.cc:64 #7 0x00007f6ee86eafea in blink::LayoutNGMixin<blink::LayoutBlockFlow>::ComputeIntrinsicLogicalWidths ( this=0x3dda8d4243d0, min_logical_width=0px, max_logical_width=0px) at ../../third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc:48 #8 0x00007f6ee83ef53a in blink::LayoutBlock::ComputePreferredLogicalWidths (this=0x3dda8d4243d0) at ../../third_party/blink/renderer/core/layout/layout_block.cc:1509 #9 0x00007f6ee8451f01 in blink::LayoutBox::MaxPreferredLogicalWidth (this=0x3dda8d4243d0) at ../../third_party/blink/renderer/core/layout/layout_box.cc:1395 #10 0x00007f6ee84adba2 in blink::LayoutFlexibleBox::ComputeInnerFlexBaseSizeForChild (this=0x3dda8d434198, child=..., main_axis_border_and_padding=0px, child_layout_type=blink::LayoutFlexibleBox::kForceLayout) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:890 #11 0x00007f6ee84ae5d1 in blink::LayoutFlexibleBox::ConstructAndAppendFlexItem (this=0x3dda8d434198, algorithm=0x7f6e7d42ed70, child=..., layout_type=blink::LayoutFlexibleBox::kForceLayout) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:1203 #12 0x00007f6ee84aa27b in blink::LayoutFlexibleBox::LayoutFlexItems (this=0x3dda8d434198, relayout_children=true, layout_scope=...) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:934 #13 0x00007f6ee84a9cff in blink::LayoutFlexibleBox::UpdateBlockLayout (this=0x3dda8d434198, relayout_children=true) at ../../third_party/blink/renderer/core/layout/layout_flexible_box.cc:369 Bug: 1019138 Change-Id: Ie94e69a5f3fe6accc3623d358315b174088d5597 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1902514 Commit-Queue: David Grogan <[email protected]> Auto-Submit: Christian Biesinger <[email protected]> Reviewed-by: David Grogan <[email protected]> Cr-Commit-Position: refs/heads/master@{#713296}

2 tests in this test suite seem inconsistent: test#2 asserts that tbody.height=10px > tr.height=1px > td.height=1px implies td.offsetHeight = 1px test#4 asserts that tbody.height=10px > tr > td.height=1px implies td.offsetHeight = 10px Edge 17 is the only browser that agrees with #2 and #4 FF agrees with #2, but not #4 Chrome agrees with #4, but not #2 Safari agrees with #4, but not #2 To me, #2 and #4 seem to be in conflict. Either tbody height propagates to rows, or it does not. The problem is that #2 is overconstrained. My suggestion is that tbody height always propagates to tr. Bug: 958381 Change-Id: I28bfd108c67968d31d0372b536c316c997d2d958 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586097 Reviewed-by: Ian Kilpatrick <[email protected]> Commit-Queue: Ian Kilpatrick <[email protected]> Cr-Commit-Position: refs/heads/master@{#845515}

…eVisibilityKeeper::PrepareToSplitBlockElement()` before splitting a text node It does the following things when caret is collapsed in a text node in a `<p>` or `<div>` element. 1. Split the text node containing caret to insert `<br>` element 2. Insert `<br>` element after it 3. Split ancestor elements which inclusive descendants of the `<p>` or `<div>` 4. Delete the `<br>` element if unnecessary from the left paragraph #3 and #4 are performed by `HTMLEditor::SplitParagraph()` and it calls `WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement()` correctly before splitting the block. However, in the case (caret is at middle of a text node), the text has already been split to 2 nodes because of #1. Therefore, it fails to handle to keep the white-space visibility. So that I believe that the root cause of this bug is, the method does much complicated things which are required, and doing the redundant things will eat memory space due to undo transactions. However, for now, I'd like to fix this with a simple patch which just call the preparation method before splitting the text node because I'd like to uplift this if it'd be approved (Note that this is not a recent regression, the root cause was created by bug 92686 which was fixed in 17 years ago: <https://searchfox.org/mozilla-central/commit/2e66280faef73e9be218e00758d4eb738395ac83>, but must be annoying bug for users who see this frequently). The new WPTs are pass in Chrome. Differential Revision: https://phabricator.services.mozilla.com/D130950 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1740416 gecko-commit: 73567f6c2bcfa078836a36760498bb11747561dd gecko-reviewers: m_kato, smaug

jugglinmike force-pushed the master branch from 0b44854 to a19a236 Compare August 23, 2018 17:24

jugglinmike force-pushed the taskcluster-stability-2-pr-example branch from 1257652 to 7eff6ed Compare August 23, 2018 17:25

jugglinmike added 3 commits August 23, 2018 13:57

[infra] Validate pull requests in TaskCluster

ca15a0c

[infra] Temporarily ignore stability status in CI

be61008

Enable TaskCluster for this fork of WPT

6ae38c9

jugglinmike force-pushed the master branch from a19a236 to 6ae38c9 Compare August 23, 2018 17:58

DEBUG Demonstrate new functionality

e34172d

jugglinmike force-pushed the taskcluster-stability-2-pr-example branch from 7eff6ed to e34172d Compare August 23, 2018 17:59

jugglinmike mentioned this pull request Aug 23, 2018

Validate pull requests in TaskCluster web-platform-tests/wpt#12657

Merged

jugglinmike force-pushed the master branch from 6ae38c9 to e570d8f Compare August 31, 2018 01:35

jugglinmike force-pushed the master branch from dba6e62 to da7e173 Compare October 16, 2018 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEBUG Demonstrate new functionality #4

DEBUG Demonstrate new functionality #4

jugglinmike commented Aug 23, 2018

DEBUG Demonstrate new functionality #4

Are you sure you want to change the base?

DEBUG Demonstrate new functionality #4

Conversation

jugglinmike commented Aug 23, 2018