1029141 - Remove dangerous public destructor of SVGTextFrame::MutationObserver

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

In bug 1028588 we removed dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist.

One of them is: SVGTextFrame::MutationObserver

Comment 2

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug1029141.diff

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8448192 [details] [diff] [review]
Bug1029141.diff

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

This looks good to me.  While you are here, you should probably make MutationObserver final by adding MOZ_FINAL:
 class MutationObserver MOZ_FINAL : 
I'd ask dholbert for review when you are ready.

Comment 4

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug1029141.diff

Thanks! :)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8448773 [details] [diff] [review]
Bug1029141.diff

Thanks for the patch!

>@@ -3162,17 +3162,17 @@ SVGTextFrame::Init(nsIContent*       aCo
>                    nsIFrame*         aPrevInFlow)
> {
[...]
>-  mMutationObserver.StartObserving(this);
>+  mMutationObserver->StartObserving(this);
> }
[...]
>diff --git a/layout/svg/SVGTextFrame.h b/layout/svg/SVGTextFrame.h
>   SVGTextFrame(nsStyleContext* aContext)
>     : SVGTextFrameBase(aContext),
>       mFontSizeScaleFactor(1.0f),
>       mLastContextScale(1.0f),
>       mLengthAdjustScaleFactor(1.0f)
>   {
>     AddStateBits(NS_STATE_SVG_POSITIONING_DIRTY);
>+    mMutationObserver = new MutationObserver();
>   }

Instead of allocating a MutationObserver() in the constructor and then enabling it in the Init() method, let's just do everything in Init().

In fact, that means we can drop "MutationObserver::StartObserving()" altogether, and just merge its implementation into the MutationObserver() constructor.  (So, MutationObserver() would now take a single param, "SVGTextFrame* aFrame", and its init list would set mFrame to that instead of to nullptr, and then we'd call AddMutationObserver())

>+  class MutationObserver MOZ_FINAL : public nsStubMutationObserver {
>   public:
>     MutationObserver()
>       : mFrame(nullptr)
>     {
>     }
> 
>     void StartObserving(SVGTextFrame* aFrame)
>     {
>       NS_ASSERTION(!mFrame, "should not be observing yet!");
>       mFrame = aFrame;
>       aFrame->GetContent()->AddMutationObserver(this);
>     }

(so, per above, these methods ^^ should just be collapsed together.)

>-    virtual ~MutationObserver()
>-    {

I don't think this needs to be virtual. The only thing deleting us now should be our ::Release() method, which knows our concrete type.  Maybe that should happen in a separate patch (or bug), but if you'd like to do it here, I'd support that.

Holding off on r+ since I'd like to see the final patch before it lands.

Comment 6

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug1029141.diff

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8449572 [details] [diff] [review]
Bug1029141.diff

>Bug 1029141 - Removed dangerous public destructor of SVGTextFrame::MutationObserver.

Maybe: "Refactor SVGTextFrame::MutationObserver, de-virtualizing & privatizing its public destructor.

Since...
 - ...we tend to use the present tense for commit messages. (so, not "removed")
 - ...this isn't so much *removing* the destructor as just changing it to be private (and non-virtual)
 - ...there's some other refactoring going on here as well.

>diff --git a/layout/svg/SVGTextFrame.h b/layout/svg/SVGTextFrame.h
>+    MutationObserver(SVGTextFrame* aFrame)
>+      : mFrame(aFrame)
>     {
>       NS_ASSERTION(!mFrame, "should not be observing yet!");

This assertion is no longer valid. (We'll have done the assignment already, via the init list, so it would fail.)

Let's just change this to assert the opposite (so, drop the "!") -- with message e.g. "MutationObserver needs a non-null frame". This should also now probably be MOZ_ASSERT (which is fatal), since if this new assertion fails, we're going to crash anyway, so we might as well do so proactively with a useful message in debug builds.

>       aFrame->GetContent()->AddMutationObserver(this);

While you're here, let's change this to use mFrame (instead of aFrame), to make this more symmetrical with the RemoveMutationObserver invocation in the destructor.

(aFrame and mFrame have the same value, so it doesn't *really* matter which one we use, but it's more readable if the two Add/RemoveMutationObserver invocations are consistent.)

>   private:
>+    ~MutationObserver()
>+    {
>+      if (mFrame) {
>+        mFrame->GetContent()->RemoveMutationObserver(this);
>+      }

We no longer need this null-check, now that we're unconditionally setting mFrame to something non-null in the constructor. So, remove this null-check.

>+
>     SVGTextFrame* mFrame;

Please change this to...
    SVGTextFrame* const mFrame;
(This ensures that we don't adjust this pointer-value is locked in after we set it in the constructor.) It wasn't able to be 'const' before, because we were setting the value in a method, but now that it's set in the init-list, the pointer can be const.

r=me with the above. Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Please change this to...
>     SVGTextFrame* const mFrame;
> (This ensures that we don't adjust this pointer-value is locked in after we
> set it in the constructor.)

(sorry, editing fail. meant to delete "we don't adjust" there. It should've said: "This ensures that this pointer-value is locked in...")

Comment 9

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug1029141.diff

@dholbert: Thank you!

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8449635 [details] [diff] [review]
Bug1029141.diff

Looks good! I think you just missed one nit:

>+    MutationObserver(SVGTextFrame* aFrame)
>+      : mFrame(aFrame)
>     {
[...]
>       aFrame->GetContent()->AddMutationObserver(this);

As noted in comment 7, let's use mFrame instead of aFrame for this call, for consistency with the later call to RemoveMutationObserver.

r=me with that changed. (no need to re-request review when posting the updated patch)

Comment 11

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug1029141.diff r=dholbert.

Comment 12

[Andrew McCreight [:mccr8]]

Anuj, is this patch ready for a try run?  Do you have try access?

Comment 13

[Anuj Agarwal [:anujagarwal464]]

@Andrew, Local build was successful for me. Yes patch is ready for try run. I don't have try access.

Comment 14

[Andrew McCreight [:mccr8]]

Anuj, let me know if you would like try access. There's a little bit of paperwork involved, but it isn't too bad.

try run: https://tbpl.mozilla.org/?tree=Try&rev=439ff60ec33c

Comment 15

[Andrew McCreight [:mccr8]]

Try run is green.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5900f66edaf9

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5900f66edaf9