343282 - bfcache is slow

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060630 Minefield/3.0a1 ID:2006063012 [cairo]


after discussion on moz forum

we're not sure when this regressed, but back and forwards speed has dropped a lot in the past week or so.

I have safebrowsing disabled, so that shouldn't be the cause

repro:
open a page
open another page
move back and forwards

result:
it's slow, far from instant

Comment 1

[Dão Gottwald [:dao]]

Even though Peter stated *usefull* comments would be welcome, I want to note that for me back/forward navigation never felt as fast as in Opera or IE.
Maybe there is a regression, but maybe bfcache also needs some more love in general.

Comment 2

[Peter van der Woude [:Peter6]]

regressionwindow:
works in 20060627 1541pdt build
fails in 20060627 2248pdt build

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20060627+1500&maxdate=20060627+2248&cvsroot=%2Fcvsroot

reproduce:
Open a mozillazine forums
Open a post
repeatedly switch back and forwards between the 2

result:
1) it takes a while to respond when you press the <- or -> button
2. I notice that out of a large number of tries at leat 10-20 % of the time the page seems loading (blue load bar, or whatever it's called)

Comment 3

[Martijn Wargers (dead)]

When I change the code to directly return NS_OK in nsPasswordManager::OnStateChange, then the bug seems to disappear in my debug build.
So this seems like a regression from bug 221634.

Comment 4

[Adam Guthrie]

Definitely not Core: Session History, then.

Comment 5

[Martijn Wargers (dead)]

So this happens because the new code uses an unload event listener on the window, which actually works instead of the old code, which used an event listener on the document, which didn't work.
So it seems that with the old code, the password manager wasn't cleaned up when a different page was visited. Isn't that a memory leak?
The new code properly calls nsPasswordManager::Unload now during the unload of the window, so the new code is actually doing now what passwordmanager was always supposed to do.

But setting unload event handlers on the window causes bfcache to stop working, this is intentional, see:
http://developer.mozilla.org/en/docs/Using_Firefox_1.5_caching
(first list-item in introduction)

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

This fixes the issue by using the pagehide event instead of the unload event.
There is one problem with it, though. Going back into history to a cached document with password fields don't show the password, this is intentional:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#4652

So I guess a pageshow event handler needs to be added, that does the same as the domcontentloaded event handler, except that event handler needs to be removed when the domcontentloaded event has fired, or something like that.

Comment 7

[Michael Wu [:mwu]]

Attachment: Fix stuff

The good news is that this fixes things, eliminates a lot of code, and generally works. The bad news is that we now fill in the form on both DOMContentLoaded *and* pageshow. However, chances are low that it would be noticed since we don't fill things in if there's something already there.

Comment 8

[Michael Wu [:mwu]]

Attachment: Use pageshow/pagehide instead of unload

Make OnStateChange a little smaller.

Comment 9

[Neil Deakin]

Comment on attachment 228492 [details] [diff] [review]
Use pageshow/pagehide instead of unload

Looks OK, but you may want to get an additional review from someone familiar with the bfcache.

Comment 10

[Michael Wu [:mwu]]

Attachment: Use STATE_RESTORING+pagehide instead of unload

While looking at the patches for bfcache, I noticed there's a STATE_RESTORING bit for nsIWebProgressListener. I prefer using this over pageshow, and it also avoids filling in the page twice.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 228712 [details] [diff] [review]
Use STATE_RESTORING+pagehide instead of unload

Note that the bulk of the patch is just moving the password filling code into its own function.

Comment 12

[Michael Wu [:mwu]]

Attachment: Use STATE_RESTORING+pagehide instead of unload, v2

Eliminate an unnecessary html doc check.

Comment 13

[Neil Deakin]

Comment on attachment 229214 [details] [diff] [review]
Use STATE_RESTORING+pagehide instead of unload, v2


>-  if (!type.EqualsLiteral("DOMContentLoaded"))
>+  if (type.EqualsLiteral("blur") ||
>+      type.EqualsLiteral("DOMAutoComplete"))
>     return FillPassword(aEvent);

There's also a Blur function in this class. Did you intend to remove it and replace it with this?

Comment 14

[Michael Wu [:mwu]]

(In reply to comment #13)
> (From update of attachment 229214 [details] [diff] [review] [edit])
> 
> >-  if (!type.EqualsLiteral("DOMContentLoaded"))
> >+  if (type.EqualsLiteral("blur") ||
> >+      type.EqualsLiteral("DOMAutoComplete"))
> >     return FillPassword(aEvent);
> 
> There's also a Blur function in this class. Did you intend to remove it and
> replace it with this?
> 
Huh, I guess it's not necessary. I checked for both to keep the behavior of the function the same as before.

Comment 15

[Michael Wu [:mwu]]

Attachment: Use STATE_RESTORING+pagehide instead of unload, v3

Comment 16

[Robert Strong (they/them - no direct email)]

checked in to trunk

Comment 17

[Mike Connor [:mconnor]]

Knocking off the fx2 blocking list because the original bug hasn't landed on branch and isn't blocking, and the new patch that's on that bug has this fix rolled in.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

This may have caused Bug 347003, or at least made it worse.
Without this patch I don't see the reload loop described in Bug 347003.

Comment 19

[Michael Wu [:mwu]]

(In reply to comment #18)
> This may have caused Bug 347003, or at least made it worse.
> Without this patch I don't see the reload loop described in Bug 347003.
> 

No, it did not. Reverting this patch merely disables your bfcache. You can set browser.sessionhistory.max_total_viewers to 0 for the same effect.