Closed Bug 1029141 Opened 6 years ago Closed 6 years ago

Remove dangerous public destructor of SVGTextFrame::MutationObserver

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bjacob, Assigned: anujagarwal464)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Flags: needinfo?(anujagarwal464)
Component: Layout → SVG
Assignee: nobody → anujagarwal464
Flags: needinfo?(anujagarwal464)
Attached patch Bug1029141.diff (obsolete) — Splinter Review
Attachment #8448192 - Flags: feedback?(continuation)
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.
Attachment #8448192 - Flags: feedback?(continuation) → feedback+
Attached patch Bug1029141.diff (obsolete) — Splinter Review
Thanks! :)
Attachment #8448192 - Attachment is obsolete: true
Attachment #8448773 - Flags: feedback?(continuation)
Attachment #8448773 - Flags: review?(dholbert)
Attachment #8448773 - Flags: feedback?(continuation)
Attachment #8448773 - Flags: feedback+
Version: Other Branch → Trunk
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.
Attachment #8448773 - Flags: review?(dholbert) → feedback+
Attached patch Bug1029141.diff (obsolete) — Splinter Review
Attachment #8448773 - Attachment is obsolete: true
Attachment #8449572 - Flags: review?(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!
Attachment #8449572 - Flags: review?(dholbert) → review+
(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...")
Attached patch Bug1029141.diff (obsolete) — Splinter Review
@dholbert: Thank you!
Attachment #8449572 - Attachment is obsolete: true
Attachment #8449635 - Flags: review?(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)
Attachment #8449635 - Flags: review?(dholbert) → review+
No longer depends on: 1028588
Anuj, is this patch ready for a try run?  Do you have try access?
Flags: needinfo?(anujagarwal464)
@Andrew, Local build was successful for me. Yes patch is ready for try run. I don't have try access.
Flags: needinfo?(anujagarwal464)
Attachment #8449635 - Attachment is obsolete: true
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
Try run is green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5900f66edaf9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.