|
Description
Simon Fraser (smfr)
2019-05-03 09:23:18 PDT
Created attachment 369080 [details]
Patch
*** Bug 197594 has been marked as a duplicate of this bug. *** Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12102569 New failing tests: media/media-continues-playing-after-replace-source.html media/media-ended.html Created attachment 369082 [details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12102575 New failing tests: media/media-continues-playing-after-replace-source.html media/media-ended.html Created attachment 369083 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12102571 Number of test failures exceeded the failure limit. Created attachment 369085 [details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12102657 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html Created attachment 369087 [details]
Archive of layout-test-results from ews214 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369080 [details] Patch Attachment 369080 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12102636 New failing tests: fast/scrolling/ios/overflow-scroll-overlap-4.html media/media-continues-playing-after-replace-source.html media/track/track-cues-enter-exit.html media/media-ended.html media/track/track-cues-cuechange.html Created attachment 369088 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369080 [details]
Patch
Crashes are because traverseUnchangedSubtree() needs to do sharing stuff.
Comment on attachment 369080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369080&action=review > Source/WebCore/rendering/RenderLayer.cpp:1817 > + RenderLayer* repaintTarget; > + if (includeSelf == IncludeSelf && (repaintTarget = repaintTargetForLayer(*this))) > + return repaintTarget; No reason to leave repaintTarget uninitialized. > Source/WebCore/rendering/RenderLayer.h:818 > + // If non,null, a non-ancestor composited layer that this layer paints into (it is sharing its backing store with this layer). "non-null" > Source/WebCore/rendering/RenderLayer.h:822 > + RenderLayer* sharedLayer() const { return m_sharedLayer; } > + void setSharedLayer(RenderLayer* sharedLayer) { m_sharedLayer = sharedLayer; } > + > + bool paintsIntoSharedLayer() const { return m_sharedLayer; } "Shared layer" does not really describe the feature. I feel it would be clearer to express this in terms of "shared backing" in code. This might be just a matter of renaming and probably also switching to RenderLayerBacking ptrs here. I wonder if there some sort of future refactoring here that actually makes RenderLayerBacking a shared object? > Source/WebCore/rendering/RenderLayer.h:1268 > + // If non,null, a non-ancestor composited layer that this layer paints into. "non-null" > Source/WebCore/rendering/RenderLayerBacking.cpp:264 > + for (auto* layer : m_sharingLayers) > + layer->setSharedLayer(nullptr); This kind of stuff is scary. Layer tree should use WeakPtrs. It would be better to call clearSharingLayers() to avoid duplication. > Source/WebCore/rendering/RenderLayerBacking.cpp:2678 > + for (auto* layer : m_sharingLayers) > + paintOneLayer(*layer, sharingLayerPaintFlags); I suppose region gathering code needs to replicate this logic too. Please file a bug. > Source/WebCore/rendering/RenderLayerBacking.h:398 > + // When this layer is a shared layer, sharing layers other than m_owningLayer are in this list, and paint into our backing store. This is a confusing sentence. (In reply to Antti Koivisto from comment #15) > Comment on attachment 369080 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369080&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:1817 > > + RenderLayer* repaintTarget; > > + if (includeSelf == IncludeSelf && (repaintTarget = repaintTargetForLayer(*this))) > > + return repaintTarget; > > No reason to leave repaintTarget uninitialized. > > > Source/WebCore/rendering/RenderLayer.h:818 > > + // If non,null, a non-ancestor composited layer that this layer paints into (it is sharing its backing store with this layer). > > "non-null" > > > Source/WebCore/rendering/RenderLayer.h:822 > > + RenderLayer* sharedLayer() const { return m_sharedLayer; } > > + void setSharedLayer(RenderLayer* sharedLayer) { m_sharedLayer = sharedLayer; } > > + > > + bool paintsIntoSharedLayer() const { return m_sharedLayer; } > > "Shared layer" does not really describe the feature. I feel it would be > clearer to express this in terms of "shared backing" in code. This might be > just a matter of renaming and probably also switching to RenderLayerBacking > ptrs here. I agree that "shared" isn't too descriptive. Other implementations have called them "inclusive" which has clearer directionality ("inclusive, included") but Zalan didn't like that. I don't want to use RLB pointers because I don't want to introduce a dependency on when backing actually gets created. > I wonder if there some sort of future refactoring here that actually makes > RenderLayerBacking a shared object? Perhaps, though there is still a clear "primary" owner. > > Source/WebCore/rendering/RenderLayer.h:1268 > > + // If non,null, a non-ancestor composited layer that this layer paints into. > > "non-null" > > > Source/WebCore/rendering/RenderLayerBacking.cpp:264 > > + for (auto* layer : m_sharingLayers) > > + layer->setSharedLayer(nullptr); > > This kind of stuff is scary. Layer tree should use WeakPtrs. Will fix. > It would be better to call clearSharingLayers() to avoid duplication. > > > Source/WebCore/rendering/RenderLayerBacking.cpp:2678 > > + for (auto* layer : m_sharingLayers) > > + paintOneLayer(*layer, sharingLayerPaintFlags); > > I suppose region gathering code needs to replicate this logic too. Please > file a bug. Good point. > > Source/WebCore/rendering/RenderLayerBacking.h:398 > > + // When this layer is a shared layer, sharing layers other than m_owningLayer are in this list, and paint into our backing store. > > This is a confusing sentence. Will fix. Comment on attachment 369080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369080&action=review > Source/WebCore/rendering/RenderLayer.cpp:4082 > + paintFlags.add(PaintLayerTemporaryClipRects); This disables clip caching right? Should we worry about performance? Created attachment 369134 [details]
Patch
Created attachment 369231 [details]
Patch
Created attachment 369232 [details]
Patch
Created attachment 369234 [details]
Patch
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12120290 New failing tests: compositing/shared-layers/partial-compositing-update.html media/modern-media-controls/volume-support/volume-support-drag-to-mute.html media/modern-media-controls/volume-support/volume-support-drag.html media/modern-media-controls/scrubber-support/scrubber-support-click.html media/modern-media-controls/fullscreen-support/fullscreen-support-press.html compositing/shared-layers/partial-compositing-update2.html media/modern-media-controls/playback-support/playback-support-button-click.html media/audio-concurrent-supported.html media/modern-media-controls/placard-support/placard-support-pip.html media/modern-media-controls/mute-support/mute-support-press-on-button.html media/modern-media-controls/scrubber-support/scrubber-support-drag.html media/modern-media-controls/volume-support/volume-support-click.html Created attachment 369243 [details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12120307 Number of test failures exceeded the failure limit. Created attachment 369245 [details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12120358 New failing tests: compositing/shared-layers/partial-compositing-update.html media/modern-media-controls/volume-support/volume-support-drag-to-mute.html media/modern-media-controls/volume-support/volume-support-drag.html media/audio-concurrent-supported.html media/modern-media-controls/fullscreen-support/fullscreen-support-press.html media/audio-mpeg-supported.html compositing/shared-layers/partial-compositing-update2.html media/modern-media-controls/playback-support/playback-support-button-click.html media/modern-media-controls/scrubber-support/scrubber-support-click.html media/media-ended.html media/media-continues-playing-after-replace-source.html media/modern-media-controls/mute-support/mute-support-press-on-button.html media/modern-media-controls/scrubber-support/scrubber-support-drag.html media/modern-media-controls/volume-support/volume-support-click.html media/media-ended-fired-once.html Created attachment 369246 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12120398 New failing tests: compositing/shared-layers/partial-compositing-update.html media/audio-mpeg4-supported.html fast/scrolling/ios/overflow-scroll-overlap-4.html media/audio-concurrent-supported.html media/audio-playback-restriction-removed-muted.html compositing/shared-layers/partial-compositing-update2.html compositing/shared-layers/overflow-scroll/absolute-in-stacking-relative-in-scroller.html media/media-continues-playing-after-replace-source.html media/media-ended-fired-once.html Created attachment 369251 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369234 [details] Patch Attachment 369234 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12123996 New failing tests: svg/repaint/remove-border-property-on-root.html Created attachment 369294 [details]
Archive of layout-test-results from ews210 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369301 [details]
Patch
Comment on attachment 369301 [details] Patch Attachment 369301 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12125821 New failing tests: fast/scrolling/ios/overflow-scroll-overlap-4.html compositing/shared-layers/partial-compositing-update.html compositing/shared-layers/overflow-scroll/absolute-in-stacking-relative-in-scroller.html compositing/shared-layers/partial-compositing-update2.html Created attachment 369316 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369301 [details] Patch Attachment 369301 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12125836 New failing tests: compositing/shared-layers/partial-compositing-update2.html compositing/shared-layers/partial-compositing-update.html media/modern-media-controls/volume-support/volume-support-drag-to-mute.html media/modern-media-controls/volume-support/volume-support-drag.html media/modern-media-controls/scrubber-support/scrubber-support-click.html media/track/track-cue-rendering-snap-to-lines-not-set.html fast/hidpi/video-controls-in-hidpi.html media/modern-media-controls/playback-support/playback-support-button-click.html media/video-zoom-controls.html media/modern-media-controls/scrubber-support/scrubber-support-media-api.html media/track/track-cue-rendering-vertical.html media/modern-media-controls/mute-support/mute-support-press-on-button.html media/modern-media-controls/scrubber-support/scrubber-support-drag.html media/modern-media-controls/volume-support/volume-support-click.html media/video-trackmenu-selection.html Created attachment 369320 [details]
Archive of layout-test-results from ews115 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369355 [details]
Patch
Created attachment 369357 [details]
Patch
Comment on attachment 369357 [details] Patch Attachment 369357 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12130330 New failing tests: fast/hidpi/video-controls-in-hidpi.html Created attachment 369359 [details]
Archive of layout-test-results from ews112 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 369357 [details] Patch Attachment 369357 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12130361 New failing tests: fast/scrolling/ios/overflow-scroll-overlap-4.html Created attachment 369360 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
> New failing tests:
> fast/scrolling/ios/overflow-scroll-overlap-4.html
This is failing because of the missing region code.
Comment on attachment 369357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369357&action=review Looks fine. Fixing updateEventRegion shouldn't be difficult either (same thing as paintIntoLayer) but it can be done separately if you prefer. > Source/WebCore/rendering/RenderLayer.cpp:1774 > +// ASSERT_IMPLIES(backingProvider, !isComposited()); Commented-out code. > Source/WebCore/rendering/RenderLayerBacking.cpp:679 > + if (!layerWeakPtr) { > + ASSERT_NOT_REACHED(); > + continue; > + } This seems overly defensive. We should just crash with nullptr if the logic is supposed to guarantee that these are non-null (here and elsewhere). Comment on attachment 369357 [details] Patch Attachment 369357 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12131207 New failing tests: http/tests/css/filters-on-iframes.html Created attachment 369364 [details]
Archive of layout-test-results from ews210 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
> webkit.org/b/197695 [ Debug ] fast/hidpi/video-controls-in-hidpi.html [ Skip ]
This patch skipped an existing test. How is this OK?
Reverted r245058 for reason: Causes crashes under ASan / GuardMalloc Committed r245085: <https://trac.webkit.org/changeset/245085> Re-landed with fixes in https://trac.webkit.org/changeset/245170/webkit This hits debug asserts on many sites (say, nytimes.com)
void RenderLayer::paintLayerContents(GraphicsContext& context, const LayerPaintingInfo& paintingInfo, OptionSet<PaintLayerFlag> paintFlags)
{
ASSERT(isSelfPaintingLayer() || hasSelfPaintingLayerDescendant());
(lldb) bt 10
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
* frame #0: 0x000000035a69285e JavaScriptCore`::WTFCrash() at Assertions.cpp:305:35
frame #1: 0x000000035ee1ce2b WebCore`WTFCrashWithInfo((null)=4368, (null)="rendering/RenderLayer.cpp", (null)="void WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext &, const WebCore::RenderLayer::LayerPaintingInfo &, OptionSet<WebCore::RenderLayer::PaintLayerFlag>)", (null)=2340) at Assertions.h:568:5
frame #2: 0x00000003622b75f6 WebCore`WebCore::RenderLayer::paintLayerContents(this=0x00000003946d8c18, context=0x00007ffee42dc358, paintingInfo=0x00007ffee42dbde0, paintFlags=(m_storage = 96)) at RenderLayer.cpp:4368:5
frame #3: 0x00000003622d81ac WebCore`WebCore::RenderLayerBacking::paintIntoLayer(this=0x00007ffee42dbec8, layer=0x00000003946d8c18, paintFlags=(m_storage = 96))::$_9::operator()(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) const at RenderLayerBacking.cpp:2674:15
frame #4: 0x00000003622d7f70 WebCore`WebCore::RenderLayerBacking::paintIntoLayer(this=0x000000039460fa20, graphicsLayer=0x000000039473d000, context=0x00007ffee42dc358, paintDirtyRect=0x00007ffee42dc0d8, paintBehavior=(m_storage = 2048), paintingPhase='\x03') at RenderLayerBacking.cpp:2701:13
frame #5: 0x00000003622d869d WebCore`WebCore::RenderLayerBacking::paintContents(this=0x000000039460fa20, graphicsLayer=0x000000039473d000, context=0x00007ffee42dc358, paintingPhase='\x03', clip=0x00007ffee42dc160, layerPaintBehavior=2) at RenderLayerBacking.cpp:2744:9
frame #6: 0x0000000361de23b7 WebCore`WebCore::GraphicsLayer::paintGraphicsLayerContents(this=0x000000039473d000, context=0x00007ffee42dc358, clip=0x00000003947f49b8, layerPaintBehavior=2) at GraphicsLayer.cpp:501:14
frame #7: 0x0000000361e5a8ad WebCore`WebCore::GraphicsLayerCA::platformCALayerPaintContents(this=0x000000039473d000, (null)=0x0000000392633000, context=0x00007ffee42dc358, clip=0x00000003947f49b8, layerPaintBehavior=2) at GraphicsLayerCA.cpp:1678:5
frame #8: 0x000000035f4df4c3 WebCore`WebCore::PlatformCALayer::drawLayerContents(context=0x00007fc3dced2ba0, platformCALayer=0x0000000392633000, dirtyRects=0x00000003947f49a8, layerPaintBehavior=2) at PlatformCALayerCocoa.mm:1198:28
frame #9: 0x000000010c5056f0 WebKit`WebKit::RemoteLayerBackingStore::drawInContext(this=0x00000003947f4958, context=0x000000039ff97840, backImage=0x0000000000000000) at RemoteLayerBackingStore.mm:355:9
Filed bug 197776 on that. [885/2246] Building CXX object Source/WebCore/CMakeFiles/...es/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp:5:
../../Source/WebCore/rendering/RenderLayer.cpp: In member function ‘WTF::Ref<WebCore::ClipRects> WebCore::RenderLayer::updateClipRects(const WebCore::RenderLayer::ClipRectsContext&)’:
../../Source/WebCore/rendering/RenderLayer.cpp:5503:15: warning: unused variable ‘parentLayer’ [-Wunused-variable]
5503 | if (auto* parentLayer = (clipRectsContext.rootLayer != this ? parent() : nullptr))
| ^~~~~~~~~~~
This condition here:
RefPtr<ClipRects> parentClipRects;
// For transformed layers, the root layer was shifted to be us, so there is no need to
// examine the parent. We want to cache clip rects with us as the root.
if (auto* parentLayer = (clipRectsContext.rootLayer != this ? parent() : nullptr))
parentClipRects = this->parentClipRects(clipRectsContext);
Should now be simplified to:
RefPtr<ClipRects> parentClipRects;
// For transformed layers, the root layer was shifted to be us, so there is no need to
// examine the parent. We want to cache clip rects with us as the root.
if (clipRectsContext.rootLayer != this)
parentClipRects = this->parentClipRects(clipRectsContext);
Er no, that's a disaster think-o, sorry. Should be simplified to: if (clipRectsContext.rootLayer != this && parent()) Will fix in: bug #197785. (Assuming the new logic is correct and only needs to be simplified.) |