Crash when slot used in MathML [@ 0x00000000][@ nsCSSFrameConstructor::ConstructFramesFromItem]

RESOLVED FIXED in Firefox 59

Status

()

defect
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: truber, Assigned: emilio)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 fixed, firefox60 fixed)

Details

Attachments

(1 attachment)

The following testcase causes a crash in m-c rev 20180201-a23212941fcd

    <math><slot>

#0: +0x0
#1: nsCSSFrameConstructor::ConstructFramesFromItem, at layout/base/nsCSSFrameConstructor.cpp:6469
#2: nsCSSFrameConstructor::ConstructFramesFromItemList, at layout/base/nsCSSFrameConstructor.cpp:10844
#3: nsCSSFrameConstructor::ProcessChildren, at layout/base/nsCSSFrameConstructor.cpp:11164
#4: nsCSSFrameConstructor::ConstructFrameFromItemInternal, at layout/base/nsCSSFrameConstructor.cpp:4251
#5: nsCSSFrameConstructor::ConstructFramesFromItem, at layout/base/nsCSSFrameConstructor.cpp:6469
#6: nsCSSFrameConstructor::ConstructFramesFromItemList, at layout/base/nsCSSFrameConstructor.cpp:10844
#7: nsCSSFrameConstructor::ContentAppended, at layout/base/nsCSSFrameConstructor.cpp:7844
#8: mozilla::RestyleManager::ProcessRestyledFrames, at layout/base/RestyleManager.cpp:1416
#9: mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1174
Flags: in-testsuite?
(Assignee)

Comment 1

a year ago
Of course. Thanks :)
Flags: needinfo?(emilio)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
This is actually a regression from bug 1427292.
(Assignee)

Updated

a year ago
Blocks: 1427292
Flags: needinfo?(emilio)
I'm still nervous about enabling display:contents for MathML.
I'd like a positive signal from the CSSWG before we go down this path.
(on https://github.com/w3c/csswg-drafts/issues/2167)
(Assignee)

Comment 5

a year ago
(In reply to Mats Palmgren (:mats) from comment #4)
> I'm still nervous about enabling display:contents for MathML.
> I'd like a positive signal from the CSSWG before we go down this path.
> (on https://github.com/w3c/csswg-drafts/issues/2167)

Yeah, that's fair, I'll revert to the previous behavior for now, will send you an updated patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
mozreview-review
Comment on attachment 8947925 [details]
Bug 1435015: Unbreak display: contents on unknown MathML elements.

https://reviewboard.mozilla.org/r/217606/#review223402

::: layout/base/nsCSSFrameConstructor.cpp:5673
(Diff revision 3)
> -        aTag != nsGkAtoms::svg &&
> -        aTag != nsGkAtoms::tspan) {
> -      return &sSuppressData;
> +        aTag == nsGkAtoms::svg ||
> +        aTag == nsGkAtoms::tspan) {
> +      return nullptr;
>      }
> +
> +    return &sSuppressData;

Err, this doesn't work now for unknown SVG elements, fixing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

a year ago
mozreview-review
Comment on attachment 8947925 [details]
Bug 1435015: Unbreak display: contents on unknown MathML elements.

https://reviewboard.mozilla.org/r/217606/#review223442

LGTM, thanks.  Please update the commit message acordingly though.
Attachment #8947925 - Flags: review?(mats) → review+
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1118b9ce8633
Unbreak display: contents on unknown MathML elements. r=mats

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1118b9ce8633
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Please request Beta approval on this when you get a chance.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Flags: in-testsuite?
Flags: in-testsuite+
(Assignee)

Comment 17

a year ago
Comment on attachment 8947925 [details]
Bug 1435015: Unbreak display: contents on unknown MathML elements.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1427292
[User impact if declined]: trivial (but hard to hit on the wild) crash
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: one-liner that fixes the condition we use to avoid display: contents to not apply to unknown mathml elements.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8947925 - Flags: approval-mozilla-beta?
Comment on attachment 8947925 [details]
Bug 1435015: Unbreak display: contents on unknown MathML elements.

Trivial crash fix that includes a test. Taking for 59b8.
Attachment #8947925 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.