303267 - Back/Forward / bfcache breaks JS javascript.

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jerry Baker]

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.

Comment 1

[Brendan Eich [:brendan]]

File against a likelier component.

/be

Comment 2

[Bob Clary [:bc] (inactive)]

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

Comment 3

[Caleb]

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

Comment 4

[Bob Clary [:bc] (inactive)]

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

Comment 5

[José Jeria]

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

Comment 6

[Gabriel Seah]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Is this a problem with bfcache disabled?

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 9

[Asa Dotzler [:asa]]

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

Comment 10

[Ben Basson]

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?

Comment 11

[Caleb]

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.

Comment 12

[Philipp Hanes]

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?

Comment 13

[Serge Gautherie (:sgautherie)]

[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.

Comment 14

[Samuel Sidler (old account; do not CC)]

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

Comment 15

[Aaron Slunt]

Attachment: testcase for comment #10

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.

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 17

[Jerry Baker]

What does this bug have to do with RSS?

Comment 18

[Frank Wein [:mcsmurf]]

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

Comment 19

[Jason Bassford]

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.

Comment 20

[Brian Ryner (not reading)]

Attachment: reduced testcase for original problem

- 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

Comment 21

[Johnny Stenback (:jst)]

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.

Comment 22

[Jaime Mitchell (use [e-mail] for email)]

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

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 24

[Dave Townsend [:mossop]]

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

Comment 25

[Brian Ryner (not reading)]

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

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: hold onto inner windows

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 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 192450 [details] [diff] [review]
hold onto inner windows

Better patch coming.

Comment 28

[Brian Ryner (not reading)]

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.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated and merged

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 30

[Johnny Stenback (:jst)]

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 31

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: this time, without the leaks=

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.

Comment 35

[Brian Ryner (not reading)]

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

Comment 36

[Johnny Stenback (:jst)]

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

sr=jst

Comment 37

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

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 39

[Blake Kaplan (:mrbkap) (inactive)]

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

We probably want this fix (and any followups) on the branch.

Comment 40

[Jason Bassford]

Verifying fixed with latest CREATURE build.

Comment 41

[Worcester12345]

(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. 

Comment 42

[Ben Basson]

(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.

Comment 43

[Ben Basson]

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

Comment 44

[Boris Zbarsky [:bzbarsky]]

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?

Comment 45

[Worcester12345]

(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.

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.

Comment 47

[Jerry Baker]

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?

Comment 48

[Tracy Walker [:tracy]]

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

Comment 49

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

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

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

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?");

Comment 51

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

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

Comment 52

[Serge Gautherie (:sgautherie)]

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.

Comment 53

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case [Checkin: Comment 54 & 56]

whitespace in Makefile.in corrected

Comment 54

[Boris Zbarsky [:bzbarsky]]

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

Comment 55

[Serge Gautherie (:sgautherie)]

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 56

[Serge Gautherie (:sgautherie)]

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