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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Alessio Placitelli [:Dexter]]

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.

Comment 1

[Nihar Mehta]

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?

Comment 2

[Alessio Placitelli [:Dexter]]

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.

Comment 3

[Nihar Mehta]

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.

Comment 4

[Nihar Mehta]

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.

Comment 5

[Alessio Placitelli [:Dexter]]

(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

[Vincent Lequertier]

Can I work on it as my first contribution ? I've set up the Firefox build.

Comment 7

[Alessio Placitelli [:Dexter]]

Nihar, any update on this?

[e-mail] - 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.

Comment 8

[Nihar Mehta]

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.

#[e-mail]- If possible,could you start on some other bug?

Comment 9

[Vincent Lequertier]

Yes, no problem :-)

Comment 10

[Nihar Mehta]

Attachment: 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.

Comment 11

[Alessio Placitelli [:Dexter]]

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

Comment 12

[Nihar Mehta]

Attachment: Bug 1102223: Wrapped [aWrappedParameters] before checking [mShuttingDown]

I have attached the patch for review.

Comment 13

[Alessio Placitelli [:Dexter]]

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.

Comment 14

[Nihar Mehta]

Attachment: Bug-1102223.patch

Sorry.. that command had erased the previous content. i did not notice it.

Comment 15

[Alessio Placitelli [:Dexter]]

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!

Comment 16

[Nihar Mehta]

Attachment: 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?

Comment 17

[Alessio Placitelli [:Dexter]]

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.

Comment 18

[Nihar Mehta]

Attachment: Bug 1102223: Wrapped [aWrappedParameters] before checking [mShuttingDown]

Added patch for review

Comment 19

[Alessio Placitelli [:Dexter]]

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

Comment 20

[Nihar Mehta]

So, what next do I have to do?

Comment 21

[Alessio Placitelli [:Dexter]]

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

Comment 22

[Nihar Mehta]

Attachment: Bug.patch

Added patch for review along with commit message

Comment 23

[Alessio Placitelli [:Dexter]]

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

Comment 24

[Nihar Mehta]

Attachment: Bug.patch

Added the above commit message.

Comment 25

[Alessio Placitelli [:Dexter]]

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.

Comment 26

[Nihar Mehta]

Attachment: Bug.patch

Removed the newline and attached the patch for review.

Comment 27

[Alessio Placitelli [:Dexter]]

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!

Comment 28

[Nihar Mehta]

Attachment: Bug.patch

Added  the initial "Bug 1102223 - " to the commit message.

Comment 29

[Alessio Placitelli [:Dexter]]

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.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 31

[Nihar Mehta]

Attachment: Bug.patch

Welcome.Attached the patch with the updated commit message.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/477d76b9d58d

Comment 34

[Nihar Mehta]

So,is the bug fixed? Or do I have to do anything else for it?

Comment 35

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 36

[Nihar Mehta]

No, I didn't.
Though, I have created it now. 
https://mozillians.org/en-US/u/niharmehta/

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/477d76b9d58d