812908 - Add a debug pref to force all layers to be active

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

It would be nice for debugging to be able to force all layers to be active, so we can see whether an error is apparent for active or inactive layers or both and to narrow down errors which involve making layers active (e.g., OMTA). Also it makes it easier to work on functionality which is only present for active layers (e.g., mask layers) without having to fiddle with CSS for many elements.

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch

Comment 2

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=5b9416c93ddc (with the pref off, I suspect we would need a lot of fuzz to pass with the pref on)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 682917 [details] [diff] [review]
patch

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

Why don't we just fix it in FrameLayerBuilder where GetLayerState is called?

And why go to ACTIVE_FORCE? Why not just ACTIVE?

Comment 4

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 682917 [details] [diff] [review]
> patch
> 
> Review of attachment 682917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why don't we just fix it in FrameLayerBuilder where GetLayerState is called?

Because some display items sometimes can't be made active, we get crashes, and it is hard to tell this from outside the display item. Also, it seems wrong to expect future users of GetLayerState to add the check wherever it is used.

> 
> And why go to ACTIVE_FORCE? Why not just ACTIVE?

Because if it is just ACTIVE ProcessDisplayItem might still choose to not make the layer active (if forceInactive is true), and I want the layers to be active whenever it is possible.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron [:nrc] from comment #4)
> Because some display items sometimes can't be made active, we get crashes,
> and it is hard to tell this from outside the display item.

What's an example of that?

> Also, it seems
> wrong to expect future users of GetLayerState to add the check wherever it
> is used.

We are more likely to add more implementations of GetLayerState than more callers of GetLayerState, so one could argue that it is wrong to expect future implementations to add the check.

One thing at least you should do is simplify the code by having ForceActiveLayers() exist always but return false if #ifndef DEBUG. Although I actually don't see why this needs to be #ifdef DEBUG if ForceActiveLayers uses AddBoolVarCache.

> > And why go to ACTIVE_FORCE? Why not just ACTIVE?
> 
> Because if it is just ACTIVE ProcessDisplayItem might still choose to not
> make the layer active (if forceInactive is true), and I want the layers to
> be active whenever it is possible.

OK.

