1073219 - Remove not-really-dangerous public destructor of MaskLayerImageKey

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andrew McCreight [:mccr8]]

MaskLayerImageKey has AddRef and Release methods, but all they do is increment or decrement a counter, so having the destructor be public is not dangerous in the way it is for most classes.

However, I'd argue that using AddRef and Release in this way, in combination with nsRefPtr<>, is kind of an abusive pun: MaskLayerUserData holds an nsRefPtr to an image key, but does not actually own it in any way.  I think this should be changed so it is more explicit what is going on.  AFAICT MarkLayerUserData is the only place that takes advantage of the AddRef/Release so that could be fixed up with some kind of getter setter thing.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Is it ever possible that we delete MaskLayerImageKey, while an nsRefPtr points to it?

Comment 2

[Daniel Holbert [:dholbert]]

Note that we have...
> 4654   nsAutoPtr<MaskLayerImageCache::MaskLayerImageKey> newKey(
> 4655     new MaskLayerImageCache::MaskLayerImageKey());
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#4654

...which will fail to compile if we remove the public ~MaskLayerImageKey() destructor. That code will need to be rewritten, if & when we remove this public destructor.

Adding dependency on bug 757347, which added this class. Bug 757347 comment 34 discusses/explains the odd nsRefPtr/AddRef/Release usage here.

Comment 3

[Andrew McCreight [:mccr8]]

My proposal is basically to rename AddRef/Release to something else, then manually invoke them in the user data thing, so this isn't a fake refcounted class any more.  Then we can leave the public destructor as is, because it won't be dangerous-looking any more.  The nsAutoPtr can stay as it is.

Comment 4

[Daniel Holbert [:dholbert]]

If possible, it might be better to replace nsRefPtr with some other similar RAII container. (Better than just invoking AddRefThing/ReleaseThing manually)

We really are doing a form of "reference counting" here, so something like nsRefPtr does kinda makes sense for maintaining the counts. We're just managing cleanup in a different way (using an explicit "sweep" step).

> MaskLayerUserData holds an nsRefPtr to an image key, but does not actually own it in any way.

It does sorta own it (or own a "share" of it), in that its reference keeps the image key alive. We just happen to know that it will never get to delete it.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use an nsRefPtr-like smart pointer wrapper to remove the HasDangerousPublicDestructor specialization for MaskLayerImageKey

Comment 6

[Daniel Holbert [:dholbert]]

Thanks for taking this!

But, hmm, this whole new MaskLayerImageKey is a lot of boilerplate (copypasted from nsRefPtr, probably?) -- and that's kind of bad, because
 (1) most of it is probably unused
 (2) it'll be doomed to fall out of sync with nsRefPtr, as we make improvements/tweaks to nsRefPtr without updating this class.

In comment 4, I think I was hoping there could be a simpler RAII class to manage this count, with less code to maintain. Can we cut down on the functions in this class? (i.e. do we really need operator= methods, or can we just mark those as "delete"? Do we really need all the different constructors here?  Do we actually need operator* and operator-> ?)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> But, hmm, this whole new MaskLayerImageKey is a lot of boilerplate

