1249389 - remove nsAutoArrayPtr usages from the startup cache and its clients

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nathan Froyd [:froydnj]]

This is the most complicated nsAutoArrayPtr removal bit, and I've split the
patches up into what should be small, self-contained parts.  We take a bit of a
shortcut at the end, though.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 0 - use getter_Transfers in TestStartupCache.cpp

TestWriteObject() in TestStartupCache.cpp uses this odd pattern of
acquiring a raw pointer from the startup cache, and then stashing that
raw pointer into an nsAutoArrayPtr.  We can do better by using the
getter_Transfers idiom and thereby always using the smart pointer.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - change NewObjectInputStreamFromBuffer to take a UniquePtr argument

Because NewObjectInputStreamFromBuffer takes a raw pointer as input, the
typical coding pattern to use it is:

  nsAutoArrayPtr<char> buf;
  // assign something to buf
  nsresult rv = NewObjectInputStreamFromBuffer(buf, ...);
  if (NS_FAILED(rv)) {
    ...
    return rv;
  }
  buf.forget();

Which is clumsy, error-prone, and obscures the ownership transfer of the
pointer into the stream returned by NewObjectInputStreamFromBuffer.
Let's address all of these concerns by changing the argument to a
UniquePtr<char[]>.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2 - change NewBufferFromStorageStream's outparam into a UniquePtr

Similar to the previous change to NewObjectInputStreamFromBuffer, we
want to make the ownership transfer out of NewBufferFromStorageStream
more obvious.  Doing this also lets us get rid of some uses of
nsAutoArrayPtr, which is less idiomatic than UniquePtr.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 3 - assert the size of nsZipItemPtr's storage type

The only uses of this class use a template argument with a size of
|char| (uint8_t and char), and the class isn't designed to accomodate
template arguments of larger size (e.g. the implementation of Forget()
neglects to divide by sizeof(T) for allocating a return buffer).  Let's
enforce this with a static_assert.  This change makes the class safer to
use and also makes future changes simpler to reason about.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 4 - make StartupCache::GetBuffer take a UniquePtr outparam

This change eliminates a number of nsAutoArrayPtr usages, as well as
making the pattern GetBuffer() -> NewObjectInputStreamFromBuffer more
pleasant.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 5 - use UniquePtr instead of nsAutoArrayPtr in mozilla::scache::CacheEntry

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 6 - provide UniquePtr overload for nsIStartupCache::GetBuffer

The lone remaining startup cache-related uses of nsAutoArrayPtr are both
in TestStartupCache.cpp, for use with nsIStartupCache::GetBuffer.  The
uses can't use StartupCache::GetBuffer because StartupCache::GetBuffer
isn't visible outside of libxul, and TestStartupCache is a normal C++
unit test.

The Right Thing is to convert TestStartupCache to a gtest so we can see
libxul internal symbols and then delete nsIStartupCache entirely.
That's a bit complicated, as TestStartupCache doesn't fit nicely into
gtest's framework.  The simpler solution is to add a UniquePtr overload
in the interface that hides the XPCOM outparam management details.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 7 - clean up calls to GetBuffer in TestStartupCache

With the previous patch, we're able to eliminate some raw pointer usages
in TestStartupCache, which is always nice.

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8720930 [details] [diff] [review]
part 2 - change NewBufferFromStorageStream's outparam into a UniquePtr

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

::: startupcache/StartupCacheUtils.h
@@ +34,3 @@
>  NS_EXPORT nsresult
>  NewBufferFromStorageStream(nsIStorageStream *storageStream, 
> +                           UniquePtr<char[]>* buffer, uint32_t* len);

Is there a reason for having this take a pointer and not a ref?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #9)
> ::: startupcache/StartupCacheUtils.h
> @@ +34,3 @@
> >  NS_EXPORT nsresult
> >  NewBufferFromStorageStream(nsIStorageStream *storageStream, 
> > +                           UniquePtr<char[]>* buffer, uint32_t* len);
> 
> Is there a reason for having this take a pointer and not a ref?

By and large, I think we prefer to have things take pointer arguments to make it obvious at the callsite that a modification is happening.  Admittedly, this is not great, because it raises questions at the callee about whether the passed in pointer is null or not.  I know Seth has advocated for a NonNull<T> type for this reason, among others...

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 7 - clean up calls to GetBuffer in TestStartupCache

Missed one nsAutoArrayPtr usage.

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8720934 [details] [diff] [review]
part 6 - provide UniquePtr overload for nsIStartupCache::GetBuffer

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

I agree this is the pragmatic route. Can you file a follow-up for removing nsIStartupCache?

Comment 13

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> (In reply to Eric Rahm [:erahm] from comment #9)
> > ::: startupcache/StartupCacheUtils.h
> > @@ +34,3 @@
> > >  NS_EXPORT nsresult
> > >  NewBufferFromStorageStream(nsIStorageStream *storageStream, 
> > > +                           UniquePtr<char[]>* buffer, uint32_t* len);
> > 
> > Is there a reason for having this take a pointer and not a ref?
> 
> By and large, I think we prefer to have things take pointer arguments to
> make it obvious at the callsite that a modification is happening. 
> Admittedly, this is not great, because it raises questions at the callee
> about whether the passed in pointer is null or not.  I know Seth has
> advocated for a NonNull<T> type for this reason, among others...

Seems like that's what const is for, but I can see arguments on both sides.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 4 - make StartupCache::GetBuffer take a UniquePtr outparam

Update of part 4 for Android compilation failures, which has the nice
side-effect of eliminating a manual free() call, as well as fixing a leak.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/615be94be086
https://hg.mozilla.org/integration/mozilla-inbound/rev/b688e6309058
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e29cda9552
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f79d41d9098
https://hg.mozilla.org/integration/mozilla-inbound/rev/50332bf18a2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcfb36dfee0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3bc10a6437
https://hg.mozilla.org/integration/mozilla-inbound/rev/4121f9b10c82

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1a5726b179

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/615be94be086
https://hg.mozilla.org/mozilla-central/rev/b688e6309058
https://hg.mozilla.org/mozilla-central/rev/14e29cda9552
https://hg.mozilla.org/mozilla-central/rev/1f79d41d9098
https://hg.mozilla.org/mozilla-central/rev/50332bf18a2f
https://hg.mozilla.org/mozilla-central/rev/bcfb36dfee0f
https://hg.mozilla.org/mozilla-central/rev/2a3bc10a6437
https://hg.mozilla.org/mozilla-central/rev/4121f9b10c82
https://hg.mozilla.org/mozilla-central/rev/5a1a5726b179