1346109 - don't hit the "no displayport base rect set" case in practice

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Timothy Nikkel (:tnikkel)]

With a few patches we can make it so that we never hit this on try server.

The motivation for this is that I was debugging an image related try server failure and found that we marked a visible image as non-visible because we used a displayport without the base rect being set in the image visibility code.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Factor out a GetDisplayPortData function

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Add a function that says if we are missing a base rect

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Allow asking a scrollframe if it's root.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Set a base rect in image visibility (or ignore the displayport)

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Set a base rect in APZCCallbackHelper::InitializeRootDisplayport

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Set a base rect before calling MaybeCreateDisplayport

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Set a base rect in MobileViewportManager::UpdateDisplayPortMargins

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Make it an assert

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8845762 [details] [diff] [review]
Factor out a GetDisplayPortData function

Review of attachment 8845762 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +1166,5 @@
>  }
>  
>  static bool
> +GetDisplayPortData(nsIContent* aContent,
> +                   DisplayPortPropertyData*& aRectData,

We have a convention of prefixing out-parameters with |aOut|, e.g. |aOutRectData|.

Also, Kats prefers out-parameters to be pointers rather than references, so there is a syntactic marker alerting you to their presence at the call site. I don't care that much, but for consistency we should probably follow that convention too.

@@ +1195,5 @@
> +  return true;
> +}
> +
> +static bool
> +GetDisplayPortImpl(nsIContent* aContent, nsRect *aResult, float aMultiplier)

|nsRect *aResult| -> |nsRect* aResult|

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8845764 [details] [diff] [review]
Allow asking a scrollframe if it's root.

Review of attachment 8845764 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIScrollableFrame.h
@@ +488,5 @@
> +   * Returns whether this scroll frame is the root scroll frame of the document
> +   * that it is in. Note that some documents don't have root scroll frames at
> +   * all (ie XUL documents) even though they may contain other scroll frames.
> +   */
> +  virtual bool IsRootScrollFrameOfDocument() = 0;

Method should be const-qualified.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8845766 [details] [diff] [review]
Set a base rect in image visibility (or ignore the displayport)

Review of attachment 8845766 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/PresShell.cpp
@@ +5972,2 @@
>      nsRect displayPort;
> +    bool usingDisplayport = !ignoreDisplayPort &&

Is having |!ignoreDisplayPort| here necessary? Won't GetDisplayPortForVisibilityTesting() return false in the first place if there is no display port base? (Or are you just adding it as a "we already know, don't recompute" shortcut?)

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8845767 [details] [diff] [review]
Set a base rect in APZCCallbackHelper::InitializeRootDisplayport

Review of attachment 8845767 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +351,5 @@
> +    } else if (pc) {
> +      baseRect = nsRect(nsPoint(0, 0), pc->GetVisibleArea().Size());
> +    }
> +    nsLayoutUtils::SetDisplayPortBaseIfNotSet(content, baseRect);
> +    // Note that the proper base rect that goes with these margins is set in

Should this comment be updated?

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8845766 [details] [diff] [review]
Set a base rect in image visibility (or ignore the displayport)

Review of attachment 8845766 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/PresShell.cpp
@@ +5972,2 @@
>      nsRect displayPort;
> +    bool usingDisplayport = !ignoreDisplayPort &&

Oh, I guess not; GetDisplayPortFromMarginsData() applies the margins to an empty rect if there is no base rect (??). Never mind, then.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #12)
> Comment on attachment 8845767 [details] [diff] [review]
> Set a base rect in APZCCallbackHelper::InitializeRootDisplayport
> 
> Review of attachment 8845767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +351,5 @@
> > +    } else if (pc) {
> > +      baseRect = nsRect(nsPoint(0, 0), pc->GetVisibleArea().Size());
> > +    }
> > +    nsLayoutUtils::SetDisplayPortBaseIfNotSet(content, baseRect);
> > +    // Note that the proper base rect that goes with these margins is set in
> 
> Should this comment be updated?

