646641 - If a document has multiple history entries (either by pushstate or anchor navs) only the last one is hooked into bfcache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Justin Lebar (not reading bugmail)]

Attachment: Testcase. Follow directions in comment 0.

This is per Jonas's description on IRC (I've verified).

STR:

* In a new tab, load attached testcase.
* Click "push some state".  Note that the page now says "Got load. State=null" and "Pushed state=(random number)."
* Navigate to http://mozilla.org by typing into the location bar.
* Click and hold the back button, and select the first history entry for this tab; this entry should correspond to the initial load of the testcase.

Expected results:

* Page still says "Got load. State=null" and "Pushed state=(random number)", because we retrieved it from bfcache.

Actual results:

* Page says "Got load. State=null", presumably because we reloaded it from the network.

It looks like skipping past the most recent history entry causes us to do a full network load, rather than pull the document out of bfcache.

Comment 1

[Justin Lebar (not reading bugmail)]

Actually, I think this happens with anchor loads too.

STR:

* Type something into this bugzilla comment box.
* Click one of the anchor links (e.g. "comment 1")
* Navigate to mozilla.org using the location bar
* Go back two history entries by clicking and holding the back button

Expected results:

* Comment box still has what you typed.

Actual results:

* Comment box is empty, because we reloaded the Bugzilla page.

Comment 2

[Justin Lebar (not reading bugmail)]

Does bfcache have any mechanism to deal with two shentries sharing a content viewer (as should happen with anchor loads, as well as pushstate)?

When I try to shove content viewers into shentries after an anchor load or a pushstate, I get many assertions.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we need to create a separate object which is where we'd stick the cached contentviewer. All nsISHEntries that refer to the same Document would then keep a pointer to this same object.

So we'd stick the contentviewer in a shared state rather in *any* individual nsISHEntry

Comment 4

[Justin Lebar (not reading bugmail)]

What would be the purpose of this level of indirection, as compared to putting the contentviewer directly in each of the shentries?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It just makes it easier (and thus hopefully less error prone) to remove or add the contentviewer from all the entries.

Either would work I guess.

One advantage of the former is that it could maybe replace the id thing we are doing to figure out which SHEntries share Document.

In any case, it's totally up to you. At this point I feel like you know this code better than me.

Comment 6

[Justin Lebar (not reading bugmail)]

Olli, do you have any thoughts about this?  I think Jonas may be overestimating how well I understand this code.  :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment 3 seems like a good way to go, yes.  And the diagnosis here (as based on the summary) is absolutely correct.

I would be surprised if we don't have an existing version of this bug for anchor nav, by the way.

Comment 8

[Justin Lebar (not reading bugmail)]

My current plan is:

 * Create a content viewer container object.  It contains one content viewer and potentially many SHEntries.

 * Modify SHistory so it owns a list of content viewer containers.  This way, we can have a correct, global view of the live content viewers.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Why not just have the shentries own the viewer container right now?  The proposed plan doesn't sound like it would necessarily work well if entries are moved between histories or whatnot....

Comment 10

[Justin Lebar (not reading bugmail)]

I was trying to address the fact that SHistory is not aware that one SHEntry doesn't correspond to one content viewer.  If a page calls pushState many times, SHistory will (I think!) go ahead and evict all the content viewers, on the (incorrect) assumption that all the SHEntries created via pushState contain a unique content viewer.

I agree that moving entries between histories would seriously mess with the plan from comment 8, though.  Maybe it's OK to evict more aggressively than we need to; certainly the user would have to click "back" many times to arrive back at the page which would be in bfcache...

Comment 11

[Justin Lebar (not reading bugmail)]

Looking at this code, I'm kind of surprised session history works with pushState or hash navigations at all.  Most of the data in an nsSHEntry actually belongs to all the SHEntries for a given document.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Most of it is cloned when the entry is cloned, no?

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to comment #12)
> Most of it is cloned when the entry is cloned, no?

Yes, but much of it is mutable, and I think some subset of that actually changes.  Apparently it doesn't change enough, or most of it is relevant only to bfcache, though, since it does appear to work.  Mostly.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Wip v1

This passes the test I wrote for this bug, but still needs some cleanup and likely some bugfixes.  I also need to fix sessionrestore.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1 - Let SHEntries share content viewers

Pushing this to try.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 530393 [details] [diff] [review]
Part 1, v1 - Let SHEntries share content viewers

Looks good on try.

I'm not totally pleased with the interface here.  My goal was to avoid COM-ifying nsSHEntryShared, but I achieved that at a cost of some pretty obtuse functions.  Perhaps there's a better way to do this.  Smaug, what do you think?

Comment 17

[Justin Lebar (not reading bugmail)]

As usual, the session restore code needs a review from zpao, but that'll change if we change the interface, so I'll wait to ask for his review.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 530393 [details] [diff] [review]
Part 1, v1 - Let SHEntries share content viewers

