Closed Bug 646641 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(4 files, 11 obsolete files)

549 bytes, text/html
Details
38.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
84.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.44 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 500328
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.
Summary: Pushstate doesn't load from bfcache when skipping past the most recent history entry for a document → If a document has multiple history entries (either by pushstate or anchor navs) only the last one is hooked into bfcache
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.
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
What would be the purpose of this level of indirection, as compared to putting the contentviewer directly in each of the shentries?
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.
Olli, do you have any thoughts about this?  I think Jonas may be overestimating how well I understand this code.  :)
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.
Assignee: nobody → justin.lebar+bug
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.
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....
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...
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.
Most of it is cloned when the entry is cloned, no?
(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.
Attached patch Wip v1 (obsolete) — Splinter Review
This passes the test I wrote for this bug, but still needs some cleanup and likely some bugfixes.  I also need to fix sessionrestore.
Pushing this to try.
Attachment #530128 - Attachment is obsolete: true
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?
Attachment #530393 - Flags: review?(Olli.Pettay)
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 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.
Attachment #530393 - Flags: review?(Olli.Pettay)
The eviction code in history makes assumptions about how far away from the current shentry it can find viewers.  Those assumptions probably need updating.
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
     *   +
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.
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
    ^

?
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...
Attachment #530393 - Attachment description: Patch v1 → Part 1, v1 - Let SHEntries share content viewers
Attachment #532312 - Attachment description: Part 2: Update SHistory so it understands that SHEntries may share content viewers. → Part 2, v1: Update SHistory
Attachment #532312 - Attachment is obsolete: true
Comment on attachment 530393 [details] [diff] [review]
Part 1, v1 - Let SHEntries share content viewers

This looks good on try.
Attachment #530393 - Flags: review?(Olli.Pettay)
Attachment #532717 - Flags: review?(Olli.Pettay)
Would you like to get this reviewed before next Aurora branching?
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.
Attachment #530393 - Attachment is obsolete: true
Attachment #530393 - Flags: review?(Olli.Pettay)
Attachment #532717 - Attachment is obsolete: true
Attachment #532717 - Flags: review?(Olli.Pettay)
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.
Attachment #549472 - Flags: review?(Olli.Pettay)
Attachment #549473 - Flags: review?(Olli.Pettay)
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
Attachment #549473 - Flags: review?(Olli.Pettay) → review-
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.
Attachment #549472 - Flags: review?(Olli.Pettay) → review+
> 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?
> 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.
Attached patch Part 2, v2Splinter Review
Attachment #549939 - Flags: review?(Olli.Pettay)
Attachment #549473 - Attachment is obsolete: true
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
Attachment #549939 - Flags: review?(Olli.Pettay) → review+
My last try run was before the rebase.  New try push: http://tbpl.mozilla.org/?tree=Try&rev=beb278605187
Argh, there was an moth opt permaorange in the try push that I didn't notice.  Backed out from m-i.
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.
Whiteboard: [inbound]
Attachment #549472 - Attachment is obsolete: true
Attached patch Interdiff Part 1 v1 --> v2 (obsolete) — Splinter Review
New try push (with part 1 v2, part 2 v2): http://tbpl.mozilla.org/?tree=Try&rev=572348ee4ac3
Attached patch Part 2, v2.1 (obsolete) — Splinter Review
Attachment #550671 - Flags: review?(Olli.Pettay)
Attached patch Part 1, v2.1 (obsolete) — Splinter Review
Attachment #550672 - Flags: review?(Olli.Pettay)
Attachment #550476 - Attachment is obsolete: true
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 on attachment 550671 [details] [diff] [review]
Part 2, v2.1

This patch is the same as part 2 v2; accidentally uploaded it.
Attachment #550671 - Flags: review?(Olli.Pettay)
Attachment #550671 - Attachment is obsolete: true
Attachment #550477 - Attachment is obsolete: true
Attachment #550674 - Flags: review?(jeff.walden)
Could BFCacheEntry just have PRUInt64 id and use the id as hashkey?
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...
> 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.
I'd prefer integer id. It makes it clear what kind of value the id is.
Comment on attachment 550672 [details] [diff] [review]
Part 1, v2.1

So, I'd like to see .id. It is less hacky, IMO.
Attachment #550672 - Flags: review?(Olli.Pettay) → review-
Will do!
Attached patch Part 1, v3Splinter Review
Attachment #551171 - Flags: review?(Olli.Pettay)
Attachment #550672 - Attachment is obsolete: true
Attachment #550674 - Attachment is obsolete: true
Attachment #550674 - Flags: review?(jeff.walden)
Attachment #551171 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [inbound/]/ → [inbound]
Target Milestone: --- → mozilla8
These patches ignored Fennec and busted our SessionStore
Whiteboard: [inbound]
Blocks: 677873
Depends on: 678274
Depends on: 683777
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out the backout due to permaorange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9670f22f912d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 687710
Backed out (temporarily) for real this time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a34e6bed361
https://hg.mozilla.org/mozilla-central/rev/8a34e6bed361
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Re-landed with an extra null check.

https://hg.mozilla.org/integration/mozilla-inbound/rev/051b3d8ed545
https://hg.mozilla.org/mozilla-central/rev/051b3d8ed545
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 696973
Depends on: 698656
You need to log in before you can comment on or make changes to this bug.