Closed
Bug 1386905
Opened 7 years ago
Closed 7 years ago
stylo: Assertion failure: !mInStyleRefresh
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla57
People
(Reporter: truber, Assigned: xidorn)
References
(Blocks 2 open bugs)
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details |
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?
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Still doesn't work... :/
Assignee | ||
Updated•7 years ago
|
Attachment #8893615 -
Attachment is obsolete: true
Attachment #8893615 -
Flags: review?(ehsan)
Assignee | ||
Updated•7 years ago
|
Attachment #8893608 -
Attachment is obsolete: true
Attachment #8893608 -
Flags: review?(ehsan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
It seems to work now... https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bf105824b1487c796cf678fb97c0535a8588428
Comment 7•7 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+
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•7 years ago
|
Assignee: nobody → xidorn+moz
Comment 9•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 10•7 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(xidorn+moz)
Flags: in-testsuite?
Flags: in-testsuite+
Assignee | ||
Comment 11•7 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)
Comment 12•7 years ago
|
||
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•7 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•7 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?
Comment 15•7 years ago
|
||
It grafts cleanly to ESR52 too (other than a trivial crashtest manifest conflict), feel free to nominate it :)
Comment 16•7 years ago
|
||
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•7 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.
Assignee | ||
Comment 19•7 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•7 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+
Comment 22•7 years ago
|
||
bugherder uplift |
Comment 23•7 years ago
|
||
(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•7 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 25•7 years ago
|
||
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+
Updated•7 years ago
|
Comment 26•7 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•