Closed Bug 1555030 Opened 3 months ago Closed 3 months ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- disabled
firefox69 --- fixed

People

(Reporter: jkratzer, Assigned: boris)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file 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
Flags: in-testsuite?

This seems ResizeObserver-related.

Blocks: 1547642

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

Flags: needinfo?(boris.chiou)
Assignee: nobody → boris.chiou
Flags: needinfo?(boris.chiou)

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);
   }

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

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(). :)

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.)

(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.)

Flags: needinfo?(longsonr)

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();
}

(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.

(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

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)

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.

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.

Flags: needinfo?(longsonr)

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.

Attachment #9068197 - Attachment description: Bug 1555030 - Add NS_FRAME_SVG_LAYOUT into SVGViewFrame. → Bug 1555030 - Add NS_FRAME_SVG_LAYOUT into SVGViewFrame and nsSVGStopFrame.

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

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58a92f5eff76
Add NS_FRAME_SVG_LAYOUT into SVGViewFrame and nsSVGStopFrame. r=dholbert,longsonr
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17106 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Upstream PR merged
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.