Closed Bug 1223808 Opened 4 years ago Closed 4 years ago

replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
This function:

- Allocates a block of memory;
- Copies things into that block of memory;
- Allocates another block of memory inside nsAutoCString;
- Copies into our string from the block of memory we just allocated.

Clearly we can avoid a memory allocation and some copying by just
allocating the amount of data we need in the string first, and copying
things into the string.
Attachment #8686061 - Flags: review?(mcmanus)
Converting HandleHashKey's use of nsAutoArrayPtr is a little difficult,
because:

  MakeUnique<SHA1Sum::Hash>();

doesn't work, and trying to do something like:

  mHash.reset(reinterpret_cast<SHA1Sum::Hash*>(new uint8_t[SHA1Sum::kHashSize]));

is just ugly.  Since we're always allocating it anyway, let's just store
it directly in HandleHashKey and avoid the dynamic allocation in the
first place.
Attachment #8686062 - Flags: review?(michal.novotny)
Attachment #8686061 - Flags: review?(mcmanus) → review+
Comment on attachment 8686062 [details] [diff] [review]
part 2 - don't dynamically allocate HandleHashKey::mHash

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

The reason why the hash is dynamically allocated is that in CacheFileHandle we need a pointer to the hash that won't change. If the hash is stored in HandleHashKey, then CacheFileHandle::mHash becomes invalid sooner or later, right?
Attachment #8686062 - Flags: review?(michal.novotny) → review-
(In reply to Michal Novotny (:michal) from comment #4)
> The reason why the hash is dynamically allocated is that in CacheFileHandle
> we need a pointer to the hash that won't change. If the hash is stored in
> HandleHashKey, then CacheFileHandle::mHash becomes invalid sooner or later,
> right?

Ah, yes.  This explains the crashes I was getting shortly after startup, sometime after I posted this patch. :)  I guess the alternative is some liberal use of reinterpret_cast.
Comment on attachment 8686063 [details] [diff] [review]
part 3 - replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/

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

thanks.. that will get picked up as an idiom
Attachment #8686063 - Flags: review?(mcmanus) → review+
Here's a patch that seems to work better.
Attachment #8689187 - Flags: review?(michal.novotny)
Attachment #8686062 - Attachment is obsolete: true
Attachment #8689187 - Flags: review?(michal.novotny) → review+
Blocks: 1229985
You need to log in before you can comment on or make changes to this bug.