Closed Bug 1338443 Opened 3 years ago Closed 3 years ago

Convert nsAutoFloatManager::new to use UniquePtr

Categories

(Core :: Layout: Floats, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(2 files)

ReflowInput, BlockReflowInput, and nsLineLayout have nsFloatManager* mFloatManager member in them, the life cycle is not obvious in the first glance. 

The life time of the nsFloatManager is controlled by nsAutoFloatManager, and the member |mNew| seems a good candidate to be converted to UniquePtr.

I want to dig a bit, and see if I can improve something.

[1] http://searchfox.org/mozilla-central/search?q=mFloatManager&case=false&regexp=false&path=*.h
[2] http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/layout/generic/nsFloatManager.cpp#983,993
Comment on attachment 8835877 [details]
Bug 1338443 Part 1 - Use ReflowInput's float manager in BlockReflowInput.

https://reviewboard.mozilla.org/r/111450/#review113044

Thanks for cleaning this up!

::: layout/generic/BlockReflowInput.h:164
(Diff revision 1)
>                        nsIFrame *aReplacedBlock = nullptr,
>                        uint32_t aFlags = 0);
>  
> +  nsFloatManager* FloatManager() const {
> +    MOZ_ASSERT(mReflowInput.mFloatManager,
> +               "Float manager should be valid during the life-time of "

Nit: no need for a hyphen in "lifetime".
Attachment #8835877 - Flags: review?(dholbert) → review+
Comment on attachment 8835878 [details]
Bug 1338443 Part 2 - Convert nsAutoFloatManager::mNew to use UniquePtr.

https://reviewboard.mozilla.org/r/111452/#review113046

r=me, with nits addressed:

::: layout/generic/ReflowInput.h:341
(Diff revision 1)
>    // the reflow states are linked together. this is the pointer to the
>    // parent's reflow state
>    const ReflowInput* mParentReflowInput;
>  
>    // pointer to the float manager associated with this area
> -  nsFloatManager* mFloatManager;
> +  nsFloatManager* MOZ_NON_OWNING_REF mFloatManager;

I don't think the MOZ_NON_OWNING_REF annotation *quite* makes sense here -- the documentation for that annotation says it's only applicable for refcounted types:
https://dxr.mozilla.org/mozilla-central/rev/511093a0d82882d4b20f91b0287ad4b610c5225f/mfbt/Attributes.h#448-449

And nsFloatManager is not refcounted.  So even though this is a "non-owning ref", I think the annotation is misapplied doesn't do us any good in our static analysis.  (Or alternately, maybe the documentation for the annotation is out of date? If so, we should fix it!)

It's good that you're conveying information to human readers, though -- perhaps you could add a code-comment instead of the annotation, indicating that this is a non-owning pointer and explaining who actually owns this object? (some nsAutoFloatManager on the stack in a caller, I think)

::: layout/generic/nsFloatManager.h:569
(Diff revision 1)
>    CreateFloatManager(nsPresContext *aPresContext);
>  
>  protected:
>    ReflowInput &mReflowInput;
> -  nsFloatManager *mNew;
> -  nsFloatManager *mOld;
> +  mozilla::UniquePtr<nsFloatManager> mNew;
> +  nsFloatManager* MOZ_NON_OWNING_REF mOld;

As noted above, this annotation doesn't make sense here (but an explanatory comment to hint at non-owning-ness would be helpful!)

::: layout/generic/nsFloatManager.cpp:991
(Diff revision 1)
>  nsAutoFloatManager::CreateFloatManager(nsPresContext *aPresContext)
>  {
>    // Create a new float manager and install it in the reflow
>    // input. `Remember' the old float manager so we can restore it
>    // later.
> -  mNew = new nsFloatManager(aPresContext->PresShell(),
> +  mNew = MakeUnique<nsFloatManager>(aPresContext->PresShell(),

While you're here, you should probably add at the beginning of this method (before you assign mNew):

  MOZ_ASSERT(!mNew, "Redundant call to CreateFloatManager");


(It looks like the current code implicitly assumes that mNew is null when this function is called -- otherwise, we'd have been leaking its old contents whenever a redundant call happened!)

::: layout/generic/nsLineLayout.h:384
(Diff revision 1)
>  
>    void SetSuppressLineWrap(bool aEnabled) { mSuppressLineWrap = aEnabled; }
>  
>  protected:
>    // This state is constant for a given block frame doing line layout
> -  nsFloatManager* mFloatManager;
> +  nsFloatManager* MOZ_NON_OWNING_REF mFloatManager;

As above, MOZ_NON_OWNING_REF doesn't seem to make sense here; but a comment to the same effect would be helpful.
Attachment #8835878 - Flags: review?(dholbert) → review+
Side note: all of the "mOld" stuff here seems like it's really wanting to use mfbt's "AutoRestore", with an AutoRestore<nsFloatManager*> that unconditionally gets set to ReflowInput::mFloatManager on construction (and unconditionally restores it on destruction, whether a new value was set in the meantime or not).

If that makes sense to you, that might be a good further-improvement here. I think it's identical to what we've currently got, but AutoRestore makes it a bit easier still to reason about lifetimes, due to its RAII nature.
(The AutoRestore conversion might be more destabilizing / more trouble than it's worth, though; not sure.)
Comment on attachment 8835878 [details]
Bug 1338443 Part 2 - Convert nsAutoFloatManager::mNew to use UniquePtr.

https://reviewboard.mozilla.org/r/111452/#review113046

> I don't think the MOZ_NON_OWNING_REF annotation *quite* makes sense here -- the documentation for that annotation says it's only applicable for refcounted types:
> https://dxr.mozilla.org/mozilla-central/rev/511093a0d82882d4b20f91b0287ad4b610c5225f/mfbt/Attributes.h#448-449
> 
> And nsFloatManager is not refcounted.  So even though this is a "non-owning ref", I think the annotation is misapplied doesn't do us any good in our static analysis.  (Or alternately, maybe the documentation for the annotation is out of date? If so, we should fix it!)
> 
> It's good that you're conveying information to human readers, though -- perhaps you could add a code-comment instead of the annotation, indicating that this is a non-owning pointer and explaining who actually owns this object? (some nsAutoFloatManager on the stack in a caller, I think)

I must misunderstand that the MOZ_NON_OWNING_REF annotation applys to all "non-owning" types. I've added comments to explain this.

> While you're here, you should probably add at the beginning of this method (before you assign mNew):
> 
>   MOZ_ASSERT(!mNew, "Redundant call to CreateFloatManager");
> 
> 
> (It looks like the current code implicitly assumes that mNew is null when this function is called -- otherwise, we'd have been leaking its old contents whenever a redundant call happened!)

Nice suggestion. Added the assertion.
Re comment 6 and comment 7:

AutoRestore might not be fit the usage case here because 
1) We need to save old float manager to mOld in CreateFloatManager, but AutoRestore doesn't have a default constructor. The old float manager must be saved in nsAutoFloatManager's constructor, which changes the semantics. (Perhaps it's OK though.)
2) AutoRestore restores the old value in its destructor, which happens after nsAutoFloatManager's destructor. If we use AutoRestore for mOld, the debug log wrapping in gNoisyFloatManager might not be what actually happens.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99965273162e
Part 1 - Use ReflowInput's float manager in BlockReflowInput. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/5c10421076ff
Part 2 - Convert nsAutoFloatManager::mNew to use UniquePtr. r=dholbert
Comment on attachment 8835878 [details]
Bug 1338443 Part 2 - Convert nsAutoFloatManager::mNew to use UniquePtr.

https://reviewboard.mozilla.org/r/111452/#review113210

Minor nit:

::: layout/generic/nsFloatManager.h:570
(Diff revisions 1 - 2)
> + // A non-owning pointer, which points to the object owned by
> + // nsAutoFloatManager::mNew.
> +  nsFloatManager* mOld;

Indentation is off in the comment here -- needs 1 more space.
oops, my nit came too late. :) Not a huge deal. We can push a followup later.
Thanks for the catch!  Needinfo myself for the followup fix.
Flags: needinfo?(tlin)
https://hg.mozilla.org/mozilla-central/rev/99965273162e
https://hg.mozilla.org/mozilla-central/rev/5c10421076ff
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe816af3015
followup - Fix comment indentation for nsFloatManager::mOld. r=me
Flags: needinfo?(tlin)
You need to log in before you can comment on or make changes to this bug.