Closed Bug 1428491 Opened 2 years ago Closed 2 years ago

Make ServoStyleSet hold a reference to a document, not to a pres context.

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #8940372 - Flags: review?(cam) → review+
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.)
Attachment #8940373 - Flags: review?(cam) → review+
Priority: -- → P3
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
Flags: needinfo?(emilio)
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.
Flags: needinfo?(emilio)
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.
Flags: needinfo?(cam)
Depends on: 1430844
(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.
Flags: needinfo?(cam)
Attachment #8942889 - Attachment is obsolete: true
Attachment #8942889 - Flags: review?(cam)
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1431474
Aryx, can we back this out of mozilla-beta for causing bug 1431474?
Flags: needinfo?(aryx.bugmail)
Thank you Cosmin!
Flags: needinfo?(emilio)
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.