Browsing around with this build, I noticed that I'm getting the following warning when pages use pushState:

WARNING: ContentViewer exists outside gHistoryMaxViewer range: '!viewer', file ../../../../src/docshell/shistory/src/nsSHistory.cpp, line 890

Cancelling the review until I figure this out and fix it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

The eviction code in history makes assumptions about how far away from the current shentry it can find viewers.  Those assumptions probably need updating.

Comment 20

[Justin Lebar (not reading bugmail)]

Yes.  I *think* the only assumption that's invalid is that a content viewer won't exist outside such and thus a range.  The new invariant is that any content viewer which exists out of range must be equal to the content viewer of the closest SHEntry in range.  That is, you can have

 A A A B C D D D
     *     +

where * and + mark the beginning and end of the expected range, but you can't have

 A A B C D E
     *   +

Comment 21

[Justin Lebar (not reading bugmail)]

This is a bit tricky, because I realized that it's totally legal to have the following history:

 A A B A

(again, letters are content viewers, or documents, if you prefer).  Just call pushState three times, go back once, and refresh the page.

So I think we're going to need to build a set of the in-range content viewers and before evicting make sure that the targeted content viewer isn't in that set.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. that seems very unfortunate and I bet will come back to bite us (I even have a feature in mind which would be basically impossible with that setup).

Could we make it so that if you have (with the ^ showing the current history position)

A A A A
    ^

and press reload, you'd end up with 

A A B B
    ^

?

Comment 23

[Justin Lebar (not reading bugmail)]

Well, the letters correspond to content viewers, which have a one-to-one correspondence to documents.  So I think what you're suggesting amounts to taking the fourth SHEntry and moving it from document A to document B.

I'm not convinced we can do that, because the way in which I interpret a SHEntry's state object might be affected by the document's state.

This is different from a session restore; here we're taking a SHEntry from an active document and transplanting it into another active document.

That said, I also don't like the A B A situation...

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Update SHistory

Comment 25

[Justin Lebar (not reading bugmail)]

Pushed to try.  We'll see.

http://tbpl.mozilla.org/?tree=Try&rev=79fa6b2edcf5

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2 - Update SHistory

Comment 27

[Justin Lebar (not reading bugmail)]

Comment on attachment 530393 [details] [diff] [review]
Part 1, v1 - Let SHEntries share content viewers

This looks good on try.

Comment 28

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

Would you like to get this reviewed before next Aurora branching?

Comment 29

[Justin Lebar (not reading bugmail)]

I wouldn't be sad if this didn't make it.  In fact, given that it risks hard-to-detect regressions, it's probably better to wait until after Tuesday.

Comment 30

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1.1 (rebased to tip)

Comment 31

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1.1 (rebased to tip)

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 549472 [details] [diff] [review]
Part 1, v1.1 (rebased to tip)

Smaug said he'd try to look at these soon, so I went ahead and rebased them.

Comment 33

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

Comment on attachment 549473 [details] [diff] [review]
Part 2, v1.1 (rebased to tip)


