20050803 trunk Firefox 1) Load http://forums.dpreview.com/forums/read.asp?forum=1019&message=14456378 and click on the link in the message. 2) Press Back button. 3) Click "Next" link. RESULT: Nothing happens. JS console shows tons of "undefined" errors. Reloading the page fixes it. Same thing happens when you view source and then close it. This definitely regressed between 20050727 and 20050803.

File against a likelier component. /be

regression range is 2005-07-30 to 2005-07-31-10, sounds like split window bug 296639 fall out

*** Bug 303237 has been marked as a duplicate of this bug. ***

Note that clicking links in step 3 in comment 0 is not necessary. All you have to do is mouse over the document to see: Error: menuopen is not defined Source File: http://www.dpreview.com/inc/menus.js Line: 139

*** Bug 303275 has been marked as a duplicate of this bug. ***

I set browser.sessionhistory.max_viewers to 0 on a clean profile with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050803 Firefox/1.0+ and didn't see this.

Is this a problem with bfcache disabled?

I posted Bug 303275, which is bfcache-specific, and it was marked as a duplicate of this. So yes, this can only be reproduced with bfcache enabled.

split window and bfcache interaction? bryner or jst, can you guys look into this?

I have another testcase that sounds like this bug, but there's more weirdness involved: 1. Go to http://www.allowe.com/ 2. Hover over "Laughs". 3. Click "Audio Humour". 4. Once the page has loaded, go back. The menu is _visible_, and unusable until you refresh the page. Could this be yet another focus-related issue?

http://www.lgphilips-lcd.com/homeContain/jsp/eng/prd/prd200_j_e.jsp Clicking any of the panel models on that page results in a "not defined.." error in the JS console. I don't think that this is strictly bfcache related though.

There's a lot of JavaScript going on on that page that's not "onload" but inline. bfcache does not execute that again, it could be that it needs some initialization to get back to a good state after clicking some types of links?

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050806 SeaMonkey/1.0a] (nightly) (W98SE) Confirming on SeaMonkey: *timeframe: mine was between -0729 and -0804. *behaviour: undefined JS objects. *bfcache related: bug with 3 or 5 viewers, no bug with 0. I see it on <http://www.battle-arenas.net/>; Other/alternate odd behaviour: on some URL (http, not js), 1st click reloads the current frame, "2nd" click needed to actually follow the link.

*** Bug 303822 has been marked as a duplicate of this bug. ***

Demonstrates the problem with comment 10. Note that I don't get any errors in the js console, and this is not a problem when bfcache is disabled.

It might be 2 different bugs, then. We have the bug that depends on bfcache and won't reproduce without enabling bfcache. Then there's the bug that can be reproduced independently from bfcache being enabled or not.

What does this bug have to do with RSS?

*** Bug 303883 has been marked as a duplicate of this bug. ***

Tweaking summary so that this bug can be easily located. Another testcase: http://wwp.greenwichmeantime.com/time-zone/usa/us-phone-codes.htm Enter a phone number and it will display a timezone. Go back, and you can't enter a number any more - none of the buttons work. Only after a reload will it work again. Javascript Console error upon fast back: Error: but4 is not defined Source File: http://wwp.greenwichmeantime.com/time-zone/usa/us-phone-codes.htm Line: 1 Disable fastback and all is well again. Note that this site throws a lot of other errors / warnings, but that (when it works) it works in spite of those.

- Load the testcase - Click the button, verify alert says "-1" - Load http://google.com/ - Click back - Click the button, note that you get an error that 'menuopen' is undefined

I figured out what's causing this. It's an interaction problem between fastback and splitwindows. The fastback code does property save/restore the property, the reason why the testcase doesn't see the property is that it's checking for "menuopen", w/o qualifying the name (window.menuopen works), and this makes mozilla look up the event handler's scope (its parent chain), which leads to the *old* inner window, not the new one where the bfcache (correctly) restored the property onto. Seems to me that this is something we really should fix by making bfcache hold on to inner window objects and not copy/restore JS properies.

*** Bug 304072 has been marked as a duplicate of this bug. ***

I have a partial patch that saves the whole inner window instead of copying the properties. It works, but is incomplete. I'll try to finish it tomorrow.

*** Bug 304121 has been marked as a duplicate of this bug. ***

*** Bug 303059 has been marked as a duplicate of this bug. ***

This holds onto the inner window through the bfcache instead of copying properties. My only real concern is that I'm not quite sure where we break the potential inner window -> document -> ... -> inner window cycle, once the inner window is in the bfcache. jst seemed to think this patch was safe in that regard, though. Bryner, do I need to add code somewhere (perhaps in the docshell?) anyway?

Comment on attachment 192450 [details] [diff] [review] hold onto inner windows Better patch coming.

