Closed Bug 14967 Opened 21 years ago Closed 21 years ago

crash hitting back or forward button

Categories

(Core :: Layout: Form Controls, defect, P3, critical)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: buster, Assigned: radha)

References

()

Details

open apprunner to bugzilla query page
look for all bugs submitted by buster, no other fields filled in
on the results page, sort by bug #
hit back button, let page redisplay
hit back button again, get the crash below because aState is garbage:

FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0284d0b0, nsILayoutHistoryState * 0x02bd3790) line 1102 + 21 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0284d790, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283a090, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02838f10, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283a570, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02839ee0, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283b7f0, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283b900, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283e120, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283d080, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x0283d140, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x027cd1a0, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02b5df60, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02b2fad0, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02b2d030, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02b2d170, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02bd3030, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02a26800, nsILayoutHistoryState * 0x02bd3790) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x02ae4cc0, nsIFrame *
0x02aefa80, nsILayoutHistoryState * 0x02bd3790) line 1120 + 20 bytes
PresShell::ContentAppended(PresShell * const 0x02ae10d8, nsIDocument *
0x02a264f0, nsIContent * 0x02ae5d3c, int 8) line 1735
nsDocument::ContentAppended(nsDocument * const 0x02a264f0, nsIContent *
0x02ae5d3c, int 8) line 1574
nsHTMLDocument::ContentAppended(nsHTMLDocument * const 0x02a264f0, nsIContent *
0x02ae5d3c, int 8) line 1044
HTMLContentSink::NotifyBody() line 276
HTMLContentSink::CloseForm(HTMLContentSink * const 0x02a249e0, const
nsIParserNode & {...}) line 2113
CNavDTD::CloseForm(const nsIParserNode & {...}) line 2478 + 31 bytes
CNavDTD::CloseContainer(const nsIParserNode & {...}, nsHTMLTag eHTMLTag_form,
int 0) line 2719 + 12 bytes
CNavDTD::HandleEndToken(CToken * 0x01cf13e0) line 1500 + 24 bytes
NavDispatchTokenHandler(CToken * 0x01cf13e0, nsIDTD * 0x02ae4960) line 257 + 12
bytes
CTokenHandler::operator()(CToken * 0x01cf13e0, nsIDTD * 0x02ae4960) line 80 + 14
bytes
CNavDTD::HandleToken(CNavDTD * const 0x02ae4960, CToken * 0x01cf13e0, nsIParser
* 0x02a24b50) line 754 + 18 bytes
CNavDTD::BuildModel(CNavDTD * const 0x02ae4960, nsIParser * 0x02a24b50,
nsITokenizer * 0x02ae6fb0, nsITokenObserver * 0x00000000, nsIContentSink *
0x02a249e0) line 564 + 20 bytes
nsParser::BuildModel() line 1001 + 34 bytes
nsParser::ResumeParse(nsIDTD * 0x00000000, int 0) line 922 + 11 bytes
nsParser::OnDataAvailable(nsParser * const 0x02a24b54, nsIChannel * 0x02a58140,
nsISupports * 0x00000000, nsIInputStream * 0x02a228e8, unsigned int 0, unsigned
int 2399) line 1337 + 19 bytes
nsDocumentBindInfo::OnDataAvailable(nsDocumentBindInfo * const 0x02a582d0,
nsIChannel * 0x02a58140, nsISupports * 0x00000000, nsIInputStream * 0x02a228e8,
unsigned int 0, unsigned int 2399) line 1371 + 32 bytes
nsChannelListener::OnDataAvailable(nsChannelListener * const 0x02a58210,
nsIChannel * 0x02a58140, nsISupports * 0x00000000, nsIInputStream * 0x02a228e8,
unsigned int 0, unsigned int 2399) line 1613
nsHTTPResponseListener::OnDataAvailable(nsHTTPResponseListener * const
0x02a23550, nsIChannel * 0x02a5aaa0, nsISupports * 0x02a58140, nsIInputStream *
0x02a228e8, unsigned int 28672, unsigned int 2399) line 186 + 47 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x02828910)
line 359
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x028288c0) line 152 + 12 bytes
PL_HandleEvent(PLEvent * 0x028288c0) line 541 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d1a850) line 500 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00180272, unsigned int 49308, unsigned int 0,
long 13740112) line 970 + 9 bytes
just got the crash again.  this time the URL sequence was:
www.search.com, enter "nissan xterra"
click on first hit: http://www.go.com/Center/Automotive/Spec/Nissan_Xterra
click on
http://www.go.com/Center/Automotive/Spec/Research_tools_and_resources_for_buying
_a_car/Makes_and_models/Nissan/Nissan_Xterra?style1=SE+4X2+Year+2000& make1=&
cid1=& cs1=& cid2=& cs2=& m1=& m2=& vnow (link name = SE 4x2 2000)
click on the image to go to
http://www.go.com/Center/Automotive/Spec/Research_tools_and_resources_for_buying
_a_car/Makes_and_models/Nissan/Nissan_Xterra?style1=SE+4X2+Year+2000&
apg=large_pic& cs1=SE+4X2+Year+2000& cid1=28215& m1=Nissan& make1=Nissan&
svx=autos_toolbox_large_pi
hit the back button
reproduced twice.