I guess I was using "proper" to mean the base rect set during painting as painting is the only place where we do a full and proper computation of every base rect.

I'll change the comment to say "We also set the base rect that goes with these margins in...".

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #11)
> Is having |!ignoreDisplayPort| here necessary? Won't
> GetDisplayPortForVisibilityTesting() return false in the first place if
> there is no display port base? (Or are you just adding it as a "we already
> know, don't recompute" shortcut?)

(In reply to Botond Ballo [:botond] from comment #13)
> Oh, I guess not; GetDisplayPortFromMarginsData() applies the margins to an
> empty rect if there is no base rect (??). Never mind, then.

Yeah, the functions that check for the presence of a displayport only check if we have the margins, not a base rect, and use an empty rect if we don't have a base rect. Because we thought it would be rare to hit that case, and then once we knew it wasn't rare we didn't observe any problems from it, until I noticed image visibility messing up, hence this bug.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecfadfea872c
Factor out a GetDisplayPortData function which gets the displayport data and chooses between margin and rect based data. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/659ceea3e795
Add a function that returns if we need a displayport base rect for calculating a displayport. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2db4358a81c
Allow asking an nsIScrollableFrame if it is the root scroll frame. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/c98a09f7a6f4
Change the image visibility frame walker to handle missing displayport base rects. r=botond

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ecfadfea872c
https://hg.mozilla.org/mozilla-central/rev/659ceea3e795
https://hg.mozilla.org/mozilla-central/rev/c2db4358a81c
https://hg.mozilla.org/mozilla-central/rev/c98a09f7a6f4

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8845768 [details] [diff] [review]
Set a base rect before calling MaybeCreateDisplayport

Review of attachment 8845768 [details] [diff] [review]:
-----------------------------------------------------------------

Typos in commit message: |vert|, |caclutions|.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3593,5 @@
>  
>      if (aAllowCreateDisplayPort) {
> +      // Set a reasonable base rect before calling MaybeCreateDisplayPort because
> +      // if MaybeCreateDisplayPort creates a displayport it will also ask for the
> +      // existing displayport and we want a base rect available for that.

I find this comment a bit misleading. MaybeCreateDisplayPort only creates a displayport if there wasn't one before.

I think what's actually happening is that SetDisplayPortMargins tries to unconditionally combine the new margins with the base rect to see if the resulting displayport is different from the one before, even if there wasn't one before. We could probably revise SetDisplayPortMargins to avoid doing that.

While I haven't tested this, I *think* it would make this patch unnecessay.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #18)
> I think what's actually happening is that SetDisplayPortMargins tries to
> unconditionally combine the new margins with the base rect to see if the
> resulting displayport is different from the one before, even if there wasn't
> one before. We could probably revise SetDisplayPortMargins to avoid doing
> that.
> 
> While I haven't tested this, I *think* it would make this patch unnecessay.

That's true. I'll try writing that patch instead.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8845769 [details] [diff] [review]
Set a base rect in MobileViewportManager::UpdateDisplayPortMargins

Review of attachment 8845769 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/MobileViewportManager.cpp
@@ +310,5 @@
> +    // We only create MobileViewportManager for root content documents. If that ever changes
> +    // we'd need to limit the size of this displayport base rect because non-toplevel documents
> +    // have no limit on their size.
> +    MOZ_ASSERT(mPresShell->GetPresContext()->IsRootContentDocument());
> +    nsLayoutUtils::SetDisplayPortBaseIfNotSet(root->GetContent(), displayportBase);

Unrelated to this change, but I notice that in the code you added to InitializeRootDisplayport() you handled the case where GetRootScrollFrame() is null; should we handle that case in this function, too?

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #18)
> While I haven't tested this, I *think* it would make this patch unnecessay.

It occurs to me that it may also make setting the base rect in InitializeRootDisplayport() unnecessary, although I guess doing so can't hurt.

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #20)
> Comment on attachment 8845769 [details] [diff] [review]
> Set a base rect in MobileViewportManager::UpdateDisplayPortMargins
> 
> Review of attachment 8845769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/MobileViewportManager.cpp
> @@ +310,5 @@
> > +    // We only create MobileViewportManager for root content documents. If that ever changes
> > +    // we'd need to limit the size of this displayport base rect because non-toplevel documents
> > +    // have no limit on their size.
> > +    MOZ_ASSERT(mPresShell->GetPresContext()->IsRootContentDocument());
> > +    nsLayoutUtils::SetDisplayPortBaseIfNotSet(root->GetContent(), displayportBase);
> 
> Unrelated to this change, but I notice that in the code you added to
> InitializeRootDisplayport() you handled the case where GetRootScrollFrame()
> is null; should we handle that case in this function, too?

We only set a displayport here if the root scroll frame is not-null. In InitializeRootDisplayport we set a displayport unconditionally.

If we change that then we can update the base rect calculation here at the same time.

Comment 23

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> We only set a displayport here if the root scroll frame is not-null. In
> InitializeRootDisplayport we set a displayport unconditionally.

Right; I'm asking if we should set a displayport in the case where the RSF is null. I don't really a have a good model of when that's the case.

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #21)
> (In reply to Botond Ballo [:botond] from comment #18)
> > While I haven't tested this, I *think* it would make this patch unnecessay.
> 
> It occurs to me that it may also make setting the base rect in
> InitializeRootDisplayport() unnecessary, although I guess doing so can't
> hurt.

Oh yeah, I guess because every other place that gets the display port also sets a base rect that would be okay (except image vis, which handles the no base rect case). I think it would be a good general practice to set a base rect whenever you set a displayport (if possible). Otherwise it'd be up to the displayport getters to do it, it'd be nice if the getters didn't have to be that sophisticated to have to do more work.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #23)
> (In reply to Timothy Nikkel (:tnikkel) from comment #22)
> > We only set a displayport here if the root scroll frame is not-null. In
> > InitializeRootDisplayport we set a displayport unconditionally.
> 
> Right; I'm asking if we should set a displayport in the case where the RSF
> is null. I don't really a have a good model of when that's the case.

Oh, okay. I guess we could do that, but I think that should be a followup. Probably not too important since it hasn't come up yet.

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #19)
> (In reply to Botond Ballo [:botond] from comment #18)
> > I think what's actually happening is that SetDisplayPortMargins tries to
> > unconditionally combine the new margins with the base rect to see if the
> > resulting displayport is different from the one before, even if there wasn't
> > one before. We could probably revise SetDisplayPortMargins to avoid doing
> > that.
> > 
> > While I haven't tested this, I *think* it would make this patch unnecessay.
> 
> That's true. I'll try writing that patch instead.

SetDisplayPortMargins actually compares against two different old display ports. The existing displayport (if there is one) and the displayport at the last image visibility update:

https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/layout/base/nsLayoutUtils.cpp#1325

So it's possible that we currently have no displayport, but there was a displayport at the last image visibility update, so we would need the current displayport rect in that case. So I guess we can't go this route.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> (In reply to Botond Ballo [:botond] from comment #23)
> > (In reply to Timothy Nikkel (:tnikkel) from comment #22)
> > > We only set a displayport here if the root scroll frame is not-null. In
> > > InitializeRootDisplayport we set a displayport unconditionally.
> > 
> > Right; I'm asking if we should set a displayport in the case where the RSF
> > is null. I don't really a have a good model of when that's the case.
> 
> Oh, okay. I guess we could do that, but I think that should be a followup.
> Probably not too important since it hasn't come up yet.

-> bug 1347052

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #26)
> (In reply to Timothy Nikkel (:tnikkel) from comment #19)
> > (In reply to Botond Ballo [:botond] from comment #18)
> > > I think what's actually happening is that SetDisplayPortMargins tries to
> > > unconditionally combine the new margins with the base rect to see if the
> > > resulting displayport is different from the one before, even if there wasn't
> > > one before. We could probably revise SetDisplayPortMargins to avoid doing
> > > that.
> > > 
> > > While I haven't tested this, I *think* it would make this patch unnecessay.
> > 
> > That's true. I'll try writing that patch instead.
> 
> SetDisplayPortMargins actually compares against two different old display
> ports. The existing displayport (if there is one) and the displayport at the
> last image visibility update:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> f9362554866b327700c7f9b18050d7b7eb3d2b23/layout/base/nsLayoutUtils.cpp#1325
> 
> So it's possible that we currently have no displayport, but there was a
> displayport at the last image visibility update, so we would need the
> current displayport rect in that case. So I guess we can't go this route.

Actually, if we had a displayport at the last image vis update then we should have had a base rect too. And we don't remove the base rect property, so even if the displayport went away the base rect property should remain. So this approach should be possible.

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: Allow SetDisplayPortMargins to work without asking for the base rect if it hasn't been set

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8845768 [details] [diff] [review]
Set a base rect before calling MaybeCreateDisplayport

Review of attachment 8845768 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for trying! This approach is fine, then.

I would still like to improve the documentation surrounding this a bit. A suggestion:

  - At the declaration of MaybeCreateDisplayPort, document that it
    requires that a base rect already be set, and why.

  - In this patch, the comment can then just say "set a base rect
    before calling MaybeCreateDisplayPort because it requires one"
    (and people can consult the declaration if they are curious why).

Comment 31

[Botond Ballo [:botond]]

Oh, whoops, I wrote comment 30 before seeing comment 28. Please ignore comment 30.

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8845768 [details] [diff] [review]
Set a base rect before calling MaybeCreateDisplayport

Review of attachment 8845768 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing, since we're going with an alternative approach, per comment 28.

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #24)
> (In reply to Botond Ballo [:botond] from comment #21)
> > (In reply to Botond Ballo [:botond] from comment #18)
> > > While I haven't tested this, I *think* it would make this patch unnecessay.
> > 
> > It occurs to me that it may also make setting the base rect in
> > InitializeRootDisplayport() unnecessary, although I guess doing so can't
> > hurt.
> 
> Oh yeah, I guess because every other place that gets the display port also
> sets a base rect that would be okay (except image vis, which handles the no
> base rect case). I think it would be a good general practice to set a base
> rect whenever you set a displayport (if possible). Otherwise it'd be up to
> the displayport getters to do it, it'd be nice if the getters didn't have to
> be that sophisticated to have to do more work.

The patch that modifies SetDisplayPortMargins to not calculate the displayport rect when there is no base rect set relies on the presence of displayport margins to indicate that a base rect is also present. So this is now a hard requirement.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d899491bd624
Set a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we set displayport margins. r=botond

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d899491bd624

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/941ddb8eb6ad
Set a displayport base rect in MobileViewportManager::UpdateDisplayPortMargins when we are setting displayport margins. r=botond

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/941ddb8eb6ad

Comment 38

[Timothy Nikkel (:tnikkel)]

Attachment: Call NotifyApproximateFrameVisibilityUpdate after we set a display port base rect

I realized that NotifyApproximateFrameVisibilityUpdate also gets the display port so it's silly to call it right before we set a base rect. And it also records the wrong information if we are ignoring the display port.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03d368159cb
Call NotifyApproximateFrameVisibilityUpdate after we set a display port base rect. r=botond

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c03d368159cb

Comment 41

[Timothy Nikkel (:tnikkel)]

Status of this bug: a try push found a couple more instances of the assert triggering. It came from js code calling a function that scrolled a non-root scroll frame. This triggers creating display port margins. Since it's not a root scrollframe I couldn't think of any good way to set a displayport base in this case (or in general). We might just have to live with a few of these. The important thing is that in the vast vast majority of normal cases we have a good displayport base rect set.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:tnikkel, maybe it's time to close this bug?

Comment 43

[Kartikaya Gupta (email:[e-mail])]

No point in keeping this bug open.