Slowly slowly making progress for bug 1418159. This still doesn't change any lifetime, nor which things are owned by which.

Comment on attachment 8940372 [details] Bug 1428491: Make the style set know about a document, not a pres context. https://reviewboard.mozilla.org/r/210636/#review216556 ::: layout/style/ServoStyleSet.h:603 (Diff revision 1) > void RemoveSheetOfType(SheetType aType, > ServoStyleSheet* aSheet); > > const Kind mKind; > > - // Nullptr if this is an XBL style set, or if we've been already detached from > + // The owner document of this style set. Null if this is an XBL style set. The pres shell is still the thing that owns the style set, right, since you haven't changed that? Though it's true the document will always outlive the pres shell. ::: layout/style/ServoStyleSet.h:605 (Diff revision 1) > - nsPresContext* MOZ_NON_OWNING_REF mPresContext = nullptr; > + // TODO(emilio): This should become a DocumentOrShadowRoot, and be owned by it > + // directly instead of the shell, eventually. I think we should still consider at some point not having an entire ServoStyleSet for shadow roots / XBL stuff... ::: layout/style/ServoStyleSet.cpp:134 (Diff revision 1) > set->ReplaceSheets(SheetType::Doc, aNewSheets); > > // Update stylist immediately. > set->UpdateStylist(); > > - // The PresContext of the bound document could be destroyed anytime later, > + // XBL resources are shared for a given URL, even across document, so we can't *documents ::: layout/style/ServoStyleSet.cpp:233 (Diff revision 1) > ServoStyleSet::MediumFeaturesChanged(bool aViewportChanged) > { > bool viewportUnitsUsed = false; > bool rulesChanged = MediumFeaturesChangedRules(&viewportUnitsUsed); > > - if (mBindingManager && > + if (GetPresContext() && Do if (nsPresContext* pc = GetPresContext()) { ... } so we don't have to walk through the document and pres shell to get it twice? ::: layout/style/ServoStyleSet.cpp:1003 (Diff revision 1) > // > // We don't need to do this for SMIL since SMIL only updates its animation > // values once at the begin of a tick. As a result, even if the previous > // traversal caused, for example, the font-size to change, the SMIL style > // won't be updated until the next tick anyway. > - if (mPresContext->EffectCompositor()->PreTraverse(aFlags)) { > + if (GetPresContext()->EffectCompositor()->PreTraverse(aFlags)) { Assert GetPresContext() at the top of this function. (Yes, PreTraverse() will do that for us, but good for each function to note its own preconditions I think.) ::: layout/style/ServoStyleSet.cpp:1395 (Diff revision 1) > Element* root = > - IsMaster() ? mPresContext->Document()->GetDocumentElement() : nullptr; > + IsMaster() ? mDocument->GetDocumentElement() : nullptr; This fits on one line now.

Comment on attachment 8940373 [details] Bug 1428491: Remove redundant mBindingManager member in ServoStyleSet. https://reviewboard.mozilla.org/r/210638/#review216558 ::: layout/style/ServoStyleSet.cpp:833 (Diff revision 1) > - if (mBindingManager) { > - mBindingManager->AppendAllSheets(aArray); > + if (mDocument) { > + mDocument->BindingManager()->AppendAllSheets(aArray); > } Are we sure that this won't append any sheets if this ServoStyleSet is Kind::ForXBL? Should we check for that explicitly? That, or assert that we didn't get any new sheets appended. (Before, we relied on mBindingManager being null.)

Comment on attachment 8940373 [details] Bug 1428491: Remove redundant mBindingManager member in ServoStyleSet. https://reviewboard.mozilla.org/r/210638/#review216558 > Are we sure that this won't append any sheets if this ServoStyleSet is Kind::ForXBL? Should we check for that explicitly? That, or assert that we didn't get any new sheets appended. (Before, we relied on mBindingManager being null.) mDocument is null in that case, since the xbl resources aren't tied to a document in particular (with all the madness that entails).