Comment on attachment 192450 [details] [diff] [review] hold onto inner windows As far as I know, we detach the document from its window when we put it into session history, then hook it back up when restoring it. So how would that cycle happen? >--- docshell/base/nsDocShell.cpp 11 Aug 2005 20:14:00 -0000 1.719 >+++ docshell/base/nsDocShell.cpp 12 Aug 2005 00:28:29 -0000 >@@ -5220,22 +5220,26 @@ nsDocShell::RestoreFromHistory() > // Destroy() is called. > > if (mContentViewer) { > mContentViewer->Close(mSavingOldViewer ? mOSHE.get() : nsnull); > mContentViewer->Destroy(); > } > > mContentViewer.swap(viewer); > viewer = nsnull; // force a release to complete ownership transfer > >+ // Grab the window state up here so we can pass it to Open. >+ nsCOMPtr<nsISupports> windowState; >+ mLSHE->GetWindowState(getter_AddRefs(windowState)); >+ Set the window state to null on mLSHE here, so we're not holding onto it in two places (I'm a little paranoid about that). >--- docshell/base/nsIContentViewer.idl 15 Jun 2005 23:52:36 -0000 1.17 >+++ docshell/base/nsIContentViewer.idl 12 Aug 2005 00:28:29 -0000 >@@ -79,18 +79,18 @@ interface nsIContentViewer : nsISupports > * This is called when the DOM window wants to be closed. Returns true > * if the window can close immediately. Otherwise, returns false and will > * close the DOM window as soon as practical. > */ > > boolean requestWindowClose(); > > /** > * Attach the content viewer to its DOM window and docshell. > */ >- void open(); >+ void open(in nsISupports aState); Maybe comment on what aState is used for. > > /** > * Clears the current history entry. This is used if we need to clear out > * the saved presentation state. > */ > void clearHistoryEntry(); > }; Also, you should rev the iid of nsIDocumentViewer, since it inherits from nsIContentViewer. >--- dom/public/nsIScriptGlobalObject.h 30 Jul 2005 20:57:05 -0000 3.26 >+++ dom/public/nsIScriptGlobalObject.h 12 Aug 2005 00:28:29 -0000 >@@ -60,20 +61,21 @@ struct JSObject; > */ > > class nsIScriptGlobalObject : public nsISupports > { > public: > NS_DEFINE_STATIC_IID_ACCESSOR(NS_ISCRIPTGLOBALOBJECT_IID) > > virtual void SetContext(nsIScriptContext *aContext) = 0; > virtual nsIScriptContext *GetContext() = 0; > virtual nsresult SetNewDocument(nsIDOMDocument *aDocument, >+ nsISupports *aState, > PRBool aRemoveEventListeners, > PRBool aClearScope) = 0; Should rev the iid (NS_ISCRIPTGLOBALOBJECT_IID) for this change. >--- dom/src/base/nsGlobalWindow.cpp 10 Aug 2005 20:10:18 -0000 1.760 >+++ dom/src/base/nsGlobalWindow.cpp 12 Aug 2005 00:28:30 -0000 >+protected: >+ ~WindowStateHolder(); Since the destructor is now empty, it can just be inlined. >@@ -739,49 +838,61 @@ nsGlobalWindow::SetNewDocument(nsIDOMDoc > } > > nsRefPtr<nsGlobalWindow> newInnerWindow; > > nsCOMPtr<nsIDOMChromeWindow> thisChrome = > do_QueryInterface(NS_STATIC_CAST(nsIDOMWindow *, this)); > nsCOMPtr<nsIXPConnectJSObjectHolder> navigatorHolder; > > PRUint32 flags = 0; > >- if (reUseInnerWindow) { >+ if (reUseInnerWindow && !currentInner->IsFrozen()) { > // We're reusing the current inner window. > newInnerWindow = currentInner; > } else { >- if (thisChrome) { >- newInnerWindow = new nsGlobalChromeWindow(this); >+ if (!aState) { Maybe reverse these blocks so that we check |if (aState)| ? I think it's easier to read, but personal preference. >--- dom/src/base/nsGlobalWindow.h 31 Jul 2005 19:43:27 -0000 1.249 >+++ dom/src/base/nsGlobalWindow.h 12 Aug 2005 00:28:30 -0000 >@@ -352,35 +355,58 @@ protected: >+ PRBool IsFrozen() >+ { >+ NS_ASSERTION(IsInnerWindow(), >+ "Why do you care if an outer window is frozen?"); >+ return mIsFrozen; >+ } This method can be |const|. >--- layout/base/nsCaret.cpp 11 Aug 2005 03:44:16 -0000 1.144 >+++ layout/base/nsCaret.cpp 12 Aug 2005 00:28:36 -0000 I assume these changes aren't part of this bug. Looks good otherwise, thanks for doing this.

This patch includes the fixes for bug 304284 and bug 304078 in it, properly updated to the new-er world (i.e., we reset the docshell on the location object if the window gets restored). It's merged to jst's checkin and contains a couple of extra checks.

Comment on attachment 192553 [details] [diff] [review] updated and merged - In WindowStateHolder::WindowStateHolder(): + // Keep the listener manager around, but not on the window. + aWindow->GetListenerManager(getter_AddRefs(mListenerManager)); + + // Clear the window's EventListenerManager pointer so that it can't have + // listeners removed from it later. + aWindow->mListenerManager = nsnull; We don't need this code any more, right? - In nsGlobalWindow::SaveWindowState(nsISupports **aState): + if (inner->mLocation) + inner->mLocation->SetDocShell(nsnull); Add a comment about why we do this... - In nsGlobalWindow::RestoreWindowState(nsISupports *aState): + inner->mListenerManager = holder->GetListenerManager(); No longer needed... sr=jst

