Closed Bug 1401706 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: aFrame->StyleContext()->IsNonInheritingAnonBox() (Why did this frame not end up with a parent context?), at /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1516

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: emilio)

References

(Blocks 3 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(7 files)

trigger.html
529 bytes, text/html
Details
log_minidump.txt
89.44 KB, text/plain
Details
stack.
2.33 KB, text/plain
Details
Bug 1401706: Move ownership of editor anon content to nsTextControlFrame.
59 bytes, text/x-review-board-request
Sylvestre
: approval-mozilla-beta+
Details
Bug 1401706: Remove unused macro.
59 bytes, text/x-review-board-request
Sylvestre
: approval-mozilla-beta+
Details
Bug 1401706: Remove redundant boolean members from nsTextControlFrame.
59 bytes, text/x-review-board-request
Sylvestre
: approval-mozilla-beta+
Details
Bug 1401706: Remove redundant UpdateValueDisplay call.
59 bytes, text/x-review-board-request
Sylvestre
: approval-mozilla-beta+
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev 20170920-a20de99fa3c1.
Flags: in-testsuite?
Attached file log_minidump.txt — 
Minidump stacktrace
Assignee: nobody → jryans
Priority: -- → P2
I took a look at this, this is restyling a frame whose content is unbound from the tree when the `type` attribute of the <input> element changes, in nsTextEditorState::SyncUpSelectionPropertiesBeforeDestruction.

The correct way to fix this IMO is moving the anon content into nsTextControlFrame.
Attached file stack. — 
Stack as requested by bz.

We're reparenting the anonymous content frame's style even though we'll end up destroying it because the <input> element posted a reconstruct due to the "type" attribute changing.
So we should really either kill off the frames for the anon content near the end of nsTextEditorState::UnbindFromFrame or defer the UnbindFromTree calls until after we have done so.  See also bug 1400618...
Requesting tracking on all outstanding p2 stylo bugs.
tracking-firefox57: --- → ?
Haven't had time to look at this yet, so if someone has bandwidth overnight, feel free to grab.  Otherwise, I'll look into this tomorrow.
I looked into this and I have patches running through try, so taking. Thanks jryans!
Assignee: jryans → emilio
This should probably get reviewed by someone familiar with the text control frame bits (ehsan, maybe mats).
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #12)
> This should probably get reviewed by someone familiar with the text control
> frame bits (ehsan, maybe mats).

Good idea, will do. Seems like ehsan has done pretty similar refactorings in the past (moving the selection controller to the state object), so will flag him for review.
Attachment #8911133 - Flags: review?(bobbyholley) → review?(ehsan)
Attachment #8911134 - Flags: review?(bobbyholley) → review?(ehsan)
Attachment #8911135 - Flags: review?(bobbyholley) → review?(ehsan)
Attachment #8911136 - Flags: review?(bobbyholley) → review?(ehsan)
Status: NEW → ASSIGNED
Comment on attachment 8911133 [details]
Bug 1401706: Move ownership of editor anon content to nsTextControlFrame.

https://reviewboard.mozilla.org/r/182630/#review188032

Looks great, I have some minor comments below but nothing too big.

::: dom/html/nsTextEditorState.h:439
(Diff revision 1)
> -  nsCOMPtr<mozilla::dom::Element> mRootNode;
> -  nsCOMPtr<mozilla::dom::Element> mPlaceholderDiv;
> -  nsCOMPtr<mozilla::dom::Element> mPreviewDiv;
> -  nsTextControlFrame* mBoundFrame;
>    RefPtr<nsTextInputListener> mTextListener;
> +  nsTextControlFrame* mBoundFrame;

Not sure why you're moving mBoundFrame, but OK.

::: dom/html/nsTextEditorState.cpp
(Diff revision 1)
>  {
>    NS_ENSURE_TRUE_VOID(mBoundFrame);
>  
>    // If it was, however, it should be unbounded from the same frame.
> -  NS_ASSERTION(!aFrame || aFrame == mBoundFrame, "Unbinding from the wrong frame");
> +  MOZ_ASSERT(aFrame == mBoundFrame, "Unbinding from the wrong frame");
> -  NS_ENSURE_TRUE_VOID(!aFrame || aFrame == mBoundFrame);

Why remove this second check?  Please add it back.

::: layout/forms/nsTextControlFrame.h:197
(Diff revision 1)
> +  }
> +
> +  mozilla::dom::Element* GetPlaceholderNode() {
> +    return mPlaceholderDiv;
> +  }
> +  mozilla::dom::Element* GetPreviewNode() {

Nit: I think these two getters could also be const too.

(But if doing this isn't easy due to the way the code is set up, don't worry about it too much!)

::: layout/forms/nsTextControlFrame.h:347
(Diff revision 1)
>  
>    void FinishedInitializer() {
>      DeleteProperty(TextControlInitializer());
>    }
>  
> +  const nsString& CachedValue() const

If possible please make this return a const nsAString&.

::: layout/forms/nsTextControlFrame.h:390
(Diff revision 1)
> +  // Cache of the |.value| of <input> or <textarea> element without hard-wrap.
> +  // If its IsVoid() returns true, it doesn't cache |.value|.
> +  // Otherwise, it's cached when setting specific value or getting value from
> +  // TextEditor.  Additionally, when contents in the anonymous <div> element
> +  // is modified, this is cleared.
> +  nsString mCachedValue;

Now that you're touching this code, I think this could be an nsAutoString, to avoid allocations for small strings.  It'd be nice if you could look into the size of the nsTextControlFrame object before and after on 32 and 64-bit to make sure you don't end up in the next jemalloc allocation bucket.

(Could be a follow-up too...)
Attachment #8911133 - Flags: review?(ehsan) → review+
Comment on attachment 8911134 [details]
Bug 1401706: Remove unused macro.

https://reviewboard.mozilla.org/r/182632/#review188042
Attachment #8911134 - Flags: review?(ehsan) → review+
Comment on attachment 8911135 [details]
Bug 1401706: Remove redundant boolean members from nsTextControlFrame.

https://reviewboard.mozilla.org/r/182634/#review188044
Attachment #8911135 - Flags: review?(ehsan) → review+
Comment on attachment 8911136 [details]
Bug 1401706: Remove redundant UpdateValueDisplay call.

https://reviewboard.mozilla.org/r/182636/#review188046
Attachment #8911136 - Flags: review?(ehsan) → review+
Comment on attachment 8911133 [details]
Bug 1401706: Move ownership of editor anon content to nsTextControlFrame.

https://reviewboard.mozilla.org/r/182630/#review188032

> Why remove this second check?  Please add it back.

I was assuming tightening an assertion provided it holds was a good thing overall, but happy to add it back.

> Now that you're touching this code, I think this could be an nsAutoString, to avoid allocations for small strings.  It'd be nice if you could look into the size of the nsTextControlFrame object before and after on 32 and 64-bit to make sure you don't end up in the next jemalloc allocation bucket.
> 
> (Could be a follow-up too...)

I think frames use an arena, so not sure how much the jemalloc bucket applies. I'll file a followup for now.
Blocks: 1402545
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/89e5fc708a1d
Move ownership of editor anon content to nsTextControlFrame. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/2ccf0d54c0f9
Remove unused macro. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/d327d1b7324d
Remove redundant boolean members from nsTextControlFrame. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c7f9baa225ff
Remove redundant UpdateValueDisplay call. r=Ehsan
Attachment #8911133 - Flags: review?(bobbyholley)
Attachment #8911134 - Flags: review?(bobbyholley)
Attachment #8911135 - Flags: review?(bobbyholley)
Attachment #8911136 - Flags: review?(bobbyholley)
Please request Beta approval on this when you get a chance.
status-firefox55: --- → unaffected
status-firefox56: --- → disabled
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(emilio)
Flags: in-testsuite?
Flags: in-testsuite+
Comment on attachment 8911133 [details]
Bug 1401706: Move ownership of editor anon content to nsTextControlFrame.

This request applies for all the patches in the bug.

Note that this isn't a particularly trivial change, and it fixes a correctness issue, but we happen to give the right answer anyway, so there's no user-visible effect, nor safety issue.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1393791 (kinda, this was more an architectural bug that it exposed)
[User impact if declined]: not any user-visible impact known.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[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 much
[Why is the change risky/not risky?]: It's not very risky, IMO, because it just moves who owns a given set of elements, so they're not removed from the tree too early.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8911133 - Flags: approval-mozilla-beta?
Comment on attachment 8911133 [details]
Bug 1401706: Move ownership of editor anon content to nsTextControlFrame.

Fix an assert, early in the beta cycle, taking it.
Please fill the uplift request for every bugs, this avoid any potential confusion for RM & sheriffs.
Thanks
Should be in 57b3
Attachment #8911133 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8911134 - Flags: approval-mozilla-beta+
Attachment #8911135 - Flags: approval-mozilla-beta+
Attachment #8911136 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: