674302 - We lose shentries associated with navigated iframes

Status: RESOLVED INVALID

(Core :: DOM: Navigation, defect)

Description

[Justin Lebar (not reading bugmail)]

Attachment: Testcase

This is split off from bug 670318.  It might be the same bug; I'm not yet sure.

STR:

 * In a new tab, open http://mozilla.org.
 * Navigate to attached testcase.
 * Examine session history by right-clicking the back button.  You should see three SHEntries -- two for the testcase, followed by one for mozilla.org.
 * Click <back> twice.  You should now be at mozilla.org.
 * Examine session history by right-clicking the back button.

Expected results:

 See three SHEntries.

Actual results:

 See only two SHEntries.  The second one for the testcase is gone.

Comment 1

[Justin Lebar (not reading bugmail)]

This is a regression; the testcase works properly in 3.6.  I'll bisect.

Comment 2

[Justin Lebar (not reading bugmail)]

It appears that there are actually two separate changes in behavior.  Before and after this date, there are three SHEntries in the STR above.  But before the date, you can go back/forward through all of them; after the date, you can't go forward onto the last one after you've gone back onto the first one.

There must be a separate point at which the third SHEntry disappears from the right-click menu.  I'm going to bisect for it, although I suspect that this is the more important regression.

Last good nightly: 2010-08-17
First bad nightly: 2010-08-18

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=f4c97baa0e51

Comment 3

[Justin Lebar (not reading bugmail)]

Here's the second regression, where the SHEntry disappears altogether:

Last good nightly: 2010-09-05
First bad nightly: 2010-09-06

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd13b6ce36bd&tochange=67aef7ffb282

Comment 4

[Justin Lebar (not reading bugmail)]

In the pushlog for comment 2 is bug 462076 again.  In the pushlog for comment 3, I'd guess it's bug 534178.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Testcase v2

Updating the testcase so it works (that is, has the same errant behavior as v1) with the patch from bug 673467.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Hack v1

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 548894 [details] [diff] [review]
Hack v1

I suspect this doesn't fix the problem, but rather only hides it.  (This change makes the testcase work properly, and also makes G+ work properly.)  We also pass all the docshell/navigation tests with this change.

It's not clear to me exactly what the intent of this call is.  Is the problem that we need to make sure that we remove all the shentries in shistory for dynamically-added iframes which aren't part of the current document?  And we have to do this because otherwise, when we empty bfcache, those shentries for dynamically-added iframes are meaningless?

If so, then it seems like the behavior on the testcase in this bug is intentional -- when we navigate away from the testcase, we remove the shentries for the dynamically-added iframes.  When we navigate back, there's only one shentry left for the testcase, which corresponds to "iframe 1".  And the second shentry, for "iframe 2" is never created, because we retrieved the document from bfcache.

That behavior seems pretty wrong to me, fwiw.

It seems like there's a separate issue here, which is that on G+, running RemoveDynEntries not only removes the second entry, but also confuses shistory so back/forward/refresh stop working altogether.

Smaug, can you help me understand what's going on here?

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 548894 [details] [diff] [review]
Hack v1

This patch almost surely does the wrong thing.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Testcase produces expected result now, closing.