stack:
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a64f10, nsILayoutHistoryState * 0x02b7d150) line 1102 + 21 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a5b0d0, nsILayoutHistoryState * 0x02b7d150) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a5ba80, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a5bb20, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a5eb80, nsILayoutHistoryState * 0x02b7d150) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a59890, nsILayoutHistoryState * 0x02b7d150) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a5a1e0, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a4da40, nsILayoutHistoryState * 0x02b7d150) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a4e0b0, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a4e950, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a4e7c0, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a39d00, nsILayoutHistoryState * 0x02b7d150) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a447c0, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a44900, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a40730, nsILayoutHistoryState * 0x02b7d150) line 1127 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a46990, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a47aa0, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a463b0, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a47040, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02a44040, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
FrameManager::RestoreFrameState(FrameManager * const 0x028f9840, nsIFrame *
0x02918030, nsILayoutHistoryState * 0x02b7d150) line 1120 + 20 bytes
PresShell::ContentAppended(PresShell * const 0x028f8658, nsIDocument *
0x0301ab30, nsIContent * 0x02915f3c, int 0) line 1735
nsDocument::ContentAppended(nsDocument * const 0x0301ab30, nsIContent *
0x02915f3c, int 0) line 1574
nsHTMLDocument::ContentAppended(nsHTMLDocument * const 0x0301ab30, nsIContent *
0x02915f3c, int 0) line 1044
HTMLContentSink::NotifyBody() line 276
HTMLContentSink::WillInterrupt(HTMLContentSink * const 0x0301bf10) line 1770
CNavDTD::WillInterruptParse(CNavDTD * const 0x028fa4a0) line 3111 + 27 bytes
nsParser::ResumeParse(nsIDTD * 0x00000000, int 0) line 958
nsParser::OnDataAvailable(nsParser * const 0x0301a184, nsIChannel * 0x023e4790,
nsISupports * 0x00000000, nsIInputStream * 0x03018c38, unsigned int 0, unsigned
int 4208) line 1337 + 19 bytes
nsDocumentBindInfo::OnDataAvailable(nsDocumentBindInfo * const 0x023e3760,
nsIChannel * 0x023e4790, nsISupports * 0x00000000, nsIInputStream * 0x03018c38,
unsigned int 0, unsigned int 4208) line 1371 + 32 bytes
nsChannelListener::OnDataAvailable(nsChannelListener * const 0x028ca8e0,
nsIChannel * 0x023e4790, nsISupports * 0x00000000, nsIInputStream * 0x03018c38,
unsigned int 0, unsigned int 4208) line 1613
nsHTTPResponseListener::OnDataAvailable(nsHTTPResponseListener * const
0x03018cb0, nsIChannel * 0x030144c0, nsISupports * 0x023e4790, nsIInputStream *
0x03018c38, unsigned int 28672, unsigned int 4208) line 186 + 47 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x02a1e560)
line 359
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x02a18fd0) line 152 + 12 bytes
PL_HandleEvent(PLEvent * 0x02a18fd0) line 541 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d1a850) line 500 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x041308e4, unsigned int 49308, unsigned int 0,
long 13740112) line 970 + 9 bytes
Summary: crash hitting back button → crash hitting back or forward button
I see this also when clicking the Forward button (as well as the Back button)
using the 1999092708 build on NT.
I see this on linux also, marking platform all. To reproduce on linux simply:
1. Launch and watch http://www.mozilla.org come up as start page
2. Enter http://www.yahoo.com into location bar and hit enter
3. Watch yahoo load and hit the back arrow
4. Watch mozilla page load again and hit forward button

