Closed Bug 972593 Opened 6 years ago Closed 6 years ago

Convert nsStreamConverterService::mAdjacencyList to a more modern hashtable

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(2 files, 3 obsolete files)

This looks slightly tricky, but maybe only because I don't really understand how the strings should be created or copied around.
https://tbpl.mozilla.org/?tree=Try&rev=ecd2e68bc23c
Assignee: nobody → continuation
Depends on: 975823
Attached patch part 2 - Inline the typedef. (obsolete) — Splinter Review
I need to look over this a bit more to convince myself all the string stuff I do is right.
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..
Attachment #8409434 - Flags: review?(mcmanus) → review+
No problem, there's not a huge hurry on these removals.  Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6c395d9364
https://hg.mozilla.org/mozilla-central/rev/ac6c395d9364
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
This fix causes a crash on Thunderbird mozmill test.
Blocks: 1004658
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.
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.
Attachment #8416310 - Flags: review?(mcmanus)
No longer blocks: 1004658
Depends on: 1004658
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.
Attachment #8416382 - Flags: review?(mcmanus)
Blocks: 1004658
No longer depends on: 1004658
Attachment #8416382 - Attachment is patch: true
please decide which patch you want and set 1 review flag.
Comment on attachment 8416310 [details] [diff] [review]
part 2 - Don't deref an always-null pointer in nsStreamConverterService::FindConverter.

The other one is better.
Attachment #8416310 - Attachment is obsolete: true
Attachment #8416310 - Flags: review?(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
Attachment #8416382 - Flags: review?(mcmanus) → review+
You need to log in before you can comment on or make changes to this bug.