Closed
Bug 16496
Opened 25 years ago
Closed 25 years ago
MLK: Leaking nsLayoutHistoryState objects
Categories
(Core Graveyard :: Tracking, defect, P3)
Core Graveyard
Tracking
Tracking
(Not tracked)
VERIFIED
FIXED
M11
People
(Reporter: beard, Assigned: radha)
References
()
Details
The extra NS_ADDREF on this line causes this leak. What was the reason for adding it in revision 1.265? The object should already have a reference count of 1.
Reporter | ||
Updated•25 years ago
|
Target Milestone: M11
Assignee | ||
Updated•25 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•25 years ago
|
||
That was not suppose to be there. Thought I had gotten rid of that line before I checked in. I'll remove it when the tree opens.
Assignee | ||
Comment 2•25 years ago
|
||
Fixing this leak causes a crash in layout. Eric Pollmann is investigating.
Comment 3•25 years ago
|
||
Adding myself to CC list. This crash seems likely to be caused by imbalanced addref/releases in session history. I'm looking at it.
Comment 4•25 years ago
|
||
Radha, I hope you are looking at this too. This is your code, and you are the expert, I can only guess what some of this is supposed to be doing.
Assignee | ||
Comment 5•25 years ago
|
||
Here's how I reproduced it. 1) Bring up apprunner with mozilla.org 2) Click on Community. ( History state for mozilla.org is saved in SH) 3) Go back (History state for community is saved in SH. History state for mozilla.org restored from SH and set on the page.) 4) Click on Community again. (#2 goes out of scope of SH. Its History state removed.) 5) Click on Bugzilla on the personal toolbar. (History state for #4 created and saved in SH. Now somewhere while loading bugzilla, RestoreFrameState() is called recursively and the History state for #2 which was removed in step #4 is accessed) 6) Crash
Comment 6•25 years ago
|
||
I've about got this pegged. It's definitely a bug in SH, not in layout...
Comment 7•25 years ago
|
||
The problem is not mismatched addref/releases, but instead, caching a stale pointer in the code Radha added to nsWebShell Go to page X Go to page Y Web shell gets frame state A from page X Session history saves state A in index 0 of mHistoryEntries Click Back button Web shell gets frame state B from page Y Session history saves state B in index 1 of mHistoryEntries **In LoadURL, session history logic sets mHistoryState to state A Pres shell restores state A to page X Go to page Z Web shell gets frame state C from page X Radha's pruning algorithm deletes state A from session history. - This is all as it should be - The problem is that nsWebShell's mHistoryState is currently holding on to state A. (above marked **) We are going to a new page now, and we don't have a history state for it, therefore nsWebShell's mHistoryState should be set to null. - In LoadURL we get a new aHistoryState of 0x000 - but Radha checks for null and therefore does not update mHistoryState - Therefore mHistoryState remains pointing to the deleted state A Pres shell restores state A to page 3 Crash. Radha, you need to remove the check for null that you put in nsWebShell::LoadURL before SetHistoryState(). I did this in my local tree and this fixed the bug. (in addition, of course, to removing the extraneous NS_ADD_REF() that you added in an earlier checkin) I determined this by setting breakpoints in your code, something I'm sure you can do just as well as I can. It took me almost a full day to do this because I am unfamiliar with your code. It might help if you drew a flowchart of your code so you could catch things like this. It also might help if you stepped though your own code in a debugger. I'm leaving it up to you to verify that this is the right thing to do, check this in, and watch the tree.
Comment 8•25 years ago
|
||
My sincere apologies if that sounded harsh. Me venting stress through bug reports is not a good thing. I know this is tough, and we're all working as hard as we can.
Assignee | ||
Updated•25 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•25 years ago
|
||
Eric, Thanks for looking in to it. I was under the wrong assumption that mHistoryState was used only when loads from History happen. Sorry about the trouble.
Comment 12•24 years ago
|
||
VERIFIED with 2000031009 builds. I did this following radha's steps to repro(1-6) and noting that it did not crash.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•