Firefox leaks 10 nsGlobalWindows with Page Load Error Page

VERIFIED FIXED in mozilla1.9beta4

Status

()

Core
General
P2
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Tomcat, Assigned: peterv)

Tracking

({memory-leak})

Trunk
mozilla1.9beta4
memory-leak
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 297537 [details]
leak log

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008011704 Minefield/3.0b3pre

Steps to reproduce:
Create a new Profile to make sure you do not run into an second leak
(in my case i found the bug after i imported IE data at the migration wizard).
Close Firefox, to make sure its not a follow up leak from the migration

Firefox starts with 2 Tabs (both the default ones - firstrun and default startpage) 
In the first tab go to a Page Load Error page. As example http://support.mozilla.org (i wanted to test the new Sumo Project and did a typo ..org instead the correct .com)
Wait till you get till you get the Page Load Error Page
Close Firefox

Leak:
 0 TOTAL 28  1056402   332664    37213 ( 1169.30 +/-  1268.80)  1065981   

Same Problem happen after restart when both tabs (the Page Load Error) and the default Homepage is restored

nsStringStats
 => mAllocCount:          19337
 => mReallocCount:         3070
 => mFreeCount:           15656  --  LEAKED 3681 !!!
 => mShareCount:          17258
 => mAdoptCount:           1692
 => mAdoptFreeCount:       1675  --  LEAKED 17 !!!


Note: during shutdown i got a lot of :
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file c:/debug/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 702

This was reproducible on Windows XP Debug Build and also on Vista Debug Build. I think this is a bad leak, since typos during surfing are a common thing...
Flags: blocking1.9?
(Reporter)

Comment 1

10 years ago
Created attachment 298174 [details]
new leak log

new test after peters patch was checked in :

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012014 Firefox/3.0b3pre

nsTraceRefcntImpl::DumpStatistics: 726 entries
nsDOMNodeAllocator leaked 128096 bytes 
nsStringStats
 => mAllocCount:          20094
 => mReallocCount:         3049
 => mFreeCount:           16792  --  LEAKED 3302 !!!
 => mShareCount:          15834
 => mAdoptCount:           1272
 => mAdoptFreeCount:       1269  --  LEAKED 3 !!!

Comment 2

10 years ago
so far no luck with reproducing on linux.
(Reporter)

Comment 3

10 years ago
Created attachment 298297 [details]
linux leak log

(In reply to comment #2)
> so far no luck with reproducing on linux.
> 

hmm, i'm still able to reproduce this, also on linux with Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre

can you reproduce this when you do the following steps:

-> Create a new profile (or rename the firefox profile folders)
-> Start Firefox
-> Firefox comes up with the firstrun tab and startpage
change the url of the firstrun page to a page that cause the Page Load Error Site (like support.mozilla.org)
-> When you see the error page, close Firefox and click if the tabs should be saved for the next start
-> wait till the 2nd tab with the start page is loaded and close firefox

Leak

Comment 4

10 years ago
Nope, still no leak, with new profile and all.
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b3pre) Gecko/2008012113 
(Assignee)

Comment 5

10 years ago
I have a hard time reproducing this too, but on OS X.
Anyone else able to reproduce?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
I'm going to leave this as a nom until we can confirm.
Flags: blocking1.9+ → blocking1.9?
Tomcat can reproduce on all platforms so confirming and upping priority.
Flags: blocking1.9? → blocking1.9+
Priority: P3 → P2
Great.  Thanks guys.
Peter, tomcat says he'll show you this one during the work week.
Assignee: nobody → peterv
(Reporter)

Updated

10 years ago
Blocks: 414514
(Reporter)

Updated

10 years ago
Blocks: 416607
(Assignee)

Comment 11

10 years ago
Created attachment 302868 [details] [diff] [review]
v1

Still looking for a better way to fix this. It looks like we keep tabs alive late into shutdown from session restore when we never got an onload notification for them. This happens for error pages and for pages that we've interrupted loading (for example by quitting while the page is loading).
(Assignee)

Updated

10 years ago
Duplicate of this bug: 416607
(Reporter)

Updated

10 years ago
Blocks: 417648

Comment 13

10 years ago
(In reply to comment #11)
> Created an attachment (id=302868) [details]

Should you want to go down that way, please (1) move this bit into sss_onClose, (2) add a comment as to why you're doing it, (3) delete the _tab property from |tabState| in sss_onTabClose as well and (4) ask me for review.

Comment 14

10 years ago
(In reply to comment #13)
> (1) move this bit into sss_onClose,
... either to the very end, or set _tab to a new object ({}) so as to not regress bug 415484.

> (4) ask me for review.
... provided, of course, that this fixes the actual issue.
(Assignee)

Comment 15

10 years ago
Created attachment 304718 [details] [diff] [review]
v2

Yes, this works too. And I've finally managed to track down the difference between regular page loads and error pages, onTabLoad is never called for error pages.
Attachment #302868 - Attachment is obsolete: true
Attachment #304718 - Flags: review?(zeniko)

Comment 16

10 years ago
Comment on attachment 304718 [details] [diff] [review]
v2

>+      aWindow.__SS_dyingCache.tabs[t]._tab = {};

Please only do this, when there's actually a _tab we were referencing.

>+    tabState._tab = {};

This one won't be needed any more, so you could also just |delete| it (one less object to carry around).

r=me with these two nits fixed.

Thanks for catching this bug, Peter!
Attachment #304718 - Flags: review?(zeniko) → review+
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> (From update of attachment 304718 [details] [diff] [review])
> >+      aWindow.__SS_dyingCache.tabs[t]._tab = {};
> 
> Please only do this, when there's actually a _tab we were referencing.

What would be the preferred way to do this?

Comment 18

10 years ago
(In reply to comment #17)
  if (aWindow.__SS_dyingCache.tabs[t]._tab) ...

OTOH, since you've put the code at the end of sss_onClose you can also just |delete| the _tab property (it's last needed for the _collectWindowData call above as an indicator whether the tab's been completely loaded: if it hasn't, we don't collect form and scroll data for it).
(Assignee)

Comment 19

10 years ago
Created attachment 305353 [details] [diff] [review]
v2.1

This is what I'll check in.
Attachment #304718 - Attachment is obsolete: true
Attachment #305353 - Flags: review+
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
OS: Windows Vista → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Comment 20

10 years ago
Tomcat: could you check whether any of the bugs that this one blocks are fixed too?
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

10 years ago
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022515 Firefox/3.0b4pre , thanks peter !
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.