>+already_AddRefed<nsIContentViewer>
>+GetContentViewerForTransaction(nsISHTransaction *aTrans)
>+{
>+  nsCOMPtr<nsISHEntry> entry;
>+  aTrans->GetSHEntry(getter_AddRefs(entry));
>+  if (!entry)
>+    return nsnull;
if (expr) {
  stmt;
}

> nsSHistory::EvictWindowContentViewers(PRInt32 aFromIndex, PRInt32 aToIndex)
> {
>-  // To enforce the per SHistory object limit on cached content viewers, we
>-  // need to release all of the content viewers that are no longer in the
>-  // "window" that now ends/begins at aToIndex.  Existing content viewers
>-  // should be in the window from
>-  // aFromIndex - gHistoryMaxViewers to aFromIndex + gHistoryMaxViewers
>+  // We need to release all content viewers that are no longer in the range
>   //
>-  // We make the assumption that entries outside this range have no viewers so
>-  // that we don't have to walk the whole entire session history checking for
>-  // content viewers.
>-
>-  // This can happen on the first load of a page in a particular window
>+  //  aToIndex - gHistoryMaxViewers to aToIndex + gHistoryMaxViewers
>+  //
>+  // to ensure that this SHistory object isn't responsible for more than
>+  // gHistoryMaxViewers content viewers.
Something strange with the comment.
It looks like 'to ensure ' starts a new statement but the sentence doesn't quite make
sense (and doesn't start with capital letter)




>+  // Calculate the range that's safe from eviction.
>+  PRInt32 startSafeIndex = PR_MAX(0, aToIndex - gHistoryMaxViewers);
>+  PRInt32 endSafeIndex = PR_MIN(mLength, aToIndex + gHistoryMaxViewers);
>+
>+  LOG(("EvictWindowContentViewers(fromIndex=%d, toIndex=%d), mLength=%d. "
>+       "Safe range [%d, %d]",
>+       aFromIndex, aToIndex, mLength, startSafeIndex, endSafeIndex)); 
>+
>+  // The content viewers in range aToIndex -/+ gHistoryMaxViewers will not be
>+  // evicted.  Collect a set of them so we don't accidentally evict one of them
>+  // if it appears outside this range.
I don't understand there indexes. Why are they counted from aToIndex ?
And why +/- gHistoryMaxViewers


>+namespace {
>+
>+class SHTransAndDist
Strange class name. What does Dist mean?
Distance? 


>+    nsCOMPtr<nsISHEntryInternal> shentryInternal = do_QueryInterface(shentry);
>+    if (shentryInternal) {
>+      shentryInternal->GetLastTouched(&mLastTouched);
>+    }
>+    else {
if (expr) {
  stmt;
} else {
  stmt;
}


>+  // We now have collected all cached content viewers.  First check that we
>+  // have enough that we actually need to evict some.
>+  if ((PRInt32)transactions.Length() <= sHistoryMaxTotalViewers)
>+    return;

if (expr) {
  stmt;
}

>+
>+  // If we need to evict, sort our list of transactions and evict the largest
>+  // ones.  (We could of course get better algorithmic complexity here by using
>+  // a heap or something more clever.  But sHistoryMaxTotalViewers isn't large,
>+  // so let's not worry about it.)
>+  transactions.Sort();
>+
>+  for (PRInt32 i = transactions.Length() - 1;
>+       i >= sHistoryMaxTotalViewers; i--) {

please use --i

Comment 34

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

Comment on attachment 549472 [details] [diff] [review]
Part 1, v1.1 (rebased to tip)

In new code, could you please use coding style
if (expr) {
  stmt;
}
>+nsSHEntryShared::RemoveFromBFCacheAsync()
>+{
>+  NS_ASSERTION(mContentViewer && mDocument,
>+               "we're not in the bfcache!");
>+
>+  // Release the reference to the contentviewer asynchronously so that the
>+  // document doesn't get nuked mid-mutation.
>+
>+  nsCOMPtr<nsIRunnable> evt =
>+      new DestroyViewerEvent(mContentViewer, mDocument);
2 extra spaces?


>+  nsresult rv = NS_DispatchToCurrentThread(evt);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("failed to dispatch DestroyViewerEvent");
>+  }
>+  else {
if (expr) {
 stmt;
} else {
  stmt;
}


>+    // Drop presentation. Also ensures that we don't post more then one
>+    // PLEvent. Only do this if we succeeded in posting the event since
>+    // otherwise the document could be torn down mid mutation causing crashes.
Please update the code. We don't have PLEvents anymore.


>+void
>+nsSHEntryShared::CharacterDataWillChange(nsIDocument* aDocument,
>+                                   nsIContent* aContent,
>+                                   CharacterDataChangeInfo* aInfo)
Align parameters here and elsewhere.

Looks good.

Comment 35

[Justin Lebar (not reading bugmail)]

> I don't understand there indexes. Why are they counted from aToIndex ?
> And why +/- gHistoryMaxViewers

This is a critical part, so if it's not clear to you, I need to fix the comment.

Before the patch, the invariant was that there could be no more than gHistoryMaxViewers at a time.  Since all the viewers are consecutive and the range of viewers must touch the current index (i.e., the sequence [viewers' indices] union [current index] must not skip any indices), this implies that before a navigation, all viewers are in the range aFromIndex +/- gHistoryMaxViewers.

But therefore we must make sure that after a navigation, all viewers are in the range aToIndex +/- gHistoryMaxViewers.

I (attempt) to maintain the same invariant.  Unless I did something wrong?

(To show the converse, that restricting content viewers to this range implies that we never have more than gHistoryMaxViewers: You can only get a content viewer at index X by first navigating to X.  So suppose we have viewers for indices X through gHistoryMaxViewers + X.  If we navigate to index X - 1 or gHistoryMaxViewers + X + 1, we'll evict one of our old viewers.  That's enough to show that the invariant holds.)

> please use --i

I've been getting this comment from some reviewers lately, and it's confused me.  Is there an effort to make this the standard style across the codebase?  If so, is there a reason?

Comment 36

[Justin Lebar (not reading bugmail)]

> Something strange with the comment. It looks like 'to ensure ' starts a new 
> statement but the sentence doesn't quite make sense (and doesn't start with 
> capital letter)

This wasn't a typo; I wanted to set off the expression aToIndex - gHistoryMaxViewers to aToIndex + gHistoryMaxViewers from the rest of the text.

Comment 37

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2

Comment 38

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

Comment on attachment 549939 [details] [diff] [review]
Part 2, v2


>+  bool operator<(const TransactionAndDistance &aOther) const {
{ should be in the next line

>+  bool operator==(const TransactionAndDistance &aOther) const {
ditto

>       gTestCount++;
>       gExpected = [false, false, true, true, true, false];
>+      dump("About to run test 9.\n");
You probably don't want to keep dump()s

Comment 39

[Justin Lebar (not reading bugmail)]

My last try run was before the rebase.  New try push: http://tbpl.mozilla.org/?tree=Try&rev=beb278605187

Comment 40

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/883e581e0849
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfea4859f458

Comment 41

[Justin Lebar (not reading bugmail)]

Argh, there was an moth opt permaorange in the try push that I didn't notice.  Backed out from m-i.

Comment 42

[Justin Lebar (not reading bugmail)]

Ah, the moth orange is a real bug.

The problem is that in session restore, I use a BFCacheEntry (nsIShentryShared) as a key into a map.  As far as I can tell, js calls ToString() to generate the hash key?  In a debug build, this works great, because ToString() contains the pointer.  But in an opt build, the pointer isn't there, and we fall flat on our face.

I spoke with some people on IRC, and the consensus is that I should just make BFCacheEntry's ToString return the pointer value.  WeakMap might be a better solution, but it's buggy.

Comment 43

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2 (fixes moth failure)

Comment 44

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff Part 1 v1 --> v2

Comment 45

[Justin Lebar (not reading bugmail)]

New try push (with part 1 v2, part 2 v2): http://tbpl.mozilla.org/?tree=Try&rev=572348ee4ac3

Comment 46

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2.1

Comment 47

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2.1

Comment 48

[Justin Lebar (not reading bugmail)]

Part 1, v2.1, part 2 v2 looks good on Try.  The change is to add a toString() method on BFCacheEntry which returns the this pointer value.  I also added license blocks which I'd forgotten.  I'll attach an interdiff in a moment.

Smaug, does this look OK to you?  I'd like Waldo to have a look too, since we were discussing the toString hack on IRC.

http://tbpl.mozilla.org/?tree=Try&rev=454a074d2944

Comment 49

[Justin Lebar (not reading bugmail)]

Comment on attachment 550671 [details] [diff] [review]
Part 2, v2.1

This patch is the same as part 2 v2; accidentally uploaded it.

Comment 50

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff, part 1 v1 --> v2.1

Comment 51

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

Could BFCacheEntry just have PRUInt64 id and use the id as hashkey?

Comment 52

[Justin Lebar (not reading bugmail)]

The problem is that AIUI you can't represent a PRUint64 precisely in Javascript as a number.  It could certainly have a PRUint64 ID and return that as a string, but that doesn't seem much better than returning the pointer...

Comment 53

[Boris Zbarsky [:bzbarsky]]

> The problem is that AIUI you can't represent a PRUint64 precisely in Javascript
> as a number.

While true, that's only an issue once you hit more than 2^53 session history entries (over the process lifetime), right?  Or about 3.5 months of creating a session history entry every nanosecond (which I don't think we can actually manage to do).  No opinion on whether the integer id is better than the string approach, though.

Comment 54

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

I'd prefer integer id. It makes it clear what kind of value the id is.

Comment 55

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

Comment on attachment 550672 [details] [diff] [review]
Part 1, v2.1

So, I'd like to see .id. It is less hacky, IMO.

Comment 56

[Justin Lebar (not reading bugmail)]

Will do!

Comment 57

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v3

Comment 58

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff part 1 v2.1 --> v3

Comment 59

[Justin Lebar (not reading bugmail)]

Looked good on try.  Fingers crossed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/dc162f9bce24
http://hg.mozilla.org/integration/mozilla-inbound/rev/f9247cadf32b

Comment 60

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/dc162f9bce24
http://hg.mozilla.org/mozilla-central/rev/f9247cadf32b

Comment 61

[Mark Finkle (:mfinkle) (use needinfo?)]

These patches ignored Fennec and busted our SessionStore

Comment 62

[Justin Lebar (not reading bugmail)]

Backed out on suspicion of causing the crash in bug 683777.

We'll watch crash stats and see if this fixed the problem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f1de1d9cba

Comment 63

[Justin Lebar (not reading bugmail)]

Backed out the backout due to permaorange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9670f22f912d

Comment 64

[Justin Lebar (not reading bugmail)]

Backed out (temporarily) for real this time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a34e6bed361
https://hg.mozilla.org/mozilla-central/rev/8a34e6bed361

Comment 65

[Justin Lebar (not reading bugmail)]

Re-landed with an extra null check.

https://hg.mozilla.org/integration/mozilla-inbound/rev/051b3d8ed545
https://hg.mozilla.org/mozilla-central/rev/051b3d8ed545