Closed Bug 16496 Opened 25 years ago Closed 25 years ago

MLK: Leaking nsLayoutHistoryState objects

Categories

(Core Graveyard :: Tracking, defect, P3)

Tracking

(Not tracked)

VERIFIED FIXED

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.
Target Milestone: M11
Status: NEW → ASSIGNED
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.
Fixing this leak causes a crash in layout. Eric Pollmann is investigating.
Blocks: 17432
Adding myself to CC list.  This crash seems likely to be caused by imbalanced
addref/releases in session history.  I'm looking at it.
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.
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
I've about got this pegged.  It's definitely a bug in SH, not in layout...
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
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.
Adding verifyme keyword.
Keywords: verifyme
Updating QA Contact.
QA Contact: leger → claudius
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
No longer blocks: 17432
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.