768351 - "ABORT: Passed bad frame" with mask pointing at data: URL

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

###!!! ABORT: Passed bad frame!: 'aFrame->IsFrameOfType(nsIFrame::eSVG) && !(aFrame->GetStateBits() & NS_STATE_IS_OUTER_SVG)', file layout/svg/base/src/nsSVGUtils.cpp, line 614

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

We're making the wrong invalidate call for masks applied to outer-<svg>. This fixes that.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Now with crashtests.list change.

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 647231 [details] [diff] [review]
patch

>+  if ((mFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT)) {

One set of brackets is surely enough.

r=longsonr with that change

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #4)
> r=longsonr with that change

Thanks. It's common convention to flag bitwise-and with an extra pair of parenthesis though, and I think some compilers warn if you don't. For that reason I checked it in with the extra parenthesis.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac323ff812e

Comment 6

[Robert Longson [:longsonr]]

The only warn if you use compare to something e.g. if (a & b == c) as that's probably not going to work as intended.

Comment 7

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because the test fails with an assertion:
https://hg.mozilla.org/integration/mozilla-inbound/rev/205889645965

https://tbpl.mozilla.org/php/getParsedLog.php?id=13978494&tree=Mozilla-Inbound

++DOMWINDOW == 87 (0xb271b3c) [serial = 3858] [outer = 0xac582a0]
###!!! ASSERTION: Should not use nsSVGIntegrationUtils on this SVG frame: '!svgChildFrame || (NS_SVGDisplayListPaintingEnabled() && !(aFrame->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD))', file ../../../../../layout/svg/base/src/nsSVGIntegrationUtils.cpp, line 390
std::map<ogg_packet*, long, std::less<ogg_packet*>, std::allocator<std::pair<ogg_packet* const, long> > >::operator[](ogg_packet* const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<nsRefPtr<imgCacheEntry>, std::allocator<nsRefPtr<imgCacheEntry> > >::push_back(nsRefPtr<imgCacheEntry> const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<nsRefPtr<imgCacheEntry>, std::allocator<nsRefPtr<imgCacheEntry> > >::push_back(nsRefPtr<imgCacheEntry> const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<mozilla::gfx::GradientStop, std::allocator<mozilla::gfx::GradientStop> >::push_back(mozilla::gfx::GradientStop const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Sorry, I backed this out

Thanks for backing it out.

Comment 9

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2bc08167c7

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb2bc08167c7

Comment 11

[Jonathan Watt [:jwatt]]

This fixed bug 779403.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 647231 [details] [diff] [review]
patch

[Approval Request Comment]

The real bug we want to fix on aurora is bug 779403, but this bug's patch fixed that bug, so requesting approval on this patch.

Bug caused by (feature/regressing bug #): 738192
User impact if declined: SVG masking is broken
Testing completed (on m-c, etc.): patch has been on m-c for several days
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 13

[Jonathan Watt [:jwatt]]

Err, for "Risk to taking this patch (and alternatives if risky)" I seem to have only processed the "alternatives" part. The risk is very low though, but yeah, there aren't really any viable alternatives.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking and updating the status for 17.

Comment 15

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/74324fb8f642

Comment 16

[Mihaela Velimiroviciu (:mihaelav)]

I have tried this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:16.0) Gecko/20120917 Firefox/16.0 debug build

using the test case provided and there is no assertion, no abord message.

Changing the flag to Verified on Firefox 16.

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0 (latest beta debug buid)

Verified the fix on the above build: there is no assertion/abort message when opening the testcase attached in bug description.