(er sorry, I meant to say "MaskLayerImageKeyRef" -- that's the name of the new class in the patch)

I think this new RAII struct purely exists to maintain a count, from skimming its usages.  If I'm not missing something, I'd expect it only needs the following:

 (1) Trivial constructor, initializing mRawPtr to null.

 (2) "Init()" method, which takes a MaskLayerImageKey* and puts that in mRawPtr, & invokes IncLayerCount on it. (Also, assert if we already had a non-null mRawPtr, to catch double-Init. We probably don't expect this in practice, so no need to actually handle this case, I think (?)).

 (3) Pretty-trivial destructor, invoking DecLayerCount() on its pointer (if non-null).

 (4) explicitly-deleted operator= and operator==, to avoid misusage.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
>  (4) explicitly-deleted operator= and operator==, to avoid misusage.

(and explicitly-deleted copy constructor as well -- see this snippet for reference:
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFontInflationData.h?rev=203acd3b8d71&mark=43-44#43

The equality operator== doesn't matter as much, really; that one just came to mind because it was part of the boilerplate and it'd be an odd thing for someone to invoke on this RAII struct.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8630818 [details] [diff] [review]
Use an nsRefPtr-like smart pointer wrapper to remove the HasDangerousPublicDestructor specialization for MaskLayerImageKey

Marking patch r- for now, per comment 6 and 7.  (This is close to r=me, though, if something simpler as discussed above ends up being feasible.)

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use a basic wrapper instead of nsRefPtr to manage mLayerCount for MaskLayerImageKey

This is a very stripped down wrapper. When I wrote the patch the first time, I didn't bother to look at the users of MaskLayerImageKeyRef to see how they used them for some reason.
I still have one unused method (get()), but I decided to leave it in, because it may be useful to be able to get the MaskLayerImageKey out of the MaskLayerImageKeyRef in the future.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8631574 [details] [diff] [review]
Use a basic wrapper instead of nsRefPtr to manage mLayerCount for MaskLayerImageKey

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1379,5 @@
>             mOffset == aOther.mOffset &&
>             mAppUnitsPerDevPixel == aOther.mAppUnitsPerDevPixel;
>    }
>  
> +  MaskLayerImageCache::MaskLayerImageKeyRef mImageKey;

Please add a brief comment for this object while you're here (particularly now that its type is changing to be something more obscure).

::: layout/base/MaskLayerImageCache.h
@@ +164,5 @@
>    };
>  
> +  /**
> +   * This is an object which holds a reference to a MaskLayerImageKey. When the
> +   * value inside the MasklayerImageKeyRef is updated, it will automatically manage the

s/updated/initialized/ (since this structure doesn't need to support updates; just a single initialization, per notes about assertions below)

@@ +181,5 @@
> +    MaskLayerImageKeyRef() : mRawPtr(nullptr) {}
> +    MaskLayerImageKeyRef(const MaskLayerImageKeyRef&) = delete;
> +    void operator=(const MaskLayerImageKeyRef&) = delete;
> +
> +    void set(const MaskLayerImageKey* aPtr)

Call this "Init", since it's only meant to be called once, and it's an initialization step.

("set" implies that it could be called multiple times to set us to different things, but we don't expect that to happen.)

(Note that "Init" is capitalized - gecko code generally uses capital method-names; not sure why nsRefPtr is an exception, but I wouldn't mimic it. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods )

@@ +184,5 @@
> +
> +    void set(const MaskLayerImageKey* aPtr)
> +    {
> +      if (aPtr) {
> +        aPtr->IncLayerCount();

No need for this "if" check I think -- just MOZ_ASSERT that "aPtr" is non-null here.

@@ +187,5 @@
> +      if (aPtr) {
> +        aPtr->IncLayerCount();
> +      }
> +      if (mRawPtr) {
> +        mRawPtr->DecLayerCount();

Similarly, I think there's no need to handle mRawPtr being nonnull at this point -- just assert that it's nonnull.

@@ +193,5 @@
> +      mRawPtr = aPtr;
> +    }
> +
> +    const MaskLayerImageKey* get()
> +    {

And let's not expose a getter -- this object isn't meant to be queryable. It's just meant to manage a count. Let's keep it focused on that.

Comment 12

[Daniel Holbert [:dholbert]]

(RE my suggested assertion in Init() (formerly "set") -- I'm suggesting these assertions based on code inspection of ContainerState::CreateMaskLayer(), the only caller, and I'm pretty sure they should be valid based on that.  Do let me know if I'm mistaken though.)

Comment 13

[Daniel Holbert [:dholbert]]

(TANGENT: for stricter guarantees here, it'd ultimately be nice if we made IncLayerCount, DecLayerCount, and mLayerCount private, and only gave your new MaskLayerImageKeyRef struct the ability to call them. [and exposed a new "MaskLayerImageKey::HasZeroLayerCount" accessor for use at the one other place we inspect the layer count right now, in Sweep.  If this makes sense to you, & you feel like doing this in a followup patch/bug, great! No pressure though.)

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> (RE my suggested assertion in Init() (formerly "set") -- I'm suggesting
> these assertions based on code inspection of
> ContainerState::CreateMaskLayer(), the only caller, and I'm pretty sure they
> should be valid based on that.  Do let me know if I'm mistaken though.)

I'll try what you're suggesting & push to try. The reason why I am slightly skeptical is because the CreateMaskLayer function calls a function called CreateOrRecycleMaskImageLayerFor, which could recycle the value without creating a new MaskLayerImageData struct. Under that situation, the reference could be re-assigned to.

I'll leave the assertions in for a try push, though. If it MOZ_ASSERTs because it is being re-assigned to, I'll add back in the handling of double-initialization.

Comment 15

[Daniel Holbert [:dholbert]]

Hmm, you might be right... I think I misread "if (*userData == newData)" as "userData = newData" before, and was thinking we'd only be working with new user-data structs here.

I don't totally follow the lifetime of these MaskLayerImageData objects -- I'm hoping the assertion should be sufficient to tell you if you need to support reassignment in your RAII class. Alternately (or if it's unclear from your Try run), nrc (who wrote this code) may be able to help, too.

(If you find that you do need to support reassignment, I think I'd marginally prefer "Reset()" over "Set()", I think.)

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use a basic wrapper instead of nsRefPtr to manage mLayerCount for MaskLayerImageKey

I was correct, these objects are re-used. I have changed the name to Reset, and handle that case correctly.

failing log due to objects being re-used: https://treeherder.mozilla.org/logviewer.html#?job_id=9272801&repo=try

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46afc1f50209

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8632402 [details] [diff] [review]
Use a basic wrapper instead of nsRefPtr to manage mLayerCount for MaskLayerImageKey

Looks great! Just 3 language tweaks, noted below:

>Bug 1073219 - Use a basic wrapper instead of nsRefPtr to manage mLayerCount for MaskLayerImageKey

"basic wrapper" is a bit vague perhaps. How about "simple RAII struct"?

>+++ b/layout/base/FrameLayerBuilder.cpp
> 
>-  nsRefPtr<const MaskLayerImageCache::MaskLayerImageKey> mImageKey;
>+  // The MaskLayerImageKeyRef is a object which maintains the mLayerCount property
>+  // on the MaskLayerImageKey. It will be initialized with a value, which will increment
>+  // mLayerCount on the key, and then when this object is destroyed, the count will
>+  // decrement again.
>+  MaskLayerImageCache::MaskLayerImageKeyRef mImageKey;

The prose is a bit confusing here - e.g. in the first time I read this, I thought "this object" was referring to the same "a value" that was mentioned just before. (e.g. I read it as saying "it will be initialized with a value [...] and then when that value is destroyed, the count will decrement again", which doesn't really make sense & isn't what you're trying to say)

This is probably too much information here anyway -- let's just make this a one-liner, something like this:

  // Keeps a MaskLayerImageKey alive by managing its mLayerCount member-var:
  MaskLayerImageCache::MaskLayerImageKeyRef mImageKey;

Anyone who wants to learn more can just look at the MaskLayerImageKeyRef struct-definition's  documentation.

>+++ b/layout/base/MaskLayerImageCache.cpp
>+  /**
>+   * This is an object which holds a reference to a MaskLayerImageKey. When the
>+   * value inside the MasklayerImageKeyRef is initialized, it will automatically manage the
>+   * MaskLayerImageKey's mLayerCount value (in much the same way as a smart pointer like
>+   * nsRefPtr manages mRefCnt).
>+   */
>+  struct MaskLayerImageKeyRef
>+  {

The documentation needs some tweaks here:
 (1) it doesn't really explain what's different from standard refcounting (why are we using this instead of nsRefPtr?)
 (2) "automatically manage" is maybe a bit too magical
 (3) There are lines here that are longer than > 80 characters -- Gecko coding style is to limit lines to 80 characters:
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style


Suggested alternative text:

  /**
   * This struct maintains a reference to a MaskLayerImageKey, via a variant on
   * refcounting. When a key is passed in via Reset(), we increment the
   * passed-in key's mLayerCount, and we decrement its mLayerCount when we're
   * destructed (or when the key is replaced via a second Reset() call).
   *
   * However, unlike standard refcounting smart-pointers, this object does
   * *not* delete the tracked MaskLayerImageKey -- instead, deletion happens
   * in MaskLayerImageCache::Sweep(), for any keys whose mLayerCount is 0.
   */

r=me with those comment tweaks. Thanks!

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
>    * However, unlike standard refcounting smart-pointers, this object does
>    * *not* delete the tracked MaskLayerImageKey -- instead, deletion happens

(er, correcting myself -- maybe s/this object/this struct/ here, to be clearer about what we're talking about & avoid the same ambiguity I mentioned earlier about "this object" in the other comment. :))

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use a simple RAII struct instead of nsRefPtr to manage mLayerCount for MaskLayerImageKey

Fixed wording of comments.

Comment 20

[Daniel Holbert [:dholbert]]

(note: it's easier for checkin-needed sheriffs if you have "r=dholbert" [or whoever] already in your commit message, when you post a patch w/ checkin-needed -- otherwise the sheriff has to notice & edit the commit message themselves.  Many people just start out with "r=$FOO" in their commit messages, optimistically, even before the patch has gone through review, to save this extra step at the end -- you might want to consider doing this as well.)

Added r=dholbert & landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/362518587479

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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