975823 - Clean up nsStreamConverterService

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Andrew McCreight [:mccr8]]

The code in this file is pretty old, and there are some things that can be cleaned up.

1. Some trailing whitespace.
2. Tons of null checks for the return value of infallible new.
3. The two different hashtables in this file use the same type for their data, for no reason.

The combination of 2 and 3 allows a lot of simplifications.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove some trailing whitespace from nsStreamConverterService.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove some trailing whitespace from nsStreamConverterService.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Take advantage of infallible new in nsStreamConverterService.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Split SCTableData into two data types, one for each table.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 4 - SCTableData doesn't use its key field.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 5 - SCTableData only uses the edges case of its union.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Remove unneeded indirection in SCTableData.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Replace SCTableData with a typedef.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 8 - Inline nsStreamConverterService::Init which is now infallible.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 9 - No reason to store a pointer to the list, just store it inline.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 10 - Substitute two SCTableData local vars.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 11 - BFSTableData only uses the state case of its union.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 12 - Inline BFSState into BFSTableData.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 13 - Make BFSTableData::predecessor into an AutoPtr.

Comment 15

[Andrew McCreight [:mccr8]]

Patrick, do you have a suggestion for who could review these changes to nsStreamConverterService, or should I just pick a random networking peer?  There are a lot of patches here, but each one makes a small mechanical change, and shouldn't really require a deep understanding of whatever this class is doing.

Comment 16

[Patrick McManus [:mcmanus]]

I'll take them

Comment 17

[Andrew McCreight [:mccr8]]

Thanks!  In part 7, SCTableData ends up as a typedef, which I don't inline.  Instead, I'm going to do that in bug 972593, because the conversion to a modern hash table eliminates a bunch of casts that mention SCTableData, so it makes the replacement a little nicer.

Comment 18

[Patrick McManus [:mcmanus]]

Andrew - I appreciate the legacy cleanup. We've got a lot of that hanging around. You deserve an award for turning the patch series into a choose-your-own-adventure (best-path) novel..

Comment 19

[Patrick McManus [:mcmanus]]

Comment on attachment 8391892 [details] [diff] [review]
part 8 - Inline nsStreamConverterService::Init which is now infallible.

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

::: netwerk/streamconv/src/nsStreamConverterService.cpp
@@ +52,5 @@
>  // Adjacency list data class.
>  typedef nsCOMArray<nsIAtom> SCTableData;
>  
> +// Delete all the entries in the adjacency list
> +static bool DeleteAdjacencyEntry(nsHashKey *aKey, void *aData, void* closure) {

nit *closure not * closure.. but you're just moving the code so don't fix if its a pain.

Comment 20

[Andrew McCreight [:mccr8]]

(In reply to Patrick McManus [:mcmanus] from comment #18)
> Andrew - I appreciate the legacy cleanup. We've got a lot of that hanging
> around. You deserve an award for turning the patch series into a
> choose-your-own-adventure (best-path) novel..

Thanks!  And thanks for all of the reviews.  I just wish I knew why it did this in the first place. ;)

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Patrick McManus [:mcmanus] from comment #19)
> nit *closure not * closure.. but you're just moving the code so don't fix if
> its a pain.

That method actually gets deleted in the hash table conversion, so I'll just leave the * alone.

Comment 22

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cd2da33bf4a2

Comment 23

[Andrew McCreight [:mccr8]]

This is leaking a small number of nsCStringKey and nsHashKey.  That's what I get for failing to push this to try without the hash table conversion on top of it!  I'm sure this will be easy to fix, so I'll look into it tomorrow.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8c20a5739d0c for leaks in mochitest-5 and crashtests

Comment 25

[Andrew McCreight [:mccr8]]

Attachment: part 4b - Don't pointlessly create new hash table keys.

This is kind of awesome.  Most hash table keys are allocated on the stack, then they get added to the hashtable, which clones the key.  These two are allocated on the heap.  Before my part 4 patch here, SCTableData would store a pointer to that heap allocated key, do nothing with it, until the entry went away, when it would delete the key, avoiding a leak.  I removed the pointer to the key in part 4, because we never used it, so these heap allocated keys started leaking.  The solution is just to not be weird and allocate them on the stack like everything else.  I'll land this folded into part 4.

try run is green: https://tbpl.mozilla.org/?tree=Try&rev=facff5057fe4

Comment 26

[Andrew McCreight [:mccr8]]

Somebody went to a lot of effort to keep around pointers to these keys in SCTableData, so they must have been used at some point...

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 8394426 [details] [diff] [review]
part 4b - Don't pointlessly create new hash table keys.

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

awesome. it's like being a forensic programmer some days.

Comment 28

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7f9fd179263e

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a96fc992024c
https://hg.mozilla.org/mozilla-central/rev/e98833ba22bf
https://hg.mozilla.org/mozilla-central/rev/951f95252494
https://hg.mozilla.org/mozilla-central/rev/c0c8ce89c57a
https://hg.mozilla.org/mozilla-central/rev/f48161f34546
https://hg.mozilla.org/mozilla-central/rev/2038aa3affdc
https://hg.mozilla.org/mozilla-central/rev/496288ab6f10
https://hg.mozilla.org/mozilla-central/rev/fa2ff68f86e4
https://hg.mozilla.org/mozilla-central/rev/31b7a5b5c31b
https://hg.mozilla.org/mozilla-central/rev/20620ef855be
https://hg.mozilla.org/mozilla-central/rev/db2568cc6e61
https://hg.mozilla.org/mozilla-central/rev/d2c6f3f8e36c
https://hg.mozilla.org/mozilla-central/rev/7f9fd179263e