The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 8

Status

()

Core
Document Navigation
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: khuey)

Tracking

({regression})

Trunk
mozilla8
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox7 unaffected, firefox8+ fixed, firefox9 fixed)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Assignee: nobody → khuey
tracking-firefox8: --- → ?
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.
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.
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?
or perhaps

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

they should be equivalent.

Comment 5

6 years ago
comment 4 looks ok to me.
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.
Attachment #554451 - Flags: review?(Olli.Pettay)
status-firefox7: --- → unaffected
status-firefox8: --- → affected
status-firefox9: --- → affected

Comment 7

6 years ago
Comment on attachment 554451 [details] [diff] [review]
Patch

I assume there is a new patch coming
Attachment #554451 - Flags: review?(Olli.Pettay)
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 :-)
Attachment #554451 - Attachment is obsolete: true
Attachment #554866 - Flags: superreview?(bzbarsky)
Attachment #554866 - Flags: review?(Olli.Pettay)
By the way, having a reproducible set of steps was *extremely* helpful here.  Thanks for that Alice.
Summary: Back Button do not work even if history entry is available. Document navigation do not work by select back/forward drop down. → Back button can stop working after navigating backwards multiple SHEntries
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.
Attachment #554866 - Flags: superreview?(bzbarsky) → superreview+
(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 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.
Attachment #554866 - Flags: review?(Olli.Pettay) → review+
That's a good point.  I'll remove those changes.
Summary: Back button can stop working after navigating backwards multiple SHEntries → Session history navigation can break after removing an iframe from the document during a history navigation
Created attachment 555088 [details] [diff] [review]
Patch w/comments addressed

Comments addressed.  Carry forward reviews.
Attachment #554866 - Attachment is obsolete: true
Attachment #555088 - Flags: superreview+
Attachment #555088 - Flags: review+
Duplicate of this bug: 678872
http://hg.mozilla.org/mozilla-central/rev/4599be47c692

I also committed the testcase for Bug 678872.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
status-firefox9: affected → fixed
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.
Attachment #555088 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #555088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/fc27973f66b1
status-firefox8: affected → fixed
Target Milestone: mozilla9 → mozilla8
qa+ for verification in Firefox 8 and 9.
Whiteboard: [qa+]

Comment 20

6 years ago
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!"

Updated

6 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]

Comment 21

6 years ago
---------------------------------[ Triage Comment ]---------------------------------

Even though this is fixed we'll track it to make sure it doesn't bounce.
tracking-firefox8: ? → +
Depends on: 693020
You need to log in before you can comment on or make changes to this bug.