1308793 - Crash when setting display:[inline-]{flex,grid} or columns properties on a SVG <text> element

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

(From Bruno Marsal (Bullja) in bug 984869 comment #34)
> Whenever I open a page including statistics of fritz.box, the browser
> crashes (if e10s enabled, the tabs only crash). Sample crash dump:
> https://crash-stats.mozilla.com/report/index/7dd8c317-355f-4e07-9e22-
> ea8792161009
> Apparently this signature started appearing 2016-10-07.
> 
> Used mozregression now for the first time, with this result: 
> 
> 2016-10-09T20:41:58: INFO : Narrowed inbound regression window from
> [6ad9a7fc, 1e3af7fd] (4 revisions) to [ffb38910, 1e3af7fd] (2 revisions) (~1
> steps left)
> 2016-10-09T20:41:58: DEBUG : Starting merge handling...
> 2016-10-09T20:41:58: DEBUG : Using url:
> https://hg.mozilla.org/integration/mozilla-inbound/json-
> pushes?changeset=1e3af7fd9bfe507cdcff5e05a95c3a763617f25c&full=1
> 2016-10-09T20:42:00: DEBUG : Found commit message:
> Bug 984869 - Reftests with display:flex/grid and columnset layout on
> <button>.
> 
> 2016-10-09T20:42:00: INFO : The bisection is done.
> 2016-10-09T20:42:00: INFO : Stopped

Comment 1

[Mats Palmgren (inactive)]

Bruno, I'm guessing "statistics of fritz.box" is on a web server on your
internal network?  If so, can you please save the web page that crashes,
and reduce it to the bare minimum that causes the crash?
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Reducing_testcases

Do you see the crash on any other (public) site?

If it's a regression from bug 984869 the crash is very likely related
to a <button> element in the web page.

Comment 2

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #1)
> If it's a regression from bug 984869 the crash is very likely related
> to a <button> element in the web page.

Actually, since there is no nsHTMLButtonControlFrame in the crash stack above,
I'm guessing it's some other element styled with display:flex, not a <button>.

Comment 3

[Bruno Marsal (Bullja)]

fritz.box is the management interface of my DSL router. In my case it is a FRITZ!Box 7490 running FRITZ!OS:06.60

Sadly there is a bunch of Javascript rendering the pages. Due to that I cannot easily save the page. Also I could not find any public site with this error.

My only guess is that it is somehow related to how the statistics are rendered on the info pages. A quick look at the source code I found that http://gionkunz.github.io/chartist-js/ is beeing used. The examples of this library render fine though.

crash reports show there are 57 reports with the same signature. Do any of those reveal public URLs?

Comment 4

[Mats Palmgren (inactive)]

I suspect that the condition "if (bits & FCDATA_CREATE_BLOCK_WRAPPER_FOR_ALL_KIDS)"
in https://bugzilla.mozilla.org/attachment.cgi?id=8794799&action=diff
is too generic.  We probably get here for all sorts of elements actually.
We should probably wrap the whole new block added in bug 984869 inside
a "if (bits & FCDATA_ALLOW_BLOCK_STYLES)" block...

Comment 5

[Mats Palmgren (inactive)]

(In reply to Bruno Marsal (Bullja) from comment #3)
> crash reports show there are 57 reports with the same signature. Do any of
> those reveal public URLs?

Thanks for the info Bruno.  I'll check the other reports and see if I find one.

Comment 6

[Bruno Marsal (Bullja)]

Found a crashing page now: http://gionkunz.github.io/chartist-js/plugins.html

Comment 7

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]:

I found an URL that crashes in bp-3e916348-0386-40f2-8627-996ee2161009.

It appears to be a SVG <text> element with display:flex style:

SVGText(text)(3)@7fb98dc4cc60 next=7fb9935de090 {0,0,0,0} [state=00018a0000010602] [content=7fb990a48320] [sc=7fb99353c7c0]<
  FlexContainer(text)(3)@7fb98dc4cde8 {0,0,900,780} [state=0000900000000000] [content=7fb990a48320] [sc=7fb99353def0:-moz-svg-text]<
    Block(text)(3)@7fb98dc4cf40 {0,0,900,780} [state=0000800000d00200] [content=7fb990a48320] [sc=7fb9beed1ae0:-moz-anonymous-flex-item]<

and then we crash here:
http://searchfox.org/mozilla-central/rev/c635b8c61d648bb8a0317c19f8905b3be8132a8a/layout/svg/SVGTextFrame.cpp#4743
because it assumes the first child is always a block frame.

Comment 8

[Mats Palmgren (inactive)]

Attachment: CRASH testcase

Comment 10

[Mats Palmgren (inactive)]

Bruno, can you verify that the build I've made here fixes your crashes:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-b4a7f74f3591e18ce3279d919b7992721799aa0e/

Comment 11

[Bruno Marsal (Bullja)]

This debug build crashes after 30-60 seconds running it (win64). Shortly before it crashes, cpu usage peaks. This happens also when no page is opened at all.

But I can open the statistics page, your test case and the plugins page of the lib I mentioned. Well, for half a minute or so, then the browser crashes (but probably due to some other reason). With the official nightly version the pages crash immediately.

Comment 12

[Mats Palmgren (inactive)]

Thanks, I think that confirms it works.  The crash that occurs later is probably
some unrelated issue in the revision I picked to build from.  (Feel free to check
again with an official Nightly build after the fix lands though.)

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix

Comment 14

[Mats Palmgren (inactive)]

Attachment: fix (wdiff)

Same without white-space changes, so this is probably easier to review.

It restricts the feature added in bug 984869 to <button>s only.

Comment 15

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4a7f74f3591e18ce3279d919b7992721799aa0e&exclusion_profile=false

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c25f3665d45
Crash when setting display:[inline-]{flex,grid} or columnset properties on a SVG <text> element.  r=tn

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c25f3665d45

Comment 18

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I would really appreciate it if future changes to frame construction code actually described what the patch is doing in the commit message instead of just copying the bug summary.  This code is complicated enough that people dealing with it need all the help they can get, and obfuscated-ish patches don't help....

Comment 19

[Marcia Knous [:marcia]]

Tracking 52- - since this is now fixed no need to track.