Comment on attachment 192553 [details] [diff] [review] updated and merged I'm just going to carry over the previous r+ from bryner here.... I've checked this into trunk.

I backed the fix for this out because it spiked the Tp numbers on btek and monkey. I suspect that we're pseudo-leaking windows. I'll investigate and check this back in once I have a potential fix.

FWIW, blake and I looked into the perf regression and didn't find anything all that interesting except for potential leak problems: in particular, the stuff that's not done in the IsFrozen cases in SetNewDocument isn't done anywhere else later on.

This is very close to the other patch, except that we actually call JS_ClearScope and clean up the old inner windows (from the destructor of the WindowStateHolder). The silly thinko I had in the first patch was that mContext is an outer window member, so checking that for null was always succeeding, even when the window had not been cleaned up. A better check would have been for mInnerWindow->mDocument. This patch avoids the assertion altogether, however.

Oh, one thing I did find is that the DEBUG_PAGE_CACHE code no longer compiles with this change.

Comment on attachment 192640 [details] [diff] [review] this time, without the leaks= sr=jst

*** Bug 304678 has been marked as a duplicate of this bug. ***

Checked in again, this time Tp numbers stayed where the were. I'm marking this bug as FIXED. I noted that body event handlers (<body onclick>, for example) are not restored, even with my patch. I'm looking into that now (and will file a new bug for it).

Comment on attachment 192640 [details] [diff] [review] this time, without the leaks= We probably want this fix (and any followups) on the branch.

Verifying fixed with latest CREATURE build.

(In reply to comment #39) > (From update of attachment 192640 [details] [diff] [review] [edit]) > We probably want this fix (and any followups) on the branch. > Is there a "fixed-trunk" or "fixed-branch" field? I hope it isn't still the "aviary" one, as that could get confusing.

(In reply to comment #41) > Is there a "fixed-trunk" or "fixed-branch" field? I hope it isn't still the > "aviary" one, as that could get confusing. Can you stop asking your random Bugzilla questions in Bugzilla please? Dozens of people don't need to get bugmail every time you don't understand something.

This wasn't checked in to the branch yet. Reopening pending checkin.

Fixed means fixed on trunk. We have a fixed1.8 keyword to indicate fixed on branch. Please do NOT reopen bugs based on what's going on with branch, ok?

(In reply to comment #44) > Fixed means fixed on trunk. We have a fixed1.8 keyword to indicate fixed on > branch. Please do NOT reopen bugs based on what's going on with branch, ok? Thanks for the answer Boris, between this flag and "[needs branch checkin]", it should cover it. Sorry Cusser.

Checked into MOZILLA_1_8_BRANCH.

Could this be the cause for bug 304970? The regression window fits, and this patch deals with changing window code. Can someone with a tree back this out and see?

verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07

Test case written with docshell sub-harness: http://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/docshell_helpers.js

Comment on attachment 387025 [details] [diff] [review] mochitest test case >diff --git a/docshell/test/chrome/Makefile.in b/docshell/test/chrome/Makefile.in > $(NULL) >- >+ Uber-nit: nuke the trailing whitespace here. >diff --git a/docshell/test/chrome/bug303267_window.xul b/docshell/test/chrome/bug303267_window.xul >+ // Now compare the current HTML of this page against an earlier copy. >+ // The copies should be different if the onpageshow script was executed >+ // with a copy of the original globals intact from the initial page So, this testcase actually does test this bug, but this comment is slightly misleading. The original cause of this bug was that access to global variables (like pageshowcount) would throw after navigating and going back. So, I think the comment should say something like "If showpageshowcount succeeded in changing the div's innerHTML, then we passed. Otherwise it threw and the following test will fail.". >+ // load. >+ var newHTML = getInnerHTMLById("div1"); >+ isnot(originalHTML, >+ newHTML, "HTML not updated on pageshow; javascript broken?");

mrbkap's comments were applied; this test now needs checked in.

Comment on attachment 387212 [details] [diff] [review] mochitest test case >diff --git a/docshell/test/chrome/Makefile.in b/docshell/test/chrome/Makefile.in > bug215405_window.xul \ >+ test_bug303267.xul \ >+ bug303267_window.xul \ >+ bug303267.html \ > test_bug364461.xul \ Nit to fix.

whitespace in Makefile.in corrected

Pushed http://hg.mozilla.org/mozilla-central/rev/0e6bc8f64614

Test can't land on 1.9.1 without bug 501235: { Error: getHttpUrl is not defined Source File: chrome://mochikit/content/chrome/docshell/test/chrome/bug303267_window.xul Line: 36 }

Comment on attachment 387671 [details] [diff] [review] mochitest test case [Checkin: Comment 54 & 56] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0274a35f0e16