stylo: Assertion failure: !mInStyleRefresh

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: truber, Assigned: xidorn)

Tracking

(Blocks 2 bugs, {assertion, testcase})

Trunk
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr5256+ fixed, firefox55 wontfix, firefox56+ fixed, firefox57+ verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted file testcase.html
Attached testcase causes an assertion in m-c rev 52285ea5e54c with stylo enabled by pref.

This also reproduces bug 1363490 without stylo, but that testcase no longer works.

Assertion failure: !mInStyleRefresh, at /home/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1011
#01: mozilla::PresShell::ContentStateChanged at layout/base/RestyleManagerInlines.h:51
#02: nsDocument::ContentStateChanged at dom/base/nsDocument.cpp:5420
#03: mozilla::dom::Element::UpdateState at dom/base/Element.cpp:272
#04: mozilla::dom::HTMLInputElement::OnValueChanged at dom/html/HTMLInputElement.cpp:7458
#05: nsTextEditorState::SetValue at dom/html/nsTextEditorState.cpp:2756
#06: nsTextEditorState::UnbindFromFrame at dom/html/nsTextEditorState.cpp:2229
#07: nsTextControlFrame::DestroyFrom at layout/forms/nsTextControlFrame.cpp:135
#08: nsFrameList::DestroyFramesFrom at layout/generic/nsFrameList.cpp:59
#09: nsContainerFrame::DestroyFrom at layout/generic/nsContainerFrame.cpp:226
#10: nsFrameList::DestroyFramesFrom at layout/generic/nsFrameList.cpp:59
#11: nsContainerFrame::DestroyFrom at layout/generic/nsContainerFrame.cpp:226
#12: nsLineBox::DeleteLineList at layout/generic/nsLineBox.cpp:396
#13: nsBlockFrame::DestroyFrom at layout/generic/nsBlockFrame.cpp:334
#14: nsLineBox::DeleteLineList at layout/generic/nsLineBox.cpp:396
#15: nsBlockFrame::DestroyFrom at layout/generic/nsBlockFrame.cpp:334
#16: nsFrameList::DestroyFramesFrom at layout/generic/nsFrameList.cpp:59
#17: nsContainerFrame::DestroyFrom at layout/generic/nsContainerFrame.cpp:226
#18: nsCanvasFrame::DestroyFrom at layout/generic/nsCanvasFrame.cpp:159
#19: nsFrameList::DestroyFramesFrom at layout/generic/nsFrameList.cpp:59
#20: nsContainerFrame::DestroyFrom at layout/generic/nsContainerFrame.cpp:226
#21: nsContainerFrame::RemoveFrame at layout/generic/nsContainerFrame.cpp:175
#22: nsFrameManager::RemoveFrame at layout/base/nsFrameManager.cpp:428
#23: nsCSSFrameConstructor::ContentRemoved at layout/base/nsCSSFrameConstructor.cpp:8866
#24: nsCSSFrameConstructor::RecreateFramesForContent at layout/base/nsCSSFrameConstructor.cpp:10062
#25: mozilla::RestyleManager::ProcessRestyledFrames at layout/base/RestyleManager.cpp:1515
#26: mozilla::ServoRestyleManager::DoProcessPendingRestyles at xpcom/ds/nsTArray.h:398
Flags: in-testsuite?
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
Still doesn't work... :/
(Assignee)

Updated

2 years ago
Attachment #8893615 - Attachment is obsolete: true
Attachment #8893615 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Attachment #8893608 - Attachment is obsolete: true
Attachment #8893608 - Flags: review?(ehsan)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8893680 [details]
Bug 1386905 - Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer.

https://reviewboard.mozilla.org/r/164794/#review170184

Wow, how did we get this far without this blowing up somehow?  Thanks for the patch!
Attachment #8893680 - Flags: review?(ehsan) → review+

Comment 8

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0800d81dbd72
Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer. r=Ehsan
(Assignee)

Updated

2 years ago
Assignee: nobody → xidorn+moz
https://hg.mozilla.org/mozilla-central/rev/0800d81dbd72
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
Flags: needinfo?(xidorn+moz)
Flags: in-testsuite?
Flags: in-testsuite+
(Assignee)

Comment 11

2 years ago
I don't think this only affects beta. This seems to be something which can affect lots of versions. It isn't Stylo-specific. We just don't have a reliable testcase to reproduce it without Stylo. (see also bug 1363490).
Flags: needinfo?(xidorn+moz)
We can call ESR52/Fx55 wontfix if you prefer :) - without any reliable STR or evidence of user impact, we wouldn't consider it for anything beyond Beta anyway.
(Assignee)

Comment 13

2 years ago
Actually I would rather uplift it to all versions... because notifying something when it shouldn't is a usual pattern of security bugs. Well...
(Assignee)

Comment 14

2 years ago
Comment on attachment 8893680 [details]
Bug 1386905 - Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer.

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: unknown
[Is this code covered by automated tests?]: yes, but that test was only reproducible with stylo
[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]: no
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: doesn't seem to have significant change on code logic
[String changes made/needed]: n/a
Attachment #8893680 - Flags: approval-mozilla-beta?
It grafts cleanly to ESR52 too (other than a trivial crashtest manifest conflict), feel free to nominate it :)
https://hg.mozilla.org/projects/date/rev/0800d81dbd72013e560ccc86c4cf4bc8197fd885
Bug 1386905 - Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer. r=Ehsan
(Reporter)

Comment 17

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> I don't think this only affects beta. This seems to be something which can
> affect lots of versions. It isn't Stylo-specific. We just don't have a
> reliable testcase to reproduce it without Stylo. (see also bug 1363490).

This testcase did reproduce without Stylo for me too (bug 1363490). Will this fix non-Stylo too?
Jesse, since you an reproduce this without Stylo, can you test and see if this patch fixes it? 
Or we can ask QE.
Flags: needinfo?(jschwartzentruber)
(Assignee)

Comment 19

2 years ago
(In reply to Jesse Schwartzentruber (:truber) from comment #17)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> > I don't think this only affects beta. This seems to be something which can
> > affect lots of versions. It isn't Stylo-specific. We just don't have a
> > reliable testcase to reproduce it without Stylo. (see also bug 1363490).
> 
> This testcase did reproduce without Stylo for me too (bug 1363490). Will
> this fix non-Stylo too?

It is supposed to fix non-Stylo as well.
(Reporter)

Comment 20

2 years ago
Confirmed in bug 1363490. This does fix non-Stylo too.
Flags: needinfo?(jschwartzentruber)
Status: RESOLVED → VERIFIED
Comment on attachment 8893680 [details]
Bug 1386905 - Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer.

Fix for unnecessary assertion, verified in nightly, OK to uplift for beta 3.
Attachment #8893680 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> It grafts cleanly to ESR52 too (other than a trivial crashtest manifest
> conflict), feel free to nominate it :)
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 24

2 years ago
Comment on attachment 8893680 [details]
Bug 1386905 - Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fix potential security bug because notifying something when it shouldn't is a usual pattern for security bugs
User impact if declined: unknown
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(xidorn+moz)
Attachment #8893680 - Flags: approval-mozilla-esr52?
Comment on attachment 8893680 [details]
Bug 1386905 - Move away mRuleNode in nsTextEditorState::UnbindFromFrame before storing the value into text buffer.

crash fix for esr52.4
Attachment #8893680 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.