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

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428491: Make the style set know about a document, not a pres context.

Review commit: https://reviewboard.mozilla.org/r/210636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210636/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428491: Remove redundant mBindingManager member in ServoStyleSet.

Review commit: https://reviewboard.mozilla.org/r/210638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210638/

Comment 3

[Cameron McCormack (:heycam)]

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 4

[Cameron McCormack (:heycam)]

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 5

[Emilio Cobos Álvarez (:emilio)]

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 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8940372 [details]
Bug 1428491: Make the style set know about a document, not a pres context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210636/diff/1-2/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8940373 [details]
Bug 1428491: Remove redundant mBindingManager member in ServoStyleSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210638/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Pulsebot]

Backout by [e-mail]:
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

Comment 11

[Andreea Pavel [:apavel]]

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428491: Make sure that ResizeReflowIgnoreOverride doesn't style a document with a detached shell.

We weren't checking mIsDestroying before flushing styles. Use the more general
path instead.

Review commit: https://reviewboard.mozilla.org/r/213156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213156/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8940372 [details]
Bug 1428491: Make the style set know about a document, not a pres context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210636/diff/2-3/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8940373 [details]
Bug 1428491: Remove redundant mBindingManager member in ServoStyleSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210638/diff/2-3/

Comment 15

[Emilio Cobos Álvarez (: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.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Dorel Luca [:dluca]]

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

Comment 21

[Julien Cristau [:jcristau]]

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

Comment 22

[Cosmin Sabou [:CosminS]]

Backed out for crashes bug 1431474

Back-out link: https://hg.mozilla.org/releases/mozilla-beta/rev/6b6e017f5974d0034b31531f44bc3907a637b5b3

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Thank you Cosmin!