Comment on attachment 8940372 [details] Bug 1428491: Make the style set know about a document, not a pres context. https://reviewboard.mozilla.org/r/210636/#review216556 > The pres shell is still the thing that owns the style set, right, since you haven't changed that? Though it's true the document will always outlive the pres shell. Right, the lifetime hasn't changed yet, and I don't think I can change it until we get rid of the old style system.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dbfd798e491b Make the style set know about a document, not a pres context. r=heycam https://hg.mozilla.org/integration/autoland/rev/308e79e6c98f Remove redundant mBindingManager member in ServoStyleSet. r=heycam

Backout by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/da9adac5500f Backed out 2 changesets for failing browser chrome mochitest at /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:941 on a CLOSED TREE

Backed out for failing browser chrome mochitest at /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:941 on a CLOSED TREE Push that caused failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=308e79e6c98f0f38c3c7c1652fd6ceae35a12955 Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=156564106&repo=autoland&lineNumber=2447 Backout: https://hg.mozilla.org/integration/autoland/rev/da9adac5500fca2ee893ebe1c363ffd24cceb969

So, the last patch causes an assertion on an a11y test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcd3b06f5ad55f83f2ffb51abc2ec8118f82d0ef The assertion happens during the closing of the test, and the culprit is this declaration: https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/toolkit/themes/shared/in-content/info-pages.inc.css#17 The reason my patch makes it fail, is because we end up restyling the world, because SetVisibleArea() ends up posting a MediaFeatureValuesChanged event. But that assertion can be hit if we happened to restyle the body element in that reflow. The visible area is NS_UNCONSTRAINEDSIZExNS_UNCONSTRAINEDSIZE, as called from nsDocumentViewer::GetContentSize. That's why we end up with a min-height of NS_UNCONSTRAINEDSIZE, which ends up causing that assertion.

So this setup (both as in trunk, and as in my patch) looks kinda busted...

So, the fact that ResizeReflow can change which media queries apply and we're ignoring it and resolving viewport units with garbage values if there's a restyle pending for a given element looks totally borked to me. At this point, I think I can do three things: * Land the patch as-is, annotating the a11y expected assertion. Sounds somewhat scary, even though it's during shutdown. Plus we do more work than what we do now, which is annoying. * Don't land that patch, but null-check mDocument->GetShell() from there. Seems the easy / low risk and it's green. I'd like to investigate a bit more before why it's needed. This of course would have a bug on file for the MQ issue. * Give up, pass pres-context down DoProcessPendingRestyles, file bug for the MQ issue. Also low risk, I guess... Cam, opinions? I'd go with (3) for now, though it's kinda unfortunate.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17) > So, the fact that ResizeReflow can change which media queries apply and > we're ignoring it and resolving viewport units with garbage values if > there's a restyle pending for a given element looks totally borked to me. > > At this point, I think I can do three things: > > * Land the patch as-is, annotating the a11y expected assertion. Sounds > somewhat scary, even though it's during shutdown. Plus we do more work than > what we do now, which is annoying. > > * Don't land that patch, but null-check mDocument->GetShell() from there. > Seems the easy / low risk and it's green. I'd like to investigate a bit more > before why it's needed. This of course would have a bug on file for the MQ > issue. > > * Give up, pass pres-context down DoProcessPendingRestyles, file bug for > the MQ issue. Also low risk, I guess... > > Cam, opinions? I'd go with (3) for now, though it's kinda unfortunate. Won't take any of these three, I'll fix this properly in bug 1430844.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d96bbef4fdb6 Make the style set know about a document, not a pres context. r=heycam https://hg.mozilla.org/integration/autoland/rev/030453672f86 Remove redundant mBindingManager member in ServoStyleSet. r=heycam

https://hg.mozilla.org/mozilla-central/rev/d96bbef4fdb6 https://hg.mozilla.org/mozilla-central/rev/030453672f86

Aryx, can we back this out of mozilla-beta for causing bug 1431474?

Backed out for crashes bug 1431474 Back-out link: https://hg.mozilla.org/releases/mozilla-beta/rev/6b6e017f5974d0034b31531f44bc3907a637b5b3

Thank you Cosmin!