|PathRunnablesParametersWrapper| potentially leaking in NativeFileWatcherWin.cpp when shutting down

RESOLVED FIXED in mozilla37

Status

()

Toolkit
OS.File
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Dexter, Assigned: Nihar Mehta, Mentored)

Tracking

Trunk
mozilla37
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 9 obsolete attachments)

The |aWrappedParameters| argument of |NativeFIleWatcherIOTask::AddPathRunnableMethod| (http://dxr.mozilla.org/mozilla-central/source/toolkit/components/filewatcher/NativeFileWatcherWin.cpp#574) may potentially leak if |mShuttingDown| is true and the function returns NS_OK (see http://dxr.mozilla.org/mozilla-central/source/toolkit/components/filewatcher/NativeFileWatcherWin.cpp#1355).

One possible solution to this problem might be wrapping aWrappedParameters in |nsAutoPtr| before the check to |mShuttingDown| is made.
Whiteboard: [lang=c++] [good first bug]
(Assignee)

Comment 1

4 years ago
Hi I am new to contribution to open source. I would like to contribute to this bug. Can you please guide me how do I go about it? What exactly do we want?
Hello Nihar and welcome!

Since this is your first contribution, have a look at the "Contributing to Mozilla Codebase" (https://developer.mozilla.org/en-US/docs/Introduction). Your first step would be to download and compile Firefox. Once you've done that, you can modify NativeFileWatcherWin.cpp as described in the previous comments.

Basically, you would need to move the two occurrences of |nsAutoPtr<PathRunnablesParametersWrapper> wrappedParameters(aWrappedParameters);| before the |if (mShuttingDown) {| check.

This search highlights the aforementioned |nsAutoPtr| locations: http://dxr.mozilla.org/mozilla-central/search?q=nsAutoPtr%3CPathRunnablesParametersWrapper%3E&redirect=true

Once done, please follow the "Step 4 - Get your code reviewed" in the "Contributing to Mozilla Codebase" to submit a patch.
(Assignee)

Comment 3

4 years ago
Thank you for guiding me. I have successfullly cloned the repository ( after waiting almost 3 hours ) and I will be finishing other steps by tonight and I will start the work on the bug from tomorrow.
(Assignee)

Comment 4

4 years ago
Unfortunately, I am unable to build mozilla. This is because I am running on dual boot Windows+Ubuntu.
The Ubuntu partition is 5.73 GB only and it is almost full with files which cannot be deleted.
Total disk space is 700 GB.
The mozilla central folder is around 2.2 GB
When I try to build it keeping it in C/D drive of windows. I am getting error Permission denied (even after logging in as superuser)
Can somebody help me?
One possible solution is enlarging the Linux partition, which is a risk to data.
If any other solution is not possible,somebody else may take the bug.
(In reply to Nihar Mehta from comment #4)
> When I try to build it keeping it in C/D drive of windows. I am getting
> error Permission denied (even after logging in as superuser)

Try to build on Windows. The way you are trying to setup your environment might require additional tinkering and hacking. It might not be the best way to start!

> Can somebody help me?

Please, use the #introduction IRC channel on irc.mozilla.org if you have trouble compiling the code base! :)

Comment 6

4 years ago
Can I work on it as my first contribution ? I've set up the Firefox build.
Nihar, any update on this?

sky@risetup.net - Hello and welcome! If Nihar is no longer working on this you can take on this bug! Let's just wait a little bit before proceeding.
Flags: needinfo?(niharmehta79)
(Assignee)

Comment 8

4 years ago
I am installing Linux again. I have created 150 GB free space and will install Linu be building mozilla by tonight.
I also have the bundle ready with me. So,it won't take much time. I am continuing to work on this bug.Sorry for the delay.

#sky@risetup.net- If possible,could you start on some other bug?
Flags: needinfo?(niharmehta79)

Comment 9

4 years ago
Yes, no problem :-)
(Assignee)

Comment 10

4 years ago
Created attachment 8531692 [details] [diff] [review]
Wrapped [aWrappedParameters] before checking [mShuttingDown]

I have build mozilla and tried to create a patch on this bug. Will only these changes suffice? If not, please guide me what further changes I must make.
Attachment #8531692 - Flags: review?(alessio.placitelli)
Assignee: nobody → niharmehta79
Comment on attachment 8531692 [details] [diff] [review]
Wrapped [aWrappedParameters] before checking [mShuttingDown]

Nihar, thank you for submitting the patch!

It appears that it was generated by using diff against two files which were both modified. Could you please follow the guidelines in [1] and submit the patch again?

Please make sure you don't add any new blank lines or trailing spaces as well!

Thanks!

[1] - https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8531692 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 12

4 years ago
Created attachment 8531988 [details] [diff] [review]
Bug 1102223: Wrapped [aWrappedParameters] before checking [mShuttingDown]

I have attached the patch for review.
Attachment #8531692 - Attachment is obsolete: true
Attachment #8531988 - Flags: review?(alessio.placitelli)
Comment on attachment 8531988 [details] [diff] [review]
Bug 1102223: Wrapped [aWrappedParameters] before checking [mShuttingDown]

Nihar, the patch you attached only contains the header and no content.
Attachment #8531988 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 14

4 years ago
Created attachment 8531996 [details] [diff] [review]
Bug-1102223.patch

Sorry.. that command had erased the previous content. i did not notice it.
Attachment #8531988 - Attachment is obsolete: true
Attachment #8531996 - Flags: review?(alessio.placitelli)
Comment on attachment 8531996 [details] [diff] [review]
Bug-1102223.patch

No problem. Unfortunately, the patch you provided cannot be applied to the codebase, but we're almost there!

Please follow the guidelines in the previous posts to generate a patch that can be reviewed and checked in. It looks like you appended the first diff to the previous empty patch.

You can generate a working patch by doing the following:

1) Start from a clean repository.
2) Use the "hg qnew" command to create a new patch (see the previous guide).
3) Directly modify the NativeFileWatcherWin.cpp file Do not create a NativeFileWatcherWinnew.cpp and get a diff of the two.
4) Issue the "hg qrefresh".
5) Export the patch with "hg export qtip >" command.

