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

I would like to work on this :)

(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.

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

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

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.

(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.

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?

(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 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?

(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!

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.

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

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.

(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 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!

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

(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!

(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.

(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!