You will then crash. Chofmann said to cc: troy on this one.
I have nothing to do with the restore frame code. Changing Cc to Nisheeth
OS: Windows NT → All
Hardware: PC → All
just crashed with the 1999092608 build on MacOS 8.5, very similar stack trace.
Changing platform to All for real this time ;-)
Assignee: karnaze → radha
Reassigning to Radha.
Target Milestone: M10
putting on my m10 radar.
cc'ing Pollmann. I don't see webshell or session History anywhere in the stack.
So, I'm not sure if the aState is null because webshell didn't pass the right
state. I think eric will know better.
Radha, could it be that the state has been NS_RELEASED?  I'll take a look to see
what's going on.
I'd bet a dollar or twenty that Warren caused this bug with his most recent
checking to webshell, which coincidentally, was to fix a history state leak on
09/26.

Warren, Session history must maintain a reference to the session history until
it is deleted from the list completely.  The pages may be revisited multiple
times.
s/must maintain a reference to the session history/must maintain a reference to
the page state/g;  :S
Radha, one way to fix this might be to make nsWebshell::mHistory be an
nsISupportsArray, have nsHistoryEntry implement nsISupports, and write a proper
destructor that will free the page state. (RemoveElementAt of nsISupportsArray
calls NS_RELEASE on the element).

In the meantime, you might want to do something quicker like back out Warren's
fix, manually call NS_RELEASE before calling mHistory.RemoveElementAt(), and
manually NS_RELEASE each element of the history array when you are done with
it.  I'll check this in my tree to see if it fixes the bug.  Unfortunately,
Purify doesn't like my 64Mb machine, so I can't as easily check to see if it
causes massive leaks to reappear.
Backed out Warren's leak fix in my local tree and the crash went away.  Here is
the diff:

Index: nsWebShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/webshell/src/nsWebShell.cpp,v
retrieving revision 1.261
diff -r1.261 nsWebShell.cpp
2153c2153
<    nsCOMPtr<nsISupports> historyState;
---
>    nsISupports* historyState;
2156c2156
<    rv = GetHistoryState(getter_AddRefs(historyState));
---
>    rv = GetHistoryState(&historyState);

I'd strongly recommend talking to Warren before backing this out, as I believe
this is exactly the type of leak that he, Vidur, and Rick Potts have been
killing themselves over lately.
Well, my simple test must have not been sufficient to get at the behavior here.
I was going to a new page, going back, then going forward to somewhere else
(presumably pruning the history entry), but I saw it leaked. Also, I thought I
saw these being leaked on shutdown.

I'd argue that the way this code was written it wasn't clear that it didn't
leak. Usually the caller doesn't addref for the called method (in this case
SetHistoryObjectForIndex) -- that's confusing.
Status: NEW → ASSIGNED
Eric, warren,

I saw your comments. The history state is being released by session History in
nsSessionHistory.cpp, when a page no-longer becomes reachable. So, I don't know
why it is leaking there. I just now got my copy of purify. I'll look in to it.

Radha
I have to agree with Warren here.  In fact, I'd leave his change in, then
instead, addref aState inside nsSessionHistory::SetHistoryState:

Index: nsSessionHistory.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsSessionHistory.cpp,v
retrieving revision 1.20
diff -r1.20 nsSessionHistory.cpp
268a269,270
>   NS_IF_RELEASE(mHistoryState);
>   NS_ADDREF(aState);

(This also fixes the crash)

You are already NS_IF_RELEASING mHistoryState on destruction, so I assume you
had planned on maintaining a reference to it?
radha, have you tried the fix eric suggests? we'd like to fix this rather
noticeable crashing behaviour before we branch m10
Hmmm, you're going to want to make that an NS_IF_ADDREF.  In the spirit of
dogfood, I was fixing another bug with my patched apprunner and noticed that
sometimes aState will be null and cause a crash.

