Convert PRCList usage in nsStandardURL to LinkedList

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: HellosAazS, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=c++][good first bug][necko-active])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

2 years ago
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
Assignee

Comment 1

2 years ago
I would like to work on this :)
Reporter

Comment 2

2 years ago
(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.
Assignee: nobody → HellosAazS
Assignee

Comment 3

2 years ago
Posted patch patch.diff (obsolete) — Splinter Review
Hello, Eric!
I've finish the code change,am I on the right track?
Flags: needinfo?(erahm)
Assignee

Comment 4

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

Updated

2 years ago
Attachment #8859430 - Attachment is patch: true
Attachment #8859430 - Attachment mime type: text/x-patch → text/plain
Reporter

Comment 5

2 years ago
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.
Attachment #8859430 - Flags: feedback+
Reporter

Comment 6

2 years ago
(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.
Flags: needinfo?(erahm)
Assignee

Comment 7

2 years ago
Posted patch patch.diff (obsolete) — Splinter Review
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?
Flags: needinfo?(erahm)
Whiteboard: [lang=c++][good first bug] → [lang=c++][good first bug][necko-active]
Reporter

Comment 8

2 years ago
(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.
Flags: needinfo?(erahm)
Reporter

Comment 9

2 years ago
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?
Attachment #8859439 - Flags: feedback+
Assignee

Comment 10

2 years ago
(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!
Assignee

Comment 11

2 years ago
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.
Attachment #8860096 - Flags: review?(erahm)
Assignee

Comment 12

2 years ago
Sorry about this!That previous r? file has a no need space, I've correct it.
Attachment #8860107 - Flags: review?(erahm)
Reporter

Comment 13

2 years ago
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.
Attachment #8860107 - Flags: review?(mcmanus)
Attachment #8860107 - Flags: review?(erahm)
Attachment #8860107 - Flags: review+
Reporter

Comment 14

2 years ago
(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.
Reporter

Updated

2 years ago
Attachment #8859430 - Attachment is obsolete: true
Reporter

Updated

2 years ago
Attachment #8859439 - Attachment is obsolete: true
Reporter

Updated

2 years ago
Attachment #8860096 - Attachment is obsolete: true
Attachment #8860096 - Flags: review?(erahm)
Attachment #8860107 - Flags: review?(mcmanus) → review?(valentin.gosu)
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!
Attachment #8860107 - Flags: review?(valentin.gosu) → review+
Reporter

Comment 16

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

Comment 17

2 years ago
(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!
Reporter

Comment 18

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5acd803dfc96
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Comment 20

2 years ago
(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!
You need to log in before you can comment on or make changes to this bug.