Which breaks an invariant I want to add in order for code in bug 1355343 to be sane.

Here's a try run, which shows unexpected passes even with the current code, which means that this is unsound for the snapshot model. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7cf801945b204ee983368f30c4a293ff2d0bc94&selectedJob=96258740

nsIReflowCallback _is_ outside reflow, no? We have other reflow callbacks that do all sorts of stuff like changing attributes and whatnot...

Comment on attachment 8864234 [details] Bug 1361766: Move MathML content state changes outside of reflow. https://reviewboard.mozilla.org/r/135872/#review138920 ::: layout/mathml/nsMathMLContainerFrame.h:13 (Diff revision 1) > > #include "mozilla/Attributes.h" > #include "nsContainerFrame.h" > #include "nsBlockFrame.h" > #include "nsInlineFrame.h" > +#include "nsIReflowCallback.h" This is no longer needed, and will go away.

Oh, I see. They're doing their change directly under SetIncrementScriptLevel, not drom the reflow callback. Yeah, that's no good. Moving that work to the reflow callback seems like exactly the right fix.

That said, you may have to deal with the frame being killed before the callback is called; in that case you probably need to CancelReflowCallback.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6) > That said, you may have to deal with the frame being killed before the > callback is called; in that case you probably need to CancelReflowCallback. I _think_ this is only called from reflow, so I'd expect the frame to be kept alive until the end unconditionally, but I'll double-check that, and in case I can't prove it I'll add the relevant bits to cancel the reflow callback when the frame is destroyed.

Frames can get created and destroyed during reflow, in general (e.g., fluid continuations can do that). Whether that can happen for these mathml frames I don't know offhand. It's possible it can't.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #8) > Frames can get created and destroyed during reflow, in general (e.g., fluid > continuations can do that). Whether that can happen for these mathml frames > I don't know offhand. It's possible it can't. I think they can't, but didn't seem worth the risk, so I added the check regardless.

Comment on attachment 8864234 [details] Bug 1361766: Move MathML content state changes outside of reflow. https://reviewboard.mozilla.org/r/135872/#review139422 ::: layout/mathml/nsMathMLmunderoverFrame.h:27 (Diff revision 2) > - virtual nsresult > - Place(DrawTarget* aDrawTarget, > + nsresult Place(DrawTarget* aDrawTarget, > + bool aPlaceOrigin, > - bool aPlaceOrigin, > - ReflowOutput& aDesiredSize) override; > + ReflowOutput& aDesiredSize) override; > > - NS_IMETHOD > - InheritAutomaticData(nsIFrame* aParent) override; > + NS_IMETHOD InheritAutomaticData(nsIFrame* aParent) override; > + > + NS_IMETHOD TransmitAutomaticData() override; > + > + NS_IMETHOD UpdatePresentationData(uint32_t aFlagsValues, > + uint32_t aFlagsToUpdate) override; > + > + void DestroyFrom(nsIFrame* aRoot) override; > > - NS_IMETHOD > - TransmitAutomaticData() override; > + nsresult AttributeChanged(int32_t aNameSpaceID, > + nsIAtom* aAttribute, > + int32_t aModType) override; > > - NS_IMETHOD > + uint8_t ScriptIncrement(nsIFrame* aFrame) override; Could you do the cosmetic changes in a separate commit? That would make things clearer and easier to review. ::: layout/mathml/nsMathMLmunderoverFrame.h:77 (Diff revision 2) > virtual ~nsMathMLmunderoverFrame(); > > private: > bool mIncrementUnder; > bool mIncrementOver; > + nsTArray<std::pair<uint32_t, bool>> mPostReflowIncrementScriptLevelTo; Please avoid using something like `std::pair<uint32_t, bool>`. It is very unclear what do the fields mean. Create a struct should be much better. (Actually, investigating the callsites of `SetIncrementScriptLevel`, it seems to me only two children may ever be passed here, so the storage can be more compact. That may or may not worth the effort, though, given that this frame would rarely be used.) ::: layout/mathml/nsMathMLmunderoverFrame.cpp:110 (Diff revision 2) > +nsMathMLmunderoverFrame::SetIncrementScriptLevel(uint32_t aChildIndex, > + bool aIncrement) > +{ > + nsIFrame* child = PrincipalChildList().FrameAt(aChildIndex); > + if (!child || !child->GetContent()->IsMathMLElement()) > + return; Please wrap the body with `{` and `}` here and elsewhere in the changed code of this commit. ::: layout/mathml/nsMathMLmunderoverFrame.cpp:112 (Diff revision 2) > +{ > + nsIFrame* child = PrincipalChildList().FrameAt(aChildIndex); > + if (!child || !child->GetContent()->IsMathMLElement()) > + return; > + > + auto* element = static_cast<nsMathMLElement*>(child->GetContent()); Just `auto` is enough (`*` is redundant). ::: layout/mathml/nsMathMLmunderoverFrame.cpp:133 (Diff revision 2) > +} > + > +void > +nsMathMLmunderoverFrame::ReflowCallbackCanceled() > +{ > + SetPendingPostReflowIncrementScriptLevel(); According to the comment of `nsIReflowCallback`, `ReflowCallbackCanceled` is called when the shell is being destroyed. Do we really need to do this work at that point? ::: layout/mathml/nsMathMLmunderoverFrame.cpp:146 (Diff revision 2) > + for (const auto& pair : mPostReflowIncrementScriptLevelTo) { > + nsIFrame* child = PrincipalChildList().FrameAt(pair.first); > + if (!child || !child->GetContent()->IsMathMLElement()) > + continue; > + > + auto* element = static_cast<nsMathMLElement*>(child->GetContent()); Just `auto` is enough. ::: layout/mathml/nsMathMLmunderoverFrame.cpp:150 (Diff revision 2) > + > + auto* element = static_cast<nsMathMLElement*>(child->GetContent()); > + element->SetIncrementScriptLevel(pair.second, true); > + } > + > + mPostReflowIncrementScriptLevelTo.Clear(); Consider moving the content of the array into a local `nsTArray` before iterating, rather than iterate-then-clear. In that way, we can guarantee that the array wouldn't be mutated during iteration.

