Emilio Cobos Álvarez (:emilio) Assignee
	Description • 7 years ago

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

	Cameron McCormack (:heycam)
	Comment 3 • 7 years ago
mozreview-review

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.

	Cameron McCormack (:heycam)
	Comment 4 • 7 years ago
mozreview-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.)

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 5 • 7 years ago
mozreview-review-reply

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 9 • 7 years ago

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

	Pulsebot
	Comment 10 • 7 years ago

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

	Andreea Pavel [:apavel]
	Comment 11 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 15 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 16 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 17 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 18 • 7 years ago

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

	Pulsebot
	Comment 19 • 7 years ago

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

	Dorel Luca [:dluca]
	Comment 20 • 7 years ago
bugherder

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

	Julien Cristau [:jcristau]
	Comment 21 • 7 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 22 • 7 years ago

Backed out for crashes bug 1431474

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 23 • 7 years ago

Thank you Cosmin!