1555030 - Assertion failure: !aNonSVGFrame->IsFrameOfType(nsIFrame::eSVG) || aNonSVGFrame->IsSVGOuterSVGFrame(), at /builds/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:231

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 5cc220ddf028.

Assertion failure: !aNonSVGFrame->IsFrameOfType(nsIFrame::eSVG) || aNonSVGFrame->IsSVGOuterSVGFrame(), at /builds/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:231

rax = 0x00005590da303e40   rdx = 0x0000000000000000
rcx = 0x00007f51c7fc4a4c   rbx = 0x0000000000000001
rsi = 0x00007f51d2ded8b0   rdi = 0x00007f51d2dec680
rbp = 0x00007ffd9348afc0   rsp = 0x00007ffd9348af00
r8 = 0x00007f51d2ded8b0    r9 = 0x00007f51d3f57740
r10 = 0x0000000000000000   r11 = 0x0000000000000000
r12 = 0x00007ffd9348b1b0   r13 = 0x0000000000000000
r14 = 0x00007f51b88dcf88   r15 = 0x0000000000000002
rip = 0x00007f51c4814183
OS|Linux|0.0.0 Linux 4.18.0-17-generic #18~18.04.1-Ubuntu SMP Fri Mar 15 15:27:12 UTC 2019 x86_64
CPU|amd64|family 6 model 94 stepping 3|1
GPU|||
Crash|SIGSEGV|0x0|0
0|0|libxul.so|nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(nsIFrame*, bool)|hg:hg.mozilla.org/mozilla-central:layout/svg/nsSVGIntegrationUtils.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|230|0x4b
0|1|libxul.so|nsSVGUtils::GetBBox(nsIFrame*, unsigned int, mozilla::gfx::BaseMatrix<double> const*)|hg:hg.mozilla.org/mozilla-central:layout/svg/nsSVGUtils.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|1018|0x17
0|2|libxul.so|mozilla::dom::GetTargetSize|hg:hg.mozilla.org/mozilla-central:dom/base/ResizeObserver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|64|0x13
0|3|libxul.so|mozilla::dom::ResizeObservation::IsActive() const|hg:hg.mozilla.org/mozilla-central:dom/base/ResizeObserver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|98|0xd
0|4|libxul.so|mozilla::dom::ResizeObserver::GatherActiveObservations(unsigned int)|hg:hg.mozilla.org/mozilla-central:dom/base/ResizeObserver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|198|0x8
0|5|libxul.so|mozilla::dom::ResizeObserverController::GatherAllActiveObservations(unsigned int)|hg:hg.mozilla.org/mozilla-central:dom/base/ResizeObserverController.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|174|0x18
0|6|libxul.so|mozilla::dom::ResizeObserverController::Notify()|hg:hg.mozilla.org/mozilla-central:dom/base/ResizeObserverController.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|118|0x5
0|7|libxul.so|nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|1900|0xd
0|8|libxul.so|mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|326|0xb
0|9|libxul.so|mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|343|0xf
0|10|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|709|0xf
0|11|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|604|0xf
0|12|libxul.so|mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&)|hg:hg.mozilla.org/mozilla-central:layout/ipc/VsyncChild.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|65|0x8
0|13|libxul.so|mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&)|s3:gecko-generated-sources:2047bdaf2a694e3463df7896963e9b0d74983159d44b6e623cf60f1a09a88c1c366e8276ef8fbe21dbd2274df73c144ea8b86af789141e2b0ade921419f7bde0/ipc/ipdl/PVsyncChild.cpp:|187|0xb
0|14|libxul.so|mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&)|s3:gecko-generated-sources:276632bb583d23c5a3325bf600a0aa917cfb44253339a725992cd0aac949fa0e9951253f93b3309e476b44af0591f67d265a013d57732bb560089dc240fdd03b/ipc/ipdl/PBackgroundChild.cpp:|4717|0x19
0|15|libxul.so|mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|2158|0x6
0|16|libxul.so|mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|2082|0xb
0|17|libxul.so|mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|1939|0xb
0|18|libxul.so|mozilla::ipc::MessageChannel::MessageTask::Run()|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|1970|0xc
0|19|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|1176|0x15
0|20|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|486|0x11
0|21|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|88|0xa
0|22|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5cc220ddf028de011a922042ee9ba691b94d055d|315|0x17
0|23|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5cc220ddf028de011a922042ee9ba691b94d055d|290|0x8
0|24|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|137|0xd
0|25|libxul.so|XRE_RunAppShell()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|911|0x11
0|26|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|238|0x5
0|27|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5cc220ddf028de011a922042ee9ba691b94d055d|315|0x17
0|28|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5cc220ddf028de011a922042ee9ba691b94d055d|290|0x8
0|29|libxul.so|XRE_InitChildProcess(int, char**, XREChildData const*)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|749|0xc
0|30|firefox-bin|content_process_main(mozilla::Bootstrap*, int, char**)|hg:hg.mozilla.org/mozilla-central:ipc/contentproc/plugin-container.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|56|0x14
0|31|firefox-bin|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:5cc220ddf028de011a922042ee9ba691b94d055d|263|0x11
0|32|libc-2.27.so||||0x21b97
0|33|firefox-bin|MOZ_ReportCrash|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:5cc220ddf028de011a922042ee9ba691b94d055d|184|0x5

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This seems ResizeObserver-related.