Comment 6

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> (In reply to Nick Cameron [:nrc] from comment #4)
> > Because some display items sometimes can't be made active, we get crashes,
> > and it is hard to tell this from outside the display item.
> 
> What's an example of that?
> 

I think, an nsDisplayBackgroundImage, before the image container is ready.

> > Also, it seems
> > wrong to expect future users of GetLayerState to add the check wherever it
> > is used.
> 
> We are more likely to add more implementations of GetLayerState than more
> callers of GetLayerState, so one could argue that it is wrong to expect
> future implementations to add the check.
> 
> One thing at least you should do is simplify the code by having
> ForceActiveLayers() exist always but return false if #ifndef DEBUG. Although
> I actually don't see why this needs to be #ifdef DEBUG if ForceActiveLayers
> uses AddBoolVarCache.
> 

I wanted to not cache it so I could turn it on and off without restarting (now I think about it, I had assumed these things are connected in that way, is that right?). I also worry about exposing this in release builds because it might introduce bugs that we wouldn't otherwise see/care about.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you use AddBoolVarCache it will update dynamically.

(In reply to Nick Cameron [:nrc] from comment #6)
> I think, an nsDisplayBackgroundImage, before the image container is ready.

It sounds like you were trying to convert LAYER_NONE to LAYER_ACTIVE/LAYER_ACTIVE_FORCE. I think you would just try converting from LAYER_INACTIVE to LAYER_ACTIVE.

Comment 8

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> If you use AddBoolVarCache it will update dynamically.
> 
> (In reply to Nick Cameron [:nrc] from comment #6)
> > I think, an nsDisplayBackgroundImage, before the image container is ready.
> 
> It sounds like you were trying to convert LAYER_NONE to
> LAYER_ACTIVE/LAYER_ACTIVE_FORCE. I think you would just try converting from
> LAYER_INACTIVE to LAYER_ACTIVE.

Yes, it is trying to convert LAYER_NONE to LAYER_ACTIVE/LAYER_ACTIVE_FORCE which is the issue, that is what I try to avoid doing in this code. I could check what is returned by GetLayerState to avoid LAYERS_NONE.

Is it the case that any display item with layer state inactive can be made active? I.e., there will never be a display item which is only inactive and has issues if it is active?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron [:nrc] from comment #8)
> Is it the case that any display item with layer state inactive can be made
> active? I.e., there will never be a display item which is only inactive and
> has issues if it is active?

I think so. I could be wrong, but I wouldn't worry about it since this option is only for debugging.

Comment 10

[Nick Cameron [:nrc]]

Attachment: patch with check on use of GetLayerState, rather than in def

Comment 11

[Nick Cameron [:nrc]]

This still has ForceActiveLayers guarded by DEBUG, similar to paint list dumping etc. Would you rather I remove the ifdefs?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 684268 [details] [diff] [review]
patch with check on use of GetLayerState, rather than in def

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1042,5 @@
>      case LAYER_INACTIVE:
> +#ifdef DEBUG
> +      if (nsDisplayItem::ForceActiveLayers()) {
> +        layerState = "LAYER_ACTIVE";
> +        break;

Why do we need this?

@@ +2040,5 @@
> +#ifdef DEBUG
> +    if (layerState == LAYER_INACTIVE &&
> +        nsDisplayItem::ForceActiveLayers()) {
> +      layerState = LAYER_ACTIVE;
> +    }

Make this code not #ifdefed and make ForceActiveLayers always return false in non-DEBUG. Or, just enable this feature for non-DEBUG builds as well. I prefer the latter.

::: layout/base/nsDisplayList.cpp
@@ +1412,5 @@
> +#ifdef DEBUG
> +/* static */ bool
> +nsDisplayItem::ForceActiveLayers()
> +{
> +  return Preferences::GetBool("layers.force-active", false);

Use AddBoolVarCache

@@ +2567,5 @@
>                                                   nsIFrame* aActiveScrolledRoot) {
> +#ifdef DEBUG
> +  if (nsDisplayItem::ForceActiveLayers()) {
> +    return LAYER_ACTIVE;
> +  }

I don't think you need this anymore.

Comment 13

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 684268 [details] [diff] [review]
> patch with check on use of GetLayerState, rather than in def
> 
> Review of attachment 684268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1042,5 @@
> >      case LAYER_INACTIVE:
> > +#ifdef DEBUG
> > +      if (nsDisplayItem::ForceActiveLayers()) {
> > +        layerState = "LAYER_ACTIVE";
> > +        break;
> 
> Why do we need this?
> 

I didn't want the debug dump to be misleading if we are using active layers.

> @@ +2040,5 @@
> > +#ifdef DEBUG
> > +    if (layerState == LAYER_INACTIVE &&
> > +        nsDisplayItem::ForceActiveLayers()) {
> > +      layerState = LAYER_ACTIVE;
> > +    }
> 
> Make this code not #ifdefed and make ForceActiveLayers always return false
> in non-DEBUG. Or, just enable this feature for non-DEBUG builds as well. I
> prefer the latter.
> 

OK, removed all the ifdefs, except the one on the pref in all.js

> ::: layout/base/nsDisplayList.cpp
> @@ +1412,5 @@
> > +#ifdef DEBUG
> > +/* static */ bool
> > +nsDisplayItem::ForceActiveLayers()
> > +{
> > +  return Preferences::GetBool("layers.force-active", false);
> 
> Use AddBoolVarCache
> 

done

> @@ +2567,5 @@
> >                                                   nsIFrame* aActiveScrolledRoot) {
> > +#ifdef DEBUG
> > +  if (nsDisplayItem::ForceActiveLayers()) {
> > +    return LAYER_ACTIVE;
> > +  }
> 
> I don't think you need this anymore.

You are right; gone.

Comment 14

[Nick Cameron [:nrc]]

Attachment: patch

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 684341 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1041,5 @@
>        layerState = "LAYER_NONE"; break;
>      case LAYER_INACTIVE:
> +      if (nsDisplayItem::ForceActiveLayers()) {
> +        layerState = "LAYER_ACTIVE (forced)";
> +        break;

Won't mLayerState already have been forced to LAYER_ACTIVE in this case?

::: modules/libpref/src/init/all.js
@@ +3703,5 @@
>  
> +// Force all possible layers to be always active layers
> +#ifdef DEBUG
> +pref("layers.force-active", false);
> +#endif

Why does this have to be #ifdef DEBUG?

Comment 16

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Comment on attachment 684341 [details] [diff] [review]
> patch
> 
> Review of attachment 684341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1041,5 @@
> >        layerState = "LAYER_NONE"; break;
> >      case LAYER_INACTIVE:
> > +      if (nsDisplayItem::ForceActiveLayers()) {
> > +        layerState = "LAYER_ACTIVE (forced)";
> > +        break;
> 
> Won't mLayerState already have been forced to LAYER_ACTIVE in this case?
> 
Yes, d'oh.

> ::: modules/libpref/src/init/all.js
> @@ +3703,5 @@
> >  
> > +// Force all possible layers to be always active layers
> > +#ifdef DEBUG
> > +pref("layers.force-active", false);
> > +#endif
> 
> Why does this have to be #ifdef DEBUG?

I didn't think we should expose potentially buggy stuff in non-debug builds, although I guess there are plenty of other prefs that expose buggy behaviour. Do you prefer it to be not ifdefed?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 18

[Nick Cameron [:nrc]]

Attachment: patch

Comment 19

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=25ab5381a8e0

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25ab5381a8e0