Back/Forward / bfcache breaks JS javascript.

VERIFIED FIXED in mozilla1.8beta4

Status

()

defect
--
major
VERIFIED FIXED
14 years ago
10 years ago

People

(Reporter: mozilla, Assigned: mrbkap)

Tracking

({regression, verified1.8})

Trunk
mozilla1.8beta4
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1.3: the test])

Attachments

(4 attachments, 4 obsolete attachments)

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
Assignee: general → general
Component: JavaScript Engine → DOM: Level 0
regression range is 2005-07-30 to 2005-07-31-10, sounds like split window bug
296639 fall out
Depends on: splitwindows
QA Contact: general → ian
*** 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
Keywords: regression
*** Bug 303275 has been marked as a duplicate of this bug. ***
Blocks: splitwindows
No longer depends on: splitwindows
Blocks: 303255
Depends on: 303151
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.
No longer depends on: 303151
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.
Flags: blocking1.8b4? → blocking1.8b4+
split window and bfcache interaction? bryner or jst, can you guys look into this?
Assignee: general → bryner
Target Milestone: --- → mozilla1.8beta4
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.
Component: DOM: Level 0 → RSS Discovery and Preview
Product: Core → Firefox
Target Milestone: mozilla1.8beta4 → ---
What does this bug have to do with RSS?
Component: RSS Discovery and Preview → DOM: Level 0
Product: Firefox → Core
Target Milestone: --- → mozilla1.8beta4
*** 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.
Summary: Back/Forward Break JS → Back/Forward / bfcache breaks JS javascript.
- 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. ***
Assignee: bryner → mrbkap
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.
Status: NEW → ASSIGNED
Whiteboard: [partial patch, ETA: 8/10]
*** Bug 304121 has been marked as a duplicate of this bug. ***
*** Bug 303059 has been marked as a duplicate of this bug. ***
Posted patch hold onto inner windows (obsolete) — Splinter Review
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?
Attachment #192450 - Flags: superreview?(jst)
Attachment #192450 - Flags: review?(bryner)
Comment on attachment 192450 [details] [diff] [review]
hold onto inner windows

Better patch coming.
Attachment #192450 - Attachment is obsolete: true
Attachment #192450 - Flags: superreview?(jst)
Attachment #192450 - Flags: review?(bryner)
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.
Attachment #192450 - Attachment is obsolete: false
Attachment #192450 - Flags: review+
Posted patch updated and merged (obsolete) — Splinter Review
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.
Attachment #192450 - Attachment is obsolete: true
Attachment #192553 - Flags: superreview?(jst)
Attachment #192553 - Flags: review?(bryner)
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
Attachment #192553 - Flags: superreview?(jst) → superreview+
Attachment #192553 - Flags: approval1.8b4?
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.
Attachment #192553 - Flags: review?(bryner) → review+
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.
Attachment #192553 - Attachment is obsolete: true
Attachment #192640 - Flags: superreview?(jst)
Attachment #192640 - Flags: review?(bryner)
Attachment #192553 - Flags: approval1.8b4?
Attachment #192640 - Flags: review?(bryner) → review+
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
Attachment #192640 - Flags: superreview?(jst) → superreview+
*** 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).
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 192640 [details] [diff] [review]
this time, without the leaks=

We probably want this fix (and any followups) on the branch.
Attachment #192640 - Flags: approval1.8b4?
Verifying fixed with latest CREATURE build.
Status: RESOLVED → VERIFIED
Attachment #192640 - Flags: approval1.8b4? → approval1.8b4+
(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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Whiteboard: [partial patch, ETA: 8/10] → [needs branch 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?
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
(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.
Keywords: fixed1.8
Whiteboard: [needs branch checkin]
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
Keywords: fixed1.8verified1.8
Depends on: 351236
Posted patch mochitest test case (obsolete) — Splinter Review
Test case written with docshell sub-harness: http://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/docshell_helpers.js
Attachment #387025 - Flags: review?(mrbkap)
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?");
Attachment #387025 - Flags: review?(mrbkap) → review+
Posted patch mochitest test case (obsolete) — Splinter Review
mrbkap's comments were applied; this test now needs checked in.
Attachment #387025 - Attachment is obsolete: true
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.
Attachment #387212 - Attachment description: mochitest test case (checkin-needed) → mochitest test case
whitespace in Makefile.in corrected
Attachment #387212 - Attachment is obsolete: true
Whiteboard: [test needs 1.9.1.x landing]
Attachment #387671 - Attachment description: mochitest test case (checkin-needed) → mochitest test case [Checkin: Comment 54]
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
}
Depends on: 501235
Keywords: checkin-needed
Whiteboard: [test needs 1.9.1.x landing]
Keywords: checkin-needed
Whiteboard: [needs 1.9.1.x landing: the test, after bug 501235]
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
Attachment #387671 - Attachment description: mochitest test case [Checkin: Comment 54] → mochitest test case [Checkin: Comment 54 & 56]
Keywords: checkin-needed
Whiteboard: [needs 1.9.1.x landing: the test, after bug 501235] → [fixed1.9.1.3: the test]
You need to log in before you can comment on or make changes to this bug.