Comment 2

[Daniel Holbert [:dholbert]]

Boris, mind taking a look? Looks like we may just need a null-check somewhere, or something like that.

Comment 3

[Robert Longson [:longsonr]]

I think we need this:

diff --git a/layout/svg/SVGViewFrame.cpp b/layout/svg/SVGViewFrame.cpp
--- a/layout/svg/SVGViewFrame.cpp
+++ b/layout/svg/SVGViewFrame.cpp
@@ -28,7 +28,7 @@ class SVGViewFrame final : public nsFram
  protected:
   explicit SVGViewFrame(ComputedStyle* aStyle, nsPresContext* aPresContext)
       : nsFrame(aStyle, aPresContext, kClassID) {
-    AddStateBits(NS_FRAME_IS_NONDISPLAY);
+    AddStateBits(NS_FRAME_SVG_LAYOUT | NS_FRAME_IS_NONDISPLAY);
   }

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Also, probably you want to make the check in ResizeObserver NS_FRAME_SVG_LAYOUT anyway, right? To catch outer SVG?

Comment 5

[Boris Chiou [:boris]]

Thanks! Adding NS_FRAME_SVG_LAYOUT into nsSVGViewFrame works because nsSVGUtils::GetBBox() return gfxRect() if it has SVG Layout and cannot be queried as nsSVGDisplayableFrame. SVGOuterSVG frame is inherited from nsSVGDisplayableFrame, so it should be handled properly in GetBBox(). :)

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1555030 - Add NS_FRAME_SVG_LAYOUT into SVGViewFrame and nsSVGStopFrame.

To avoid hit the assertion for ResizeObserver, which calls GetBBox() for all
SVG elements.

Note:
If the target has SVG layout and its primary frame cannot be queries as
nsSVGDisplayableFrame, we return a default gfxRect(). And always
returning a default gfxRect() on <view> element makes ResizeObserver
doesn't report it (because it is always not active). This behavior
is the same as what Google Chrome has. (i.e. No event is fired.)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson [:longsonr] from comment #3)

I think we need this:
[...]

+    AddStateBits(NS_FRAME_SVG_LAYOUT | NS_FRAME_IS_NONDISPLAY);

Hmm. I wonder if it'd be better for the ResizeObserver code to test for NS_FRAME_IS_NONDISPLAY before we call GetBBox?

Because:

1. NS_FRAME_SVG_LAYOUT feels a bit odd for a frame that's not displayed.
2. If we did go this add-the-state-bit route, we have a number of other frames that would need the same treatment: https://searchfox.org/mozilla-central/search?q=AddStateBits%28NS_FRAME_IS_NONDISPLAY%29&path=
   Each of those would presumably cause the same problem right now, with a slightly different fuzzer testcase. Rather than tweaking all those frames' init methods, it seems simpler to just have resizeobserver check the NONDISPLAY bit.

