1357155 - Convert PRCList usage in nsStandardURL to LinkedList

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Eric Rahm [:erahm]]

nsStandardURL uses a PRCList when |DEBUG_DUMP_URLS_AT_SHUTDOWN| is set to maintain a list of leaked URLs at shutdown. We should convert that to a LinkedList [1].

Steps:
- Remove |mDebugCList| [2], and make nStandardURL inherit from LinkedListElement if DEBUG_DUMP_URLS_AT_SHUTDOWN is set
  - Update the constructor to insert using LinkedList methods
  - Remove list removal code from the destructor, it will remove itself
- Convert |gAllURLs| [3] to a LinkedList, update all calls to use LinkedList methods
- Make sure |gAllURLs| is cleared at shutdown [4]

[1] http://searchfox.org/mozilla-central/source/mfbt/LinkedList.h
[2] http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/netwerk/base/nsStandardURL.h#307
[3] http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/netwerk/base/nsStandardURL.cpp#286
[4] http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/netwerk/base/nsStandardURL.cpp#349-359

Comment 1

[HellosAazS]

I would like to work on this :)

Comment 2

[Eric Rahm [:erahm]]

(In reply to HellosAazS from comment #1)
> I would like to work on this :)

Great, I'll go ahead and assign it to you. Please let me know if you need any help.

Comment 3

[HellosAazS]

Attachment: patch.diff

Hello, Eric!
I've finish the code change,am I on the right track?

Comment 4

[HellosAazS]

Oops sorry,I don't know why couldn't my file view on bugzilla.

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8859430 [details] [diff] [review]
patch.diff

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

This looks good! Just a few very minor comments.

::: nsStandardURL.cpp
@@ +343,2 @@
>          printf("Leaked URLs:\n");
> +		for (nsStandardURL* url = gAllURLs.getFirst(); url != nullptr;url->getNext()) {

This can be simplified a bit further by using a range-based for loop, ie:

> for (auto url : gAllURLs)

::: nsStandardURL.h
@@ +48,5 @@
>                      , public nsISizeOf
>                      , public nsIIPCSerializableURI
>                      , public nsISensitiveInfoHiddenURI
> +#ifdef DEBUG_DUMP_URLS_AT_SHUTDOWN
> +					, public LinkedListElement<nsStandardURL>

nit: please use spaces, not tabs.

Comment 6

[Eric Rahm [:erahm]]

(In reply to HellosAazS from comment #4)
> Oops sorry,I don't know why couldn't my file view on bugzilla.

When you upload a patch you need to check the "patch" field so that bugzilla knows it's a patch.

Comment 7

[HellosAazS]

Attachment: patch.diff

Thanks for the feedback!I've change the code.
Also I have a little question,should I use the review flag every time I upload
the patch?

Comment 8

[Eric Rahm [:erahm]]

(In reply to HellosAazS from comment #7)
> Created attachment 8859439 [details] [diff] [review]
> patch.diff
> 
> Thanks for the feedback!I've change the code.
> Also I have a little question,should I use the review flag every time I
> upload
> the patch?

Good question, yes setting r? when uploading new patch is the way to go. This lets me know I have something new to review.

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8859439 [details] [diff] [review]
patch.diff

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

This looks good, just one more small indentation cleanup.

For the next review can you provide a patch generated by mercurial (or git)? You can find more details on MDN (ignore the mozreview part) [1]. This will include your info as the patch author as well as a commit message.

For this bug a commit message similar to the following should suffice:

> Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm

So this includes the bug number, a brief description of what is being done, and marks me as the reviewer.

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: nsStandardURL.cpp
@@ +343,5 @@
>          printf("Leaked URLs:\n");
> +		for (auto url : gAllURLs) {
> +			url->PrintSpec();
> +		}
> +		gAllURLs.clear();

Just one more small request, can you fix the indentation to match the rest and replace the tabs with spaces here?

Comment 10

[HellosAazS]

(In reply to Eric Rahm [:erahm] from comment #9)
> For the next review can you provide a patch generated by mercurial (or git)?
> You can find more details on MDN (ignore the mozreview part) [1]. This will
> include your info as the patch author as well as a commit message.
> 
> For this bug a commit message similar to the following should suffice:
> 
> > Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm
> 
> So this includes the bug number, a brief description of what is being done,
> and marks me as the reviewer.
Of course!I'm doing that now, but it seems I couldn't correctly generate the patch
now,since I've never use these tools.I will upload the patch as soon as I learn to 
use it!
> 
> Just one more small request, can you fix the indentation to match the rest
> and replace the tabs with spaces here?
Of course!I've used the text editor to check that no tab remain.Sorry for 
forgetting to do that!

Comment 11

[HellosAazS]

Attachment: Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm

I'm glad that I finally know how to use mercurial to generate 
this patch :)
But I'm not sure if it is the "patch" we need.I hope that I 
didn't do anything wrong.
I follow the steps in MDN,but failed for some reason(the file
contains many others patch).
At the end I use the method(hg qnew,qrefresh,then export) I found
on a blog about patching Firefox and got this file.

Comment 12

[HellosAazS]

Attachment: Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm

Sorry about this!That previous r? file has a no need space, I've correct it.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8860107 [details] [diff] [review]
Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm

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

This looks good to me, thanks for your patience! I'm going to ask for review from a network peer as well.

Comment 14

[Eric Rahm [:erahm]]

(In reply to HellosAazS from comment #11)
> At the end I use the method(hg qnew,qrefresh,then export) I found
> on a blog about patching Firefox and got this file.

That's how I do things as well, in the future you might be interested in the |hg bzexport| extension. It makes uploading the patch a bit easier.

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8860107 [details] [diff] [review]
Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm

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

This is great. Thanks!

Comment 16

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5acd803dfc969ad064fecf5dd22823457ca8cb43
Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm

Comment 17

[HellosAazS]

(In reply to Eric Rahm [:erahm] from comment #13)
> Comment on attachment 8860107 [details] [diff] [review]
> Bug 1357155 - Convert PRCList usage in nsStandardURL to LinkedList. r=erahm
> 
> Review of attachment 8860107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, thanks for your patience! I'm going to ask for review
> from a network peer as well.

Thanks Eric!Never thought I could really get involved in open source,your kind
and patient guide really help me a lot!Thank you very much!

Comment 18

[Eric Rahm [:erahm]]

(In reply to HellosAazS from comment #17)
> Thanks Eric!Never thought I could really get involved in open source,your
> kind
> and patient guide really help me a lot!Thank you very much!

Happy to help, thank you for you contribution! Please let me know if you want help finding other bugs to work on.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/5acd803dfc96

Comment 20

[HellosAazS]

(In reply to Eric Rahm [:erahm] from comment #18)
> Happy to help, thank you for you contribution! Please let me know if you
> want help finding other bugs to work on.
That would be great, I was wondering if ask for help like that is ok.
I'm ready for solving more bugs!