972593 - Convert nsStreamConverterService::mAdjacencyList to a more modern hashtable

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

This looks slightly tricky, but maybe only because I don't really understand how the strings should be created or copied around.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=ecd2e68bc23c

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 1 - Convert nsStreamConverterService hashtables to something modern.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Inline the typedef.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I need to look over this a bit more to convince myself all the string stuff I do is right.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: Convert nsStreamConverterService hashtables to something modern.

https://tbpl.mozilla.org/?tree=Try&rev=4d406b9ca8ec

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8409434 [details] [diff] [review]
Convert nsStreamConverterService hashtables to something modern.

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

sorry about the lag here..

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

No problem, there's not a huge hurry on these removals.  Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6c395d9364

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ac6c395d9364

Comment 9

[Hiroyuki Ikezoe (:hiro)]

This fix causes a crash on Thunderbird mozmill test.

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8409434 [details] [diff] [review]
Convert nsStreamConverterService hashtables to something modern.

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

::: netwerk/streamconv/src/nsStreamConverterService.cpp
@@ +271,5 @@
>              nsIAtom* curVertexAtom = data2->ObjectAt(i);
>              nsAutoString curVertexStr;
>              curVertexAtom->ToString(curVertexStr);
> +            nsCString *curVertex = nullptr;
> +            CopyUTF16toUTF8(curVertexStr, *curVertex);

Somehow, I suspect this is very, very, very, very wrong.

Comment 11

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Don't deref an always-null pointer in nsStreamConverterService::FindConverter.

Thanks for pointing out the error, Joshua!

I suppose it isn't worth doing a try run, given that the current code will crash every time it is run.

Comment 12

[Mark Banner (:standard8)]

Attachment: Alternative part 2

Here's an alternative part 2 if you want it. It makes use of the ToUTF8String function of nsIAtom to reduce the LoC a bit.

I've tested locally that it fixes bug 1004658.

Comment 13

[Patrick McManus [:mcmanus]]

please decide which patch you want and set 1 review flag.

Comment 14

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 8416310 [details] [diff] [review]
part 2 - Don't deref an always-null pointer in nsStreamConverterService::FindConverter.

The other one is better.

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8416382 [details] [diff] [review]
Alternative part 2

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

sorry i didn't notice this the first time around. thanks for the fix

Comment 16

[Mark Banner (:standard8)]

Thanks for the review, landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/865805d83d5a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/865805d83d5a