Status

ASSIGNED
9 years ago
2 months ago

People

(Reporter: mathstuf, Assigned: mathstuf)

Tracking

({memory-leak})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

9 years ago
User-Agent:       Uzbl (Webkit 1.1.15) (GNU/Linux i686 [i686]) (Commit 38ef86e4d1827338495ff52fb25a6808cf4e45d2)
Build Identifier: nss-3.12.6-1.2.fc13.i686

While going through fixing memory leaks in CHASM[1], I fixed some leaks                         
within NSPR. Here's a list of the leaks that CHASM exposed                       
(doing minimal things with NSS, basically just hashing):

  * The error tables are not cleaned up. This is the most invasive                
    change since it adds a new public function                                                                                                   
    (nspr_CleanupPRErrorTables) which cleans up *all* error tables. This                                                                         
    means that anything that calls this cleans up every error table that                                                                         
    has been installed. Unfortunately, I see no better way of handling                
    this since everything is already not threadsafe. This is then called                                    
    in PR_Cleanup (The positioning may be too early, but I put it in                           
    reverse order that PR_InitStuff does). A note was also added to the               
    PR_Cleanup comment.                                                                                                                          
  * Clean up the TPD pointer within _PR_CleanupTPD.

[1]http://chasmd.org

Reproducible: Always
(Assignee)

Comment 1

9 years ago
Created attachment 437676 [details] [diff] [review]
Patch to fix memory leaks
Attachment #437676 - Attachment is patch: true
Attachment #437676 - Attachment mime type: application/octet-stream → text/plain
Attachment #437676 - Flags: review?(wtc)
Assignee: wtc → mathstuf
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: mlk
Summary: [PATCH] Memory leaks → Memory leaks
(Assignee)

Comment 2

9 years ago
Ping?
This patch needs a review.
Whiteboard: [MemShrink]
Summary: Memory leaks → Memory leaks in NSPR and NSS

Updated

7 years ago
Attachment #437676 - Flags: review?(ted.mielczarek)

Updated

7 years ago
Summary: Memory leaks in NSPR and NSS → Memory leaks in NSPR
Comment on attachment 437676 [details] [diff] [review]
Patch to fix memory leaks

I'm sorry, but I don't know this code well enough to feel comfortable reviewing it.
Attachment #437676 - Flags: review?(ted.mielczarek)

Comment 5

7 years ago
Comment on attachment 437676 [details] [diff] [review]
Patch to fix memory leaks

Thank you for the patch.  I took a quick look at the patch.
It seems correct.  I suggest some changes.

1. It doesn't seem necessary to have two functions
nspr_CleanupPRErrorTables and PR_ErrorCleanupTables
that do the same thing.

I think we can remove nspr_CleanupPRErrorTables and
just use PR_ErrorCleanupTables.

2. PR_ErrorCleanupTables should be an internal function,
called by PR_Cleanup only.

In nsprpub/pr/src/linking/prlink.c:

>+    PR_FREEIF(pr_exe_loadmap->name);
>+    PR_FREEIF(pr_exe_loadmap);

pr_exe_loadmap->name should be freed with free() because
it is allocated with strdup().

In nsprpub/pr/src/misc/prerrortable.c:

>+    while (et) {
>+        struct PRErrorTableList *cet;
>+
>+        cet = et->next;

Nit: rename the variable 'cet' to 'next' or 'next_et'.

In nsprpub/pr/src/misc/prinit.c:

>+ *   PR_Cleanup() calls nspr_CleanupPRErrorTables() which invalidates
>+ *   all error tables that have been installed.
>+ *

It is not necessary to add this comment.  We don't enumerate all the
cleanups performed by PR_Cleanup().

In nsprpub/pr/src/threads/prtpd.c:

> void _PR_CleanupTPD(void)
> {
>+    PR_FREEIF(_pr_tpd_destructors);
> }  /* _PR_CleanupTPD */

I don't know why _PR_CleanupTPD is a no-op.  It should
set _pr_tpd_destructors to NULL after freeing, and
also set _pr_tpd_length and _pr_tpd_highwater to 0.
Attachment #437676 - Flags: review?(wtc) → review-
Whiteboard: [MemShrink] → [MemShrink:P3]
(Assignee)

Comment 6

7 years ago
Created attachment 609976 [details] [diff] [review]
Updated patch

Updated per comments. My CVS checkout has bitrotted to some extent (cvs diff was taking forever at the toplevel), so the patch is based in the nsprpub directory.
Attachment #437676 - Attachment is obsolete: true

Comment 7

3 years ago
Created attachment 8692293 [details] [diff] [review]
nspr-errortable-memleak.patch

Proposed patch for the PR_ErrorInstallTable() memory leak.

Add new nspr_CleanupPRErrorTable() function to destroy only the nspr error table.
Add new public function PR_ErrorRemoveTable() to remove one entry from the error table and call it from nspr_CleanupPRErrorTable().

Comment 8

3 years ago
Created attachment 8692295 [details] [diff] [review]
nspr-tpd-memleak.patch

Proposed patch for the TPD memory leak.

Free the TBD data in _PR_CleanupTPD(), and add a call to _PR_CleanupTPD to _PR_Cleanup() nspr/pr/src/pthreads/ptthread.c.

Comment 9

3 years ago
Created attachment 8692299 [details] [diff] [review]
nspr-loadmap-memleak.patch

Proposed patch for the pr_exe_loadmap memory leak.

Add a call of PR_UnloadLibary(pr_exe_load_map) to _PR_ShutdownLinker(), and set pr_exe_load_map to NULL thereby bypassing the dereference PR_LoadStaticLibrary().

Comment 10

2 months ago
Comment on attachment 8692299 [details] [diff] [review]
nspr-loadmap-memleak.patch

Let's discuss this change in bug 96122, where you also attached the patch. It has more comments. Marking as obsolete in this bug.
Attachment #8692299 - Attachment is obsolete: true

Comment 11

2 months ago
Created attachment 8994783 [details] [diff] [review]
ben-merged-v3.patch

Looking at the old patches, it seems that Ben had correctly addressed Wan-Teh's review comments in 2012, but the newer attached patch (comment 6) was not handled. It still applies with a trivial merge necessary.

I think we should commit that patch.

Wan-Teh had requested to rename "cet" to "next_et" inside _PR_ErrorCleanupTables, which wasn't done. I'll make that change.

Looking at the names of the other cleanup functions, they all appear to being with _PR_ErrorCleanupTables. Therefore it seems more consistens to rename _PR_ErrorCleanupTables to _PR_CleanupErrorTables. It also allows one to find the function when searching for the string ErrorTable. I'll make that change.

I'm attaching an updated version of Ben's patch. I'm giving it r=kaie based on Wan-Teh's review. I think ny renames don't need a review.
Attachment #609976 - Attachment is obsolete: true
Attachment #8994783 - Flags: review+

Comment 12

2 months ago
hduston: IIUC you attempted to provide an alternative implementation to the patch that already existed. Do you think your patches are better than the one that existed? If yes, could you please explain why?

Comment 14

2 months ago
Prior to uplifting to firefox, let's check it passes all firefox tests.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd345196a91432a20dc912f355630419bfb1e945

Updated

2 months ago
Target Milestone: --- → 4.20

Comment 15

2 months ago
The try build shows multiple crashes in tests on Linux and Android, so something still depends on the items that are cleaned up. I'll revert the commit.

Updated

2 months ago
Attachment #8994783 - Flags: review-
Attachment #8994783 - Flags: review+
Attachment #8994783 - Flags: checked-in+

Comment 17

2 months ago
Created attachment 8995264 [details] [diff] [review]
Updated nspr-tpd-memleak.patch

Added zeroing of static tpd_length and tpd_highwater values.
Attachment #8692295 - Attachment is obsolete: true

Comment 18

2 months ago
(In reply to Kai Engert (:kaie:) from comment #12)
> hduston: IIUC you attempted to provide an alternative implementation to the
> patch that already existed. Do you think your patches are better than the
> one that existed? If yes, could you please explain why?

The most significant difference, in my view, is that I supplied multiple patches.  Each one addressed one and only one unfreed-allocation-at-exit issue.  This permits each one to be applied independently.

I have attached an updated tpd-memleak patch which zeroes the two static values as the original multi-unfreed patch did. The call to the cleanup function remains in my chosen location, between CleanupEnv and DestroyZones, as my initial analysis leads me to conclude that is the more correct sequence.

Comment 19

2 months ago
(In reply to Kai Engert (:kaie:) from comment #14)
> Prior to uplifting to firefox, let's check it passes all firefox tests.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bd345196a91432a20dc912f355630419bfb1e945

If you decide to commit/test any of my patches, would you mind indulging me and run a "before" baseline, so as to verify that none of the failures preexist these patches?

Comment 20

2 months ago
Created attachment 8995561 [details] [diff] [review]
Even more updated nspr-tpd-memleak.patch

Corrected to apply against a vanilla tree not already patched with the nspr-errortable-memleak.patch.
Attachment #8995264 - Attachment is obsolete: true

Comment 21

2 months ago
Created attachment 8995738 [details] [diff] [review]
tpd patch to be against 4.19

I just realized I was patching against a 4.10 tree instead of a 4.19 tree.  Oops.
Attachment #8995561 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.