Comment on attachment 8864234 [details] Bug 1361766: Move MathML content state changes outside of reflow. https://reviewboard.mozilla.org/r/135872/#review139598 ::: layout/mathml/nsMathMLmunderoverFrame.h:77 (Diff revision 2) > virtual ~nsMathMLmunderoverFrame(); > > private: > bool mIncrementUnder; > bool mIncrementOver; > + nsTArray<std::pair<uint32_t, bool>> mPostReflowIncrementScriptLevelTo; Sounds good ::: layout/mathml/nsMathMLmunderoverFrame.cpp:110 (Diff revision 2) > +nsMathMLmunderoverFrame::SetIncrementScriptLevel(uint32_t aChildIndex, > + bool aIncrement) > +{ > + nsIFrame* child = PrincipalChildList().FrameAt(aChildIndex); > + if (!child || !child->GetContent()->IsMathMLElement()) > + return; I was following local file style fwiw, but the file is inconsistent within itself, so will do, thanks for the chatch :) ::: layout/mathml/nsMathMLmunderoverFrame.cpp:112 (Diff revision 2) > +{ > + nsIFrame* child = PrincipalChildList().FrameAt(aChildIndex); > + if (!child || !child->GetContent()->IsMathMLElement()) > + return; > + > + auto* element = static_cast<nsMathMLElement*>(child->GetContent()); I actually prefer the star. Not in the case of `static_cast`, but in other situations it may very well be a footgun. Can remove this one, I guess. ::: layout/mathml/nsMathMLmunderoverFrame.cpp:133 (Diff revision 2) > +} > + > +void > +nsMathMLmunderoverFrame::ReflowCallbackCanceled() > +{ > + SetPendingPostReflowIncrementScriptLevel(); Yeah, I guess at that point it doesn't make much sense, will leave this empty. ::: layout/mathml/nsMathMLmunderoverFrame.cpp:150 (Diff revision 2) > + > + auto* element = static_cast<nsMathMLElement*>(child->GetContent()); > + element->SetIncrementScriptLevel(pair.second, true); > + } > + > + mPostReflowIncrementScriptLevelTo.Clear(); Sounds good

Comment on attachment 8864234 [details] Bug 1361766: Move MathML content state changes outside of reflow. https://reviewboard.mozilla.org/r/135872/#review139598 > I actually prefer the star. Not in the case of `static_cast`, but in other situations it may very well be a footgun. Can remove this one, I guess. I think in case that `auto` could be a footgun, we prefer writting down the complete type.

Comment on attachment 8864234 [details] Bug 1361766: Move MathML content state changes outside of reflow. https://reviewboard.mozilla.org/r/135872/#review139608 ::: layout/mathml/nsMathMLmunderoverFrame.h:58 (Diff revision 3) > + void SetIncrementScriptLevel(uint32_t aChildIndex, bool aIncrement); > + void SetPendingPostReflowIncrementScriptLevel(); Do the two methods need to be public? Probably they should be private instead. ::: layout/mathml/nsMathMLmunderoverFrame.cpp:137 (Diff revision 3) > +} > + > +void > +nsMathMLmunderoverFrame::ReflowCallbackCanceled() > +{ > + // Do nothing, at this point our work will just be useless. You probably still want to clear the array, I guess, otherwise `DestroyFrom` may try to remove the instance from PresShell while it has already been removed after this call.

Comment on attachment 8864234 [details] Bug 1361766: Move MathML content state changes outside of reflow. https://reviewboard.mozilla.org/r/135872/#review139608 > Do the two methods need to be public? Probably they should be private instead. Sure, thanks. > You probably still want to clear the array, I guess, otherwise `DestroyFrom` may try to remove the instance from PresShell while it has already been removed after this call. Great catch. It's inocuous to try to remove an unexistent callback, but yeah, much better to just avoid the work :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30cb346f069fd2e1811b7b30936327c5bda2f1a1&selectedJob=96912993 (The patch already have the expectations updated)

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3e6477d93203 Move MathML content state changes outside of reflow. r=xidorn