680344 - Session history navigation can break after removing an iframe from the document during a history navigation

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/f69a10f23bf3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110818 Firefox/9.0a1 ID:20110818030747

I encountered a problem during I was testing Bug 678872.

Back Button do not work even if history entry is available.
Document navigation do not work by select back/forward drop down.


Reproducible: Always

Steps to Reproduce:
1. Open Firefox with clean profile
2. Open Page (ex. "http://www.mozilla.com/en-US/about/")
3. Open testcase https://bugzilla.mozilla.org/attachment.cgi?id=553033
4. Click Back Button in Navbar

5. Right click Back Button in Navbar to pop up back/forward drop down list.
6. Select a history entry of the page of step2 (ex. "Firefox Web Browser — Learn About Mozilla")


Actual Results:
	Back Button do not work at Step 4
	Nothing happens at Step 6

Expected Results:
  Document navigatio should work properly



Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/daf167d2e21e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110608 Firefox/7.0a1 ID:20110608140145
Fails:
http://hg.mozilla.org/mozilla-central/rev/3a509617644e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110608 Firefox/7.0a1 ID:20110608144956
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=daf167d2e21e&tochange=3a509617644e


Regression window(cached ceder hourly):
Works:
http://hg.mozilla.org/projects/cedar/rev/d9c15147d35d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110607 Firefox/7.0a1 ID:20110607105413
Fails:
http://hg.mozilla.org/projects/cedar/rev/c3be577cd86a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110607 Firefox/7.0a1 ID:20110607132619
Pushlog:
http://hg.mozilla.org/projects/cedar/pushloghtml?fromchange=d9c15147d35d&tochange=c3be577cd86a


In local build from ceder repository,
built from 580ddd2f1267 : fails
built from bfd819293f59 : works

Triggered by:
580ddd2f1267	Kyle Huey — Bug 662200: Update the session history object for reloads. r=bz

Comment 1

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

So the problem is that we have an nsSHistory whose mIndex is 2 but only has 2 transactions (at indexed 0 and 1) so every navigation fails because prevEntry is null.

Comment 2

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

So the problem here is that the removeChild call causes us to destroy an nsSHTransaction, and then we try to fix things up and then mRequestedIndex is off the end of the nsSHistory.  When the reload completes, we set mIndex to mRequestedIndex and then every history navigation fails because the existing index doesn't actually exist.

Comment 3

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

1348     if (mIndex > aIndex) {
1349       mIndex = mIndex - 1;
1350     }
1351     if (mRequestedIndex > aIndex) {
1352       mRequestedIndex = mRequestedIndex - 1;
1353     }

seems bogus.  In this case, mRequestedIndex == aIndex, but aIndex is also the last nsSHTransaction.  I think these conditions should be

if (m[Requested]Index > aIndex ||
    (m[Requested]Index == aIndex && !txNext))

does that sound reasonable?

Comment 4

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

or perhaps

if (mRequestedIndex > aIndex || mRequestedIndex == aIndex == mLength)

they should be equivalent.

Comment 5

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

comment 4 looks ok to me.

Comment 6

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

Attachment: Patch

This fixes the reported issue.

Note the comment that I'm not sure if we're handling the case where mRequestedIndex == aIndex but aIndex is not the last transaction properly.

Comment 7

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

Comment on attachment 554451 [details] [diff] [review]
Patch

I assume there is a new patch coming

Comment 8

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

Attachment: Patch 2

Olli suggested the following instead on irc:

if (mRequestedIndex && ((mRequestedIndex > aIndex) ||
(mRequestedIndex == aIndex && !aKeepNext))) {

I've dropped the non-zero mRequestedIndex check at the beginning of the conditional, because I don't think that can ever be the deciding factor here.  Either:
1. mRequestedIndex > aIndex, and since aIndex >= 0, mRequestedIndex > 0
or
2. mRequestedIndex == aIndex.  If aIndex == 0, then aKeepNext must be true (because keeping the previous here makes no sense).

I've added an assert that we're never keeping the previous when aIndex == 0, and added the same logic for mIndex because I see no reason why it shouldn't apply there.

I've asked for two reviews because touching docshell code scares the crap out of me :-)

Comment 9

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

By the way, having a reproducible set of steps was *extremely* helpful here.  Thanks for that Alice.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 554866 [details] [diff] [review]
Patch 2

> +  NS_ASSERTION(!(aIndex == 0 && !aKeepNext),

I would find "NS_ASSERTION(aIndex != 0 || aKeepNext" somewhat easier to read, personally....

>or they are equal to aIndex in which case aKeepNext must be
>+    // true.

That needs an "if aIndex is 0" at the end, right?

sr=me with those dealt with or responded to.

Comment 11

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

(In reply to Boris Zbarsky (:bz) from comment #10)
> Comment on attachment 554866 [details] [diff] [review]
> Patch 2
> 
> > +  NS_ASSERTION(!(aIndex == 0 && !aKeepNext),
> 
> I would find "NS_ASSERTION(aIndex != 0 || aKeepNext" somewhat easier to
> read, personally....

Ok.

> >or they are equal to aIndex in which case aKeepNext must be
> >+    // true.
> 
> That needs an "if aIndex is 0" at the end, right?

Yes.

> sr=me with those dealt with or responded to.

Fixed up in my queue.

Comment 12

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

Comment on attachment 554866 [details] [diff] [review]
Patch 2

>+    if (mIndex > aIndex || (mIndex == aIndex && !aKeepNext)) {
mIndex should never be aIndex. There is even an assertion above.
So I don't see reason for this change.


>       mIndex = mIndex - 1;
>     }
>-    if (mRequestedIndex > aIndex) {
>+    if (mRequestedIndex > aIndex || (mRequestedIndex == aIndex && !aKeepNext)) {
>       mRequestedIndex = mRequestedIndex - 1;
>     }
This looks ok. r+ for this part.

Comment 13

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

That's a good point.  I'll remove those changes.

Comment 14

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

Attachment: Patch w/comments addressed

Comments addressed.  Carry forward reviews.

Comment 16

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

http://hg.mozilla.org/mozilla-central/rev/4599be47c692

I also committed the testcase for Bug 678872.

Comment 17

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

Comment on attachment 555088 [details] [diff] [review]
Patch w/comments addressed

Requesting approval-mozilla-aurora to fix this regression that causes back/forward to stop working.  The patch is pretty straightforward.

Comment 18

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

http://hg.mozilla.org/releases/mozilla-aurora/rev/fc27973f66b1

Comment 19

[u279076]

qa+ for verification in Firefox 8 and 9.

Comment 20

[AndreiD[QA]]

I tested the issue to see if it is still reproducible. Here are the results:
WORKS:
->  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0 (Win7 - Beta 3)
->  Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a2) Gecko/20111013 Firefox/9.0a2 (Win7 - Aurora)
-> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111014 Firefox/10.0a1 (Win7 - Nightly)

WORKS:
-> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0 (Mac OS 10.6 - Beta)
-> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111013 Firefox/9.0a2 (Mac OS 10.6 - Aurora)
-> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20111013 Firefox/10.0a1 (Mac OS 10.6 - Nightly)

WORKS:
-> Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0 (Ubuntu 11.04 - Beta)
-> Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111014 Firefox/9.0a2 (Ubuntu 11.04 - Aurora)
-> Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111014 Firefox/10.0a1 (Ubuntu 11.04 - Nightly)

Since  the issue is not reproducible anymore on all channels and across platforms, I'm setting this bug's resolution to verified and the qa flag to "qa!"

Comment 21

[LegNeato]

---------------------------------[ Triage Comment ]---------------------------------

Even though this is fixed we'll track it to make sure it doesn't bounce.