If you've got questions, please feel free to drop by in the IRC channel!
Attachment #8531996 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 16

4 years ago
Created attachment 8532120 [details] [diff] [review]
Bugfixer.patch

Well I figured out "hg add" command is necessary before creating the patch or reverting any file.However the patch which is obtained after following the instructions is the whole file and not just the changes made.I have attached it. Can you suggest what else I can do for it?
Attachment #8531996 - Attachment is obsolete: true
Attachment #8532120 - Flags: review?(alessio.placitelli)
Comment on attachment 8532120 [details] [diff] [review]
Bugfixer.patch

Nihar, no "hg add" is needed when you are only modifying a file which is already under version control (at least to my knowledge). Apparently your patch has no parent.

I think there are two ways you can resolve this issue and produce a correct patch:

1) Pop all your patches, revert to a known working changeset, follow the instructions on MDN or in the previous posts to produce a working patch.

2) Clone the whole repository again. This option might take some time but, if you played too much with the local repository, it might be the easiest way to continue working.
Attachment #8532120 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 18

4 years ago
Created attachment 8532862 [details] [diff] [review]
Bug 1102223: Wrapped [aWrappedParameters] before checking [mShuttingDown]

Added patch for review
Attachment #8532120 - Attachment is obsolete: true
Attachment #8532862 - Flags: review?(alessio.placitelli)
Comment on attachment 8532862 [details] [diff] [review]
Bug 1102223: Wrapped [aWrappedParameters] before checking [mShuttingDown]

Thanks for attaching this patch, it looks good! You just forgot to provide a commit message. Please see [1] for Mozilla's commit message conventions.

I've pushed the patch to the try server for you, that's required before a patch can land [2].

Keep up the great work!

[1] - https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

[2] - 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8c409fc6dae3
Attachment #8532862 - Flags: review?(alessio.placitelli) → feedback+
(Assignee)

Comment 20

