396649 - Content viewers not evicted when going back

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Andrew Schultz]

Attachment: patch

When going back one page at a time, session history does not evict content viewers.  In the case where you're at index 1 and go back to 0 (and 2, 3 and 4 have content viewers saved), aFromIndex=1 and aToIndex=0.

startIndex = aToIndex + gHistoryMaxViewers + 1
endIndex = PR_MIN(mLength, aFromIndex + gHistoryMaxViewers);

So, startIndex=0+3+1=4 and endIndex is 1+3=4.  So the eviction loop does nothing.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So what is this code actually doing?  Why is it adding to both indices?  Why is there even more than one index involved; I'd think we'd just have the target index....

Comment 2

[Andrew Schultz]

Yes, this code is relatively confusing (and not entirely wholesome).  I was tempted to drop it and replace it with simpler code for each case (add, goto, goback, goforward) for 396519, but I still needed something like this for goto, so I used it in each case.

It adds to both indices because we're going back.  Entries with content viewers will always be within 3 of the old index (not the target).  So if you go from j to j-1, the content viewer's you'll evict are going to be at higher indices, specifically at j+3.  When going from j to j-10, the code would need to look at indices from j-3 to j+3.  Well, the code does j-7 to j+3, so it could do:

startIndex = PR_MAX(startIndex, aFromIndex - gHistoryMaxViewers);

... but that would be a different bug.

Anyway, the code is hackish but works except for this bug and I couldn't think of a cleaner way to handle the gotoIndex case.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281418 [details] [diff] [review]
patch

> Entries with content viewers will always be within 3 of the old index (not the
> target).

This is worth documenting (with s|3|gHistoryMaxViewers | or whatever).

And perhaps we should add asserts to that effect.  The point is that we're using this invariant to not have to walk the entire session history in a given direction, right?  That could use documenting too.

With some comments added, r+sr+a=bzbarsky

Comment 4

[Andrew Schultz]

> And perhaps we should add asserts to that effect.

Hmm.  What exactly would we assert?  We don't look at entries outside that range and entries within that range might not have content viewers.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think nsSHistory::EvictWindowContentViewers or the callee should maybe walk the whole history (in debug builds only) and assert that only things within the range have content viewers.

Comment 6

[Andrew Schultz]

OK, patch landed, so this is fixed.  I have a patch for adding debug asserts that I'll attach tomorrow.

Comment 7

[Andrew Schultz]

Attachment: assert that content viewers aren't where they shouldn't be

I actually hit the assertion the first time I tried this, but I was unable to reproduce.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283119 [details] [diff] [review]
assert that content viewers aren't where they shouldn't be

Looks fine.  Could use an assert that the |startIndex| to |endIndex| range fully covers the range outside which you're asserting there are no content viewers.

Comment 9

[Andrew Schultz]

> Looks fine.  Could use an assert that the |startIndex| to |endIndex| range
> fully covers the range outside which you're asserting there are no content
> viewers.

This isn't actually true.  |startIndex| to |endIndex| covers a range which might have content viewers which (if they do exist) should be evicted.  We could assert that we wouldn't evict contentViewers we shouldn't:
  (startIndex > aToIndex + gHistoryMaxViewers ||
   endIndex < aToIndex - gHistoryMaxViewers)

If we add the line of code I mentioned in comment 2, we could also assert that the range where contentViewers might be fully contains the |startIndex| to |endIndex| range -- that we don't look for contentViewers in places where no contentViewers should be.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> This isn't actually true.  

Indeed.  I don't know what I was thinking.  The patch is good to go as is.

Your call on the comment 2 part.

Comment 11

[Andrew Schultz]

Comment on attachment 283119 [details] [diff] [review]
assert that content viewers aren't where they shouldn't be

assertions, debug-only code

Comment 12

[Andrew Schultz]

Comment on attachment 283119 [details] [diff] [review]
assert that content viewers aren't where they shouldn't be

(landed this too)

Comment 13

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

Comment 14

[Boris Zbarsky [:bzbarsky]]

> +      // Load test pages 0 through 5, all of which should be stored
> +      // in the bfcache at the time they're loaded.

They're stored into bfcache when they're unloaded, and you should assert that by listening for their pagehide events.

Replace (i > 2 ? true : false) with "i > 2".  That's already a boolean, no?

Same for i > 4.

This test is assuming our limit of "3" for bfcache.  That's actually pref-dependant and subject to change; please put it into a const in the test so that if we change it we can update the test easily.   That means that the 2 and 4 should be written in terms of that constant, as should the 5 and 6.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Jonathan, see comment 14.

Comment 16

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

Thanks for the review Boris; the new version addresses all your comments.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed test as http://hg.mozilla.org/mozilla-central/rev/be3584f973fd

Comment 18

[Henri Sivonen (:hsivonen)]

Do we really care about the "persisted" property on the pagehide event for about:blank? Does it have known Web compat impact?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Web compat, perhaps not.  Extension compat, likely yes.

Comment 20

[Henri Sivonen (:hsivonen)]

(In reply to comment #19)
> Web compat, perhaps not.  Extension compat, likely yes.

The persisted property is about back/forward cache and nothing else, right? Isn't it bad use of RAM to cache the initial about:blank? For extension compat, should this be assumed to be so important that I can't change the initial about:blank to report persisted==false?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> The persisted property is about back/forward cache and nothing else, right?

Yes.

> Isn't it bad use of RAM to cache the initial about:blank?

Either it can be gone back to (in which case, why is it bad to cache it?) or it can't be gone back to (in which case, why are we caching it at all)?

That said, if you're just proposing never bfcaching initial about:blank, that's probably fine.  Why is that needed, though?

Comment 22

[Henri Sivonen (:hsivonen)]

(In reply to comment #21)
> That said, if you're just proposing never bfcaching initial about:blank,
> that's probably fine.  Why is that needed, though?

It just naturally happens that way when getting rid of a parser-generated quasi-initial about:blank and having only a truly-initial docshell-created about:blank. The alternative is that I spend time figuring out how to make something different happen. I'm unsure if I should spent that time or just tweak the mochitest expectations.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I think tweaking the mochitest is fine in that case.