Testcase found while fuzzing mozilla-central rev 20170920-a20de99fa3c1.

Minidump stacktrace

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.

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.

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!

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.

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...)

Comment on attachment 8911134 [details] Bug 1401706: Remove unused macro. https://reviewboard.mozilla.org/r/182632/#review188042

Comment on attachment 8911135 [details] Bug 1401706: Remove redundant boolean members from nsTextControlFrame. https://reviewboard.mozilla.org/r/182634/#review188044

Comment on attachment 8911136 [details] Bug 1401706: Remove redundant UpdateValueDisplay call. https://reviewboard.mozilla.org/r/182636/#review188046

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.

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

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/08e7d627c201 Crashtest. r=me

https://hg.mozilla.org/mozilla-central/rev/89e5fc708a1d https://hg.mozilla.org/mozilla-central/rev/2ccf0d54c0f9 https://hg.mozilla.org/mozilla-central/rev/d327d1b7324d https://hg.mozilla.org/mozilla-central/rev/c7f9baa225ff https://hg.mozilla.org/mozilla-central/rev/08e7d627c201

Please request Beta approval on this when you get a chance.

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

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4d33cf91c8bd https://hg.mozilla.org/releases/mozilla-beta/rev/f1e83779aee9 https://hg.mozilla.org/releases/mozilla-beta/rev/a896c16082f4 https://hg.mozilla.org/releases/mozilla-beta/rev/73daa36f9532 https://hg.mozilla.org/releases/mozilla-beta/rev/2ac580ab9211