Index: nsSessionHistory.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsSessionHistory.cpp,v
retrieving revision 1.20
diff -r1.20 nsSessionHistory.cpp
268a269,270
>   NS_IF_RELEASE(mHistoryState);
>   NS_IF_ADDREF(aState);
*** Bug 15011 has been marked as a duplicate of this bug. ***
I still the crash with pollman's patches. The problem is, the addref in
nsSessionHistory gets nullified when historyState goes out of scope in
nsWebShell::LoadURL(). The following patch + warren's changes + eric's changes
doesn't cause the crash.

chiffon:radha [15] cvs diff nsWebShell.cpp
Index: nsWebShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/webshell/src/nsWebShell.cpp,v
retrieving revision 1.261
diff -r1.261 nsWebShell.cpp
2789a2790
>                 NS_ADDREF(*aLayoutHistoryState);
If you just need to get something in for M10, this sounds fine.

I'll have to say though, 1) I don't see the crash and 2) after a little digging
around, it's seems strange to me that you need this extra addref.  Looking at
nsWebShell::LoadURI() there is:

  1) An addref in
nsWebShell::GetHistoryState->nsPresShell::GetHistoryState->NS_NewLayoutHistoryState

  2) An addref inside
nsSessioinHistory::SetHistoryObjectForIndex->nsHistoryEntry::SetHistoryState

  1) A release when the nsCOMPtr "historyState" goes out of scope.

This should leave you with a refcount of one, and you should not loose the
instance.  Is there yet another extra release somewhere I'm missing?
I think Radha found the extra release.  She also suggested adding an addref to
GetHistoryState.  We're working through the refcounting via email now...
Here's the leak I was trying to fix:

[W] MLK: Memory leak of 16 bytes from 1 block allocated in
NS_NewLayoutHistoryState(nsILayoutHistoryState * *)
    Distribution of leaked blocks
    Allocation location
        new(UINT)      [new.cpp:23]
        NS_NewLayoutHistoryState(nsILayoutHistoryState * *)
[nsLayoutHistoryState.cpp:77]
        PresShell::GetHistoryState(nsILayoutHistoryState * *)
[nsPresShell.cpp:1644]
        nsWebShell::GetHistoryState(nsISupports * *) [nsWebShell.cpp:2789]
        nsWebShell::LoadURI(nsIURI *,char const*,nsIInputStream
*,int,UINT,UINT,nsISupports *,WORD const*) [nsWebShell.cpp:2156]
        nsWebShell::LoadURL(WORD const*,char const*,nsIInputStream
*,int,UINT,UINT,nsISupports *,WORD const*) [nsWebShell.cpp:2331]
        nsWebShell::LoadURL(WORD const*,nsIInputStream
*,int,UINT,UINT,nsISupports *,WORD const*) [nsWebShell.cpp:1927]
        nsBrowserWindow::DispatchMenuItem(int) [nsBrowserWindow.cpp:573]
        nsNativeBrowserWindow::DispatchMenuItem(int) [nsWinMain.cpp:122]
        HandleBrowserEvent [nsBrowserWindow.cpp:340]
        nsWindow::DispatchEvent(nsGUIEvent *,nsEventStatus&) [nsWindow.cpp:338]
        nsWindow::DispatchWindowEvent(nsGUIEvent *) [nsWindow.cpp:358]
        nsWindow::ProcessMessage(UINT,UINT,long,long *) [nsWindow.cpp:2248]
        nsWindow::WindowProc(HWND__ *,UINT,UINT,long) [nsWindow.cpp:447]
        TranslateMessageEx [user32.dll]
        DispatchMessageA [USER32.dll]
        nsNativeViewerApp::Run(void) [nsWinMain.cpp:71]
        main           [nsWinMain.cpp:135]
        mainCRTStartup [crtexe.c:338]
radha...is this fixed in today's build...as far as the crashing?
*** Bug 15072 has been marked as a duplicate of this bug. ***
*** Bug 15369 has been marked as a duplicate of this bug. ***
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
radha has checked into the m10 branch and I don't see the
crash in the Friday night m10 builds..  marking fixed
Status: RESOLVED → VERIFIED
verified
You need to log in before you can comment on or make changes to this bug.