4 years ago
So, what next do I have to do?
(In reply to Nihar Mehta from comment #20)
> So, what next do I have to do?

You should add a commit message to your patch and attach the new patch here. Please check the guides in the previous posts for the commit message format and the command to add/update a commit message.
(Assignee)

Comment 22

4 years ago
Created attachment 8533671 [details] [diff] [review]
Bug.patch

Added patch for review along with commit message
Attachment #8532862 - Attachment is obsolete: true
Attachment #8533671 - Flags: review?(alessio.placitelli)
Comment on attachment 8533671 [details] [diff] [review]
Bug.patch

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

Nihar, please change the commit message according to the guidelines in [1] and upload the patch again. A possible commit message could be

Bug 1102223 - Wrapping |aWrappedParameters| before checking for shutdown to prevent memory leaks.

[1] - https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions
Attachment #8533671 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 24

4 years ago
Created attachment 8533684 [details] [diff] [review]
Bug.patch

Added the above commit message.
Attachment #8533671 - Attachment is obsolete: true
Attachment #8533684 - Flags: review?(alessio.placitelli)
Comment on attachment 8533684 [details] [diff] [review]
Bug.patch

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

Just a nit: please remove the newline in the commit message before "prevent memory leaks" and attach the updated patch.

Other than that, it looks good to me.

Once this is done, we can request a review from Yoric.
Attachment #8533684 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 26

4 years ago
Created attachment 8533696 [details] [diff] [review]
Bug.patch

Removed the newline and attached the patch for review.
Attachment #8533684 - Attachment is obsolete: true
Attachment #8533696 - Flags: review?(alessio.placitelli)
Comment on attachment 8533696 [details] [diff] [review]
Bug.patch

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

Nihar, thanks for fixing the newline. Unfortunately, it appears that you removed the initial "Bug 1102223 - " from the commit message.

Could you please fix this, make sure the commit message is consistent with the previous observations and attach the updated patch?

Thanks for working on this!
Attachment #8533696 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 28

4 years ago
Created attachment 8533704 [details] [diff] [review]
Bug.patch

Added  the initial "Bug 1102223 - " to the commit message.
Attachment #8533696 - Attachment is obsolete: true
Attachment #8533704 - Flags: review?(alessio.placitelli)
Comment on attachment 8533704 [details] [diff] [review]
Bug.patch

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

Congrats Nihar!

The patch looks good to me. Since you only changed the commit message, I think there's no need to push again to the try server.

Yoric should review the patch before it can land.
Attachment #8533704 - Flags: review?(dteller)
Attachment #8533704 - Flags: review?(alessio.placitelli)
Attachment #8533704 - Flags: feedback+
Comment on attachment 8533704 [details] [diff] [review]
Bug.patch

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

This looks good. Thanks, Nihar.
Just one last thing before we can land this patch: can you add "r=Dexter,Yoric" at the end of your commit message? This helps us keep track of people who reviewed patches.
Attachment #8533704 - Flags: review?(dteller) → review+
(Assignee)

Comment 31

4 years ago
Created attachment 8534618 [details] [diff] [review]
Bug.patch

Welcome.Attached the patch with the updated commit message.
Flags: needinfo?(dteller)
Attachment #8534618 - Flags: review?(dteller)
Comment on attachment 8534618 [details] [diff] [review]
Bug.patch

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

Looks good to me, thank you very much.
Since Dexter has already pushed to Try, let's land this.
Attachment #8534618 - Flags: review?(dteller) → review+
Attachment #8533704 - Attachment is obsolete: true
Flags: needinfo?(dteller)
https://hg.mozilla.org/integration/fx-team/rev/477d76b9d58d
Keywords: checkin-needed
Whiteboard: [lang=c++] [good first bug] → [lang=c++] [good first bug][fixed-in-fx-team]
(Assignee)

Comment 34

4 years ago
So,is the bug fixed? Or do I have to do anything else for it?
Once the sheriffs (and the automated testing infrastructure) have confirmed that your fix hasn't broken anything else, they will mark the bug as fixed.

You will be expected to celebrate :)

y the way, do you already have an account on mozillians.org?
(Assignee)

Comment 36

4 years ago
No, I didn't.
Though, I have created it now. 
https://mozillians.org/en-US/u/niharmehta/
https://hg.mozilla.org/mozilla-central/rev/477d76b9d58d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=c++] [good first bug][fixed-in-fx-team] → [lang=c++] [good first bug]
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.