(I initially posted this^^ on Phabricator, but I'm moving it over here since the approach has already been discussed here a bit.)

Comment 8

[Daniel Holbert [:dholbert]]

Or really, perhaps GetBBox() should be the one checking that NONDISPLAY state bit? e.g. as part of this early-return:
https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/layout/svg/nsSVGUtils.cpp#1002-1008

Logically, something like this:

 if (frameIsNondisplay ||  // This is the new line
    (hasSVGLayout && !svg)) {
   return gfxRect();
}

Comment 9

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #8)

Or really, perhaps GetBBox() should be the one checking that NONDISPLAY state bit? e.g. as part of this early-return:
https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/layout/svg/nsSVGUtils.cpp#1002-1008

Ya. I prefer this way. Checking this way with try now.

Comment 10

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #9)

(In reply to Daniel Holbert [:dholbert] from comment #8)

Or really, perhaps GetBBox() should be the one checking that NONDISPLAY state bit? e.g. as part of this early-return:
https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/layout/svg/nsSVGUtils.cpp#1002-1008

Ya. I prefer this way. Checking this way with try now.

OK. Seems this may cause a lot of svg try failures [1]. Perhaps, we should add this into GetTargetSize() to make sure we handle NONDISPLAY only for ResizeObserver.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ea1866e1cde5474de302dfeb3ac34550b0ef13

Comment 11

[Daniel Holbert [:dholbert]]

Interesting. I guess masks (and maybe other things) are nondisplay and yet have nonzero GetBBox return values.

That probably means those things (e.g. masks) also need to work the same way via ResizeObserver (i.e. return nonzero target-sizes, and fire notifications when their size changes).

Maybe that means the original patch here (or something like it) is in fact correct... (I'm not sure)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Are nondisplay frames something that works according to a spec? Last I checked we use them for some things the SVG spec defines using display: none... I don't think that GetBoundingClientRect(), resize observer, and co should work with those.

Comment 13

[Robert Longson [:longsonr]]

nondisplay frames are things in SVG that are not rendered directly. A gradient element for instance can only render indirectly if a rendered element refers to it in its fill or stroke. nondisplay frames are gecko's way of tracking that. All elements that are descendants of a <defs> element become non-rendered.

If there are other SVG frames that are missing the SVG_LAYOUT, state bit add it to them too would be my advice.

Comment 14

[Boris Chiou [:boris]]

Thanks for all your help. I rechecked all non display frames from your list:

layout/svg/nsSVGClipPathFrame.h
layout/svg/nsSVGContainerFrame.cpp
layout/svg/nsSVGFilterFrame.h
layout/svg/nsSVGMarkerFrame.h
layout/svg/nsSVGMaskFrame.h
layout/svg/nsSVGOuterSVGFrame.cpp
layout/svg/nsSVGPaintServerFrame.h

These are kind of nsSVGContainerFrame, so all of these set NS_FRAME_SVG_LAYOUT. That means they don't hit this assertion right now.

layout/svg/nsSVGStopFrame.cpp
layout/svg/SVGViewFrame.cpp

These two don't set NS_FRAME_SVG_LAYOUT, so we got this assertion on <view> element and <stop> element. (Ya, we also need to fix <stop> case).

Besides, I checked the test failures I mentioned in comment 10, and it seems in some cases, for example [1], SVGGeometryFrame (contained in SVGMaskFrame) might set NS_FRAME_NONDISPLAY (Maybe we inherit the bits from mask frame [2]), so returning gfxRect() in this case results in the test failure.

[1] https://searchfox.org/mozilla-central/source/layout/reftests/svg/clipPath-basic-shape-transform.html
[2] https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/layout/generic/nsFrame.cpp#631-634

Based on Robert's suggestion, I will update the patch to add NS_FRAME_SVG_LAYOUT into nsSVGStopFrame as well.

Comment 15

[Daniel Holbert [:dholbert]]

Thank you for looking into that. That sounds good to me.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58a92f5eff76
Add NS_FRAME_SVG_LAYOUT into SVGViewFrame and nsSVGStopFrame. r=dholbert,longsonr

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17106 for changes under testing/web-platform/tests

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/58a92f5eff76

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged