Closed Bug 1435015 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: truber, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(1 file)

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?
Of course. Thanks :)
Flags: needinfo?(emilio)
This is actually a regression from bug 1427292.
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)
(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 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 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+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1118b9ce8633
Unbreak display: contents on unknown MathML elements. r=mats
https://hg.mozilla.org/mozilla-central/rev/1118b9ce8633
Status: NEW → RESOLVED
Closed: 3 years 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+
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.