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

NEW
Assigned to

Status

()

Core
Panning and Zooming
P3
normal
9 months ago
6 months ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

({leave-open})

Trunk
leave-open
Points:
---

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [gfx-noted])

Attachments

(10 attachments)

3.03 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.21 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.02 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.26 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.90 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.84 KB, patch
botond
: review-
Details | Diff | Splinter Review
1.60 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.63 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.18 KB, patch
botond
: review+
Details | Diff | Splinter Review
7.93 KB, patch
botond
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 months ago
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.
(Assignee)

Updated

9 months ago
QA Contact: tnikkel
(Assignee)

Updated

9 months ago
Assignee: nobody → tnikkel
QA Contact: tnikkel
(Assignee)

Comment 1

9 months ago
Created attachment 8845762 [details] [diff] [review]
Factor out a GetDisplayPortData function
Attachment #8845762 - Flags: review?(botond)
(Assignee)

Comment 2

9 months ago
Created attachment 8845763 [details] [diff] [review]
Add a function that says if we are missing a base rect
Attachment #8845763 - Flags: review?(botond)
(Assignee)

Comment 3

9 months ago
Created attachment 8845764 [details] [diff] [review]
Allow asking a scrollframe if it's root.
Attachment #8845764 - Flags: review?(botond)
(Assignee)

Comment 4

9 months ago
Created attachment 8845766 [details] [diff] [review]
Set a base rect in image visibility (or ignore the displayport)
Attachment #8845766 - Flags: review?(botond)
(Assignee)

Comment 5

9 months ago
Created attachment 8845767 [details] [diff] [review]
Set a base rect in APZCCallbackHelper::InitializeRootDisplayport
Attachment #8845767 - Flags: review?(botond)
(Assignee)

Comment 6

9 months ago
Created attachment 8845768 [details] [diff] [review]
Set a base rect before calling MaybeCreateDisplayport
Attachment #8845768 - Flags: review?(botond)
(Assignee)

Comment 7

9 months ago
Created attachment 8845769 [details] [diff] [review]
Set a base rect in MobileViewportManager::UpdateDisplayPortMargins
Attachment #8845769 - Flags: review?(botond)
(Assignee)

Comment 8

9 months ago
Created attachment 8845770 [details] [diff] [review]
Make it an assert
Attachment #8845770 - Flags: review?(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|
Attachment #8845762 - Flags: review?(botond) → review+

Updated

9 months ago
Attachment #8845763 - Flags: review?(botond) → review+
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.
Attachment #8845764 - Flags: review?(botond) → review+
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 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?
Attachment #8845767 - Flags: review?(botond) → review+
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.
Attachment #8845766 - Flags: review?(botond) → review+
(Assignee)

Comment 14

9 months ago
(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...".
(Assignee)

Comment 15

9 months ago
(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.
(Assignee)

Updated

9 months ago
Keywords: leave-open

Comment 16

9 months ago
Pushed by tnikkel@gmail.com:
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

9 months ago
bugherder
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
Priority: -- → P3
Whiteboard: [gfx-noted]
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.
(Assignee)

Comment 19

9 months ago
(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 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?
Attachment #8845769 - Flags: review?(botond) → review+

Updated

9 months ago
Attachment #8845770 - Flags: review?(botond) → review+
(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.
(Assignee)

Comment 22

9 months ago
(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.
(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.
(Assignee)

Comment 24

9 months ago
(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.
(Assignee)

Comment 25

9 months ago
(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.
(Assignee)

Comment 26

9 months ago
(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.
(Assignee)

Comment 27

9 months ago
(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
(Assignee)

Comment 28

9 months ago
(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.
(Assignee)

Comment 29

9 months ago
Created attachment 8846997 [details] [diff] [review]
Allow SetDisplayPortMargins to work without asking for the base rect if it hasn't been set
Attachment #8846997 - Flags: review?(botond)
Comment hidden (typo)
Oh, whoops, I wrote comment 30 before seeing comment 28. Please ignore comment 30.

Updated

9 months ago
Attachment #8846997 - Flags: review?(botond) → review+
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.
Attachment #8845768 - Flags: review+ → review-
(Assignee)

Comment 33

9 months ago
(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

9 months ago
Pushed by tnikkel@gmail.com:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d899491bd624

Comment 36

9 months ago
Pushed by tnikkel@gmail.com:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/941ddb8eb6ad
(Assignee)

Comment 38

9 months ago
Created attachment 8849852 [details] [diff] [review]
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.
Attachment #8849852 - Flags: review?(botond)

Updated

9 months ago
Attachment #8849852 - Flags: review?(botond) → review+

Comment 39

8 months ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03d368159cb
Call NotifyApproximateFrameVisibilityUpdate after we set a display port base rect. r=botond

Comment 40

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c03d368159cb
(Assignee)

Comment 41

6 months ago
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.
You need to log in before you can comment on or make changes to this bug.