Bug 139294

Created attachment 242628 [details] Patch

Created attachment 242632 [details] Patch

Created attachment 242666 [details] Patch

Created attachment 242843 [details] Patch

Created https://bugs.webkit.org/show_bug.cgi?id=139440 and https://bugs.webkit.org/show_bug.cgi?id=139442 for further improvements.

Comment on attachment 242843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242843&action=review > LayoutTests/css3/masking/mask-multiple-values-expected.html:26 > + p > + { > + margin: 0px; > + } > + #back { > + width: 500px; > + height: 500px; > + background-color: green; > + } > + .front > + { > + position: absolute; > + width: 300px; > + height: 300px; > + background-color: black; > + border: 50px solid blue; > + padding: 50px; > + -webkit-mask-repeat: no-repeat; > + -webkit-mask-origin: content-box; > + -webkit-mask-clip: border-box; > + } > + #front1 { Inconsistent brace style. We prefer the opening brace on the same line. > Source/WebCore/css/StyleResolver.cpp:1189 > + while (newMaskLayer && oldMaskLayer) { > + RefPtr<MaskImageOperation> newMaskImage = newMaskLayer->maskImage(); > + RefPtr<MaskImageOperation> oldMaskImage = oldMaskLayer->maskImage(); > + if (newMaskImage.get() && oldMaskImage.get() && (*oldMaskImage == *newMaskImage)) { > + newMaskLayer->setMaskImage(oldMaskImage); > + if (newMaskImage->isExternalDocument()) > + removedExternalResources.append(newMaskImage); > + } else > + break; > + > + newMaskLayer = newMaskLayer->next(); > + oldMaskLayer = oldMaskLayer->next(); > + } > + > + Vector<RefPtr<MaskImageOperation>>& pendingResources = m_state.maskImagesWithPendingSVGDocuments(); > + for (int i = pendingResources.size() - 1; i >= 0; i--) { > + if (removedExternalResources.contains(pendingResources[i])) > + pendingResources.remove(i); > + } This doesn't seem correct if a new mask was inserted early in the list. Seems like you should just consider sets of the old and new images. > Source/WebCore/rendering/RenderLayerMaskImageInfo.cpp:124 > - const FillLayer* maskLayer = m_layer.renderer().style().maskLayers(); > + const FillLayer* maskLayer = oldStyle->maskLayers(); oldStyle should be a reference.

Created attachment 243118 [details] Implemented Simon's suggestions

Landed this in http://trac.webkit.org/changeset/177155

Rolled out in r177169, because the patch caused crashes: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK1%20(Tests)/r177159%20(1259)/results.html

Created attachment 243194 [details] Moved WebKitCSSResourceClass out of the list values section

r=me for that change

Comment on attachment 243194 [details] Moved WebKitCSSResourceClass out of the list values section Clearing flags on attachment: 243194 Committed r177223: <http://trac.webkit.org/changeset/177223>

Test animations/cross-fade-webkit-mask-image.html fails on multiple bots: +CONSOLE MESSAGE: line 238: Unexpected mismatch between CSS Image functions. +CONSOLE MESSAGE: line 238: Unexpected mismatch between CSS Image functions.

Re-opened since this is blocked by bug 139589

Radu is going to mark the test as flaky for now, as this is likely an issue with the test itself.

Created https://bugs.webkit.org/show_bug.cgi?id=139590 for the flaky cross-fade-webkit-mask-image test.

One of the new tests flakily crashes: https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r177237%20(9289)/css3/masking/mask-multiple-values-crash-log.txt Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010616b1fa WTFCrash + 42 (Assertions.cpp:321) 1 com.apple.WebCore 0x000000010b6be019 WebCore::StyleResolver::loadPendingResources() + 153 (StyleResolver.cpp:3759) 2 com.apple.WebCore 0x000000010b6b7e12 WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&, WebCore::Element const*, WebCore::StyleResolver::ShouldUseMatchedPropertiesCache) + 1906 (StyleResolver.cpp:1827) 3 com.apple.WebCore 0x000000010b6b5a53 WebCore::StyleResolver::styleForElement(WebCore::Element*, WebCore::RenderStyle*, WebCore::StyleSharingBehavior, WebCore::RuleMatchingBehavior, WebCore::RenderRegion const*) + 1251 (StyleResolver.cpp:803) 4 com.apple.WebCore 0x000000010b770ab6 WebCore::SVGElement::customStyleForRenderer(WebCore::RenderStyle&) + 150 (SVGElement.cpp:790)

Simon already filed a bug, I'll handle it asap: https://bugs.webkit.org/show_bug.cgi?id=139592

It's also crashing css3/masking/mask-svg-no-fragmentId.html in the same way when I run tests locally. It's already four broken tests from this change, most of them flakily asserting. It doesn't seem OK to keep the patch in the tree, even if root causes are pre-existing - skipping all potentially affected tests is a non-trivial task. I'm going to roll out.

Re-opened since this is blocked by bug 139631

Created attachment 243490 [details] Rebased and prepared for reland The pre-existing issue that caused the rollout has been fixed (https://bugs.webkit.org/show_bug.cgi?id=139644), relanding this.

Comment on attachment 243490 [details] Rebased and prepared for reland Clearing flags on attachment: 243490 Committed r177494: <http://trac.webkit.org/changeset/177494>

This broke data URIs in mask images: -webkit-mask-image: url(data:image/png;base64,...

Filed bug 141857.

This also caused bug 146509.

Also caused bug 146561.

I'm going to roll this out.

Rolled out via bug 146653.

When this gets worked on again, it needs to: 1. not have layering violations 2. not break mime types used to load content 3. not change cross-origin checking

(In reply to comment #29) > When this gets worked on again, it needs to: > 1. not have layering violations > 2. not break mime types used to load content > 3. not change cross-origin checking To sad that I didn't see the warning about rolling it out. The cross-origin checking is needed for mask element references. We didn't use to make it for normal images though. A message earlier said that point 2 was fixed. Where is the layering violation?

(In reply to comment #28) > Rolled out via bug 146653. A really sad step backwards :(

I warned on 7/2. Sorry that I had to roll this out, but the number of late-discovered regressions scared me. The layering violation is platform/graphics/MaskImageOperation.cpp using CachedSVGDocument etc. It's also very weird that all mask image loading goes via CachedSVGDocument.

This also cause security bug 145276. I would add 4. not use raw pointers to Simon's list.

This broke the image zoom on http://bicycleoutfitter.com/product/bontrager-trip-200-195280-1.htm

@Sam: Is there more to do here? Or did we resolve this with future work?

(In reply to Brent Fulgham from comment #35) > @Sam: Is there more to do here? Or did we resolve this with future work? I have no idea! Simon, Said?

<rdar://problem/114683360>

This patch was reverted twice but was eventually landed as 157671@main. But it was reverted again in 164756@main. Applying SVG masks in CSS was reimplemented in 268272@main. See bug 260732. So no more action is needed here. *** This bug has been marked as a duplicate of bug 260732 ***