Last Comment Bug 680344 - Session history navigation can break after removing an iframe from the document during a history navigation
: Session history navigation can break after removing an iframe from the docume...
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
:
Mentors:
: 678872 (view as bug list)
Depends on: 693020
Blocks: 662200
  Show dependency treegraph
 
Reported: 2011-08-18 22:23 PDT by Alice0775 White
Modified: 2013-09-24 02:33 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
fixed


Attachments
Patch (1.33 KB, patch)
2011-08-19 09:54 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
no flags Details | Diff | Review
Patch 2 (2.44 KB, patch)
2011-08-22 07:59 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bugs: review+
bzbarsky: superreview+
Details | Diff | Review
Patch w/comments addressed (2.41 KB, patch)
2011-08-23 06:26 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
khuey: review+
khuey: superreview+
asa: approval‑mozilla‑aurora+
Details | Diff | Review

Description Alice0775 White 2011-08-18 22:23:08 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-19 06:34:21 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-19 07:55:28 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-19 07:58:21 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-19 07:59:53 PDT
or perhaps

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

they should be equivalent.
Comment 5 Olli Pettay [:smaug] 2011-08-19 08:28:54 PDT
comment 4 looks ok to me.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-19 09:54:11 PDT
Created attachment 554451 [details] [diff] [review]
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] 2011-08-19 12:46:23 PDT
Comment on attachment 554451 [details] [diff] [review]
Patch

I assume there is a new patch coming
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-22 07:59:54 PDT
Created attachment 554866 [details] [diff] [review]
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-22 08:24:44 PDT
By the way, having a reproducible set of steps was *extremely* helpful here.  Thanks for that Alice.
Comment 10 Boris Zbarsky [:bz] 2011-08-22 23:57:14 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-23 04:42:55 PDT
(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] 2011-08-23 05:34:17 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-23 06:03:11 PDT
That's a good point.  I'll remove those changes.
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-23 06:26:41 PDT
Created attachment 555088 [details] [diff] [review]
Patch w/comments addressed

Comments addressed.  Carry forward reviews.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-24 07:46:23 PDT
*** Bug 678872 has been marked as a duplicate of this bug. ***
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-24 07:56:57 PDT
http://hg.mozilla.org/mozilla-central/rev/4599be47c692

I also committed the testcase for Bug 678872.
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-24 08:01:02 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-25 17:06:11 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/fc27973f66b1
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:57:35 PDT
qa+ for verification in Firefox 8 and 9.
Comment 20 AndreiD[QA] 2011-10-14 07:46:59 PDT
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 christian 2011-10-25 22:20:45 PDT
---------------------------------[ Triage Comment ]---------------------------------

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

Note You need to log in before you can comment on or make changes to this bug.