Closed Bug 1249389 Opened 4 years ago Closed 4 years ago

remove nsAutoArrayPtr usages from the startup cache and its clients

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(8 files, 2 obsolete files)

1.79 KB, patch
erahm
: review+
Details | Diff | Splinter Review
8.22 KB, patch
erahm
: review+
Details | Diff | Splinter Review
7.15 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.42 KB, patch
erahm
: review+
Details | Diff | Splinter Review
3.82 KB, patch
erahm
: review+
Details | Diff | Splinter Review
3.57 KB, patch
erahm
: review+
Details | Diff | Splinter Review
4.29 KB, patch
erahm
: review+
Details | Diff | Splinter Review
14.52 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8720928 - Flags: review?(erahm)
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[]>.
Attachment #8720929 - Flags: review?(erahm)
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.
Attachment #8720930 - Flags: review?(erahm)
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.
Attachment #8720931 - Flags: review?(erahm)
This change eliminates a number of nsAutoArrayPtr usages, as well as
making the pattern GetBuffer() -> NewObjectInputStreamFromBuffer more
pleasant.
Attachment #8720932 - Flags: review?(erahm)
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.
Attachment #8720934 - Flags: review?(erahm)
With the previous patch, we're able to eliminate some raw pointer usages
in TestStartupCache, which is always nice.
Attachment #8720935 - Flags: review?(erahm)
Blocks: 1229985
Attachment #8720928 - Flags: review?(erahm) → review+
Attachment #8720929 - Flags: review?(erahm) → review+
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?
Attachment #8720930 - Flags: review?(erahm) → review+
(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...
Missed one nsAutoArrayPtr usage.
Attachment #8722559 - Flags: review?(erahm)
Attachment #8720935 - Attachment is obsolete: true
Attachment #8720935 - Flags: review?(erahm)
Attachment #8720931 - Flags: review?(erahm) → review+
Attachment #8720932 - Flags: review?(erahm) → review+
Attachment #8720933 - Flags: review?(erahm) → review+
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?
Attachment #8720934 - Flags: review?(erahm) → review+
Attachment #8722559 - Flags: review?(erahm) → review+
(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.
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.
Attachment #8723139 - Flags: review+
Attachment #8720932 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.