Closed Bug 1342994 Opened 3 years ago Closed 3 years ago

Convert nsPrintObject "mKids" array to use UniquePtr

Categories

(Core :: Printing: Output, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

Details

Attachments

(2 files, 2 obsolete files)

nsPrintObject has an array "mKids" which stores raw pointers to uniquely-owned resources:
https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/layout/printing/nsPrintObject.h#53

...which it destroys with explicit "delete" calls in its destructor:
https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/layout/printing/nsPrintObject.cpp#34-37

We should just convert this array to use UniquePtr (and convert all of the references to this array to use e.g. "const UniquePtr&" instead of raw pointers, to be sure we're enforcing the invariants around ownership etc.)

[CC'ing TYLin since he's done some other UniquePtr upgrades recently, & might be interested in taking this.]
layout/printing/nsPrintEngine.h has many APIs have |nsPrintObject*| parameter. I was trying to convert them all as |const UniquePtr<nsPrintObject>&|. However, I found that we pass non-owning pointers to some APIs like SetPrintPO [1], so they cannot be converted. It's also inconsistent to have some API receiving |const UniquePtr<nsPrintObject>&| while some receiving |nsPrintObject*|, so I leave them as they were, and using |get()| to transform UniquePtr to non-owning pointers.

At least, we have replace all the explicit new/delete by UniquePtr under layout/printing/.

[1] http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/layout/printing/nsPrintEngine.cpp#3237-3242
Comment on attachment 8843944 [details]
Bug 1342994 Part 1 - Strip trailing whitespaces in nsPrintData, nsPrintEngine and nsPrintObject.

https://reviewboard.mozilla.org/r/117546/#review119256
Attachment #8843944 - Flags: review?(dholbert) → review+
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #4)
> layout/printing/nsPrintEngine.h has many APIs have |nsPrintObject*|
> parameter. I was trying to convert them all as |const
> UniquePtr<nsPrintObject>&|

Admirable!  That would be great if we could do that.

> However, I found that we pass non-owning
> pointers to some APIs like SetPrintPO [1], so they cannot be converted.
> [1]
> http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/layout/printing/nsPrintEngine.cpp#3237-3242

I see. This partly comes from the fact that "mPrintDocList" is a flat array of non-owning pointers, and is a bit of a hack, it seems. :-/

> It's
> also inconsistent to have some API receiving |const
> UniquePtr<nsPrintObject>&| while some receiving |nsPrintObject*|, so I leave
> them as they were, and using |get()| to transform UniquePtr to non-owning
> pointers.

I'm not as concerned with that inconsistency... If there are APIs that we could convert (i.e. which don't take mPrintDocList), I'd lean towards converting them. But, not a big deal.

> At least, we have replace all the explicit new/delete by UniquePtr under
> layout/printing/.

True. Thanks!
Comment on attachment 8843945 [details]
Bug 1342994 Part 2 - Use UniquePtr to replace explicit new/delete and non-owing pointers in printing APIs.

https://reviewboard.mozilla.org/r/117548/#review119258

r=me

::: commit-message-2a0e1:3
(Diff revision 1)
> +Bug 1342994 Part 2 - Convert nsPrintObject::mKids to an array of UniquePtr<nsPrintObject>.
> +
> +Also, rewrite some for-loops as ranged-based.

s/ranged/range/

::: layout/printing/nsPrintEngine.cpp:1183
(Diff revision 1)
>        childAsShell->GetContentViewer(getter_AddRefs(viewer));
>        if (viewer) {
>          nsCOMPtr<nsIContentViewerFile> viewerFile(do_QueryInterface(viewer));
>          if (viewerFile) {
>            nsCOMPtr<nsIDOMDocument> doc = do_GetInterface(childAsShell);
> -          nsPrintObject * po = new nsPrintObject();
> +          UniquePtr<nsPrintObject> po = MakeUnique<nsPrintObject>();

Optional nit: Probably better to just use "auto po" here, so that you don't repeat the type (nsPrintObject) twice on one line.

(This is one of our explicitly-recommended/blessed usages of the "auto" type, as noted here:
https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#649  )
Attachment #8843945 - Flags: review?(dholbert) → review+
Comment on attachment 8843946 [details]
Bug 1342994 Part 3 - Convert nsPrintData::mPrintObject to UniquePtr.

https://reviewboard.mozilla.org/r/117550/#review119266

r=me, though ideally I'd love to see an additional patch here to upgrade as many raw-pointers in the APIs here as possible so we can get rid of all these get()'s and get closer to being able to usefully reason about the UniquePtr guarantees.

Now that you've upgraded both mPrintObject and its mKids array, I suspect that allows most (or many) APIs here to be upgraded... basically any that doesn't have any callers that use mPrintDocList entries, perhaps.

::: layout/printing/nsPrintData.h:69
(Diff revision 1)
>    nsCOMArray<nsIWebProgressListener> mPrintProgressListeners;
>    nsCOMPtr<nsIPrintProgressParams> mPrintProgressParams;
>  
>    nsCOMPtr<nsPIDOMWindowOuter> mCurrentFocusWin; // cache a pointer to the currently focused window
>  
> -  nsTArray<nsPrintObject*>    mPrintDocList;
> +  nsTArray<nsPrintObject*>    mPrintDocList;   // This is an array of non-owning pointers.

This struct (an array of raw pointers to "uniquely" owned resources) is pretty scary.  Please add a clearer explanation about the lifetime of these objects (hinting at how we know we're safe to use them, given that they're non-owning).

Perhaps something like the following, THOUGH please sanity-check that this is indeed accurate:

"Array of non-owning pointers to all the nsPrintObjects owned by this nsPrintData. This includes this->mPrintObject, as well as all of its mKids (and their mKids, etc.)"

::: layout/printing/nsPrintEngine.cpp:516
(Diff revision 1)
> -    mPrt->mPrintObject = new nsPrintObject();
> +    mPrt->mPrintObject = MakeUnique<nsPrintObject>();
>      NS_ENSURE_TRUE(mPrt->mPrintObject, NS_ERROR_OUT_OF_MEMORY);

While you're here, please get rid of this NS_ENSURE_TRUE. mPrintObject can never be null here (which has been the case for quite a while; this is just an obsolete allocation check)
Attachment #8843946 - Flags: review?(dholbert) → review+
Comment on attachment 8843946 [details]
Bug 1342994 Part 3 - Convert nsPrintData::mPrintObject to UniquePtr.

https://reviewboard.mozilla.org/r/117550/#review119266

> This struct (an array of raw pointers to "uniquely" owned resources) is pretty scary.  Please add a clearer explanation about the lifetime of these objects (hinting at how we know we're safe to use them, given that they're non-owning).
> 
> Perhaps something like the following, THOUGH please sanity-check that this is indeed accurate:
> 
> "Array of non-owning pointers to all the nsPrintObjects owned by this nsPrintData. This includes this->mPrintObject, as well as all of its mKids (and their mKids, etc.)"

I think your description of mPrintDocList's life cycle is correct, so I'll just use your wording.  mPrintDocList only got element appended at [1] and [2]. [1] is owned by nsPrintData::mPrintObject, and [2] is owned by nsPrintObject::mKids.

[1] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/layout/printing/nsPrintEngine.cpp#521
[2] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/layout/printing/nsPrintEngine.cpp#1189
Addressed the review comments of patchset 1, and added Part 4 to convert most of the raw-pointers to const UniquePtr& in the nsPrintEngine APIs.  Part 4 removed many get() added in Part 2 and Part 3, so it might be a good idea to squash Part 2 ~ 4 when landing to save some blame history.
Comment on attachment 8844451 [details]
Bug 1342994 Part 4 - Convert most of the nsPrintEngine's methods to use UniquePtr.

https://reviewboard.mozilla.org/r/117902/#review119618

r=me, thanks!
Attachment #8844451 - Flags: review?(dholbert) → review+
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #15)
> so it might be a good
> idea to squash Part 2 ~ 4 when landing to save some blame history.

I was thinking that as well, yeah -- probably a good idea. Would you mind doing that before landing?

(That'll require a bit of commit-message-smithing, to collapse the 3 commit messages into 1 meaningful commit message that encompasses the newly-squished parts.)
Attachment #8843946 - Attachment is obsolete: true
Attachment #8844451 - Attachment is obsolete: true
Comment on attachment 8843945 [details]
Bug 1342994 Part 2 - Use UniquePtr to replace explicit new/delete and non-owing pointers in printing APIs.

https://reviewboard.mozilla.org/r/117548/#review119868

This new Part 2 (patchset 3) is the squashed version of the Part 2 ~ 4 from patchset 2 with a new title.
Looks good. thanks!
Assignee: nobody → tlin
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3332a33334d0
Part 1 - Strip trailing whitespaces in nsPrintData, nsPrintEngine and nsPrintObject. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/902c21221272
Part 2 - Use UniquePtr to replace explicit new/delete and non-owing pointers in printing APIs. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/3332a33334d0
https://hg.mozilla.org/mozilla-central/rev/902c21221272
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.