Closed Bug 597315 Opened 9 years ago Closed 9 years ago

Frameset history does not work properly when restoring a tab

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bws42, Assigned: zpao)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre

When browsing a site with framesets like the Java API documentation, if you close the tab, then restore it and hit back the wrong frame will update.

The attached test case is a simplified copy of the API docs.

Reproducible: Always
Attached file reduced test case
That's certainly special.

Thanks for hunting down a regression range!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Regressed by Bug 462076.
blocking? We should fix it, but if it came down to this holding a release, I wouldn't. So I vote for block on it now so it stays on the radar, but we can reevaluate down the line if it comes to that.
blocking2.0: --- → ?
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
This happens even if the history is added once the tab is restored.

* Open the test case in a new tab
* Close then restore the tab
* Click a link in the bottom left
* Click back
Blocks: 462076
Component: Session Restore → Document Navigation
Product: Firefox → Core
QA Contact: session.restore → docshell
Hmm.  Paul, how does undo close tab handle subframes, exactly?
Blocking per Paul's request.
Assignee: nobody → paul
blocking2.0: ? → betaN+
(In reply to comment #7)
> Hmm.  Paul, how does undo close tab handle subframes, exactly?

So when collecting tab data, we iterate over each SHEntry and save it (and it's children) we also check and save isSubFrame. We'll also iterate over children if instanceof nsISHContainer.. http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1460

When restoring a tab, we read our data, call setIsSubFrame appropriately, etc. http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2636

We also set & read docIdentifier, ID.
Hmm, I wonder if session restore should handle docshellID somehow.
Ah, yes.  That could cause things to load in the wrong subframe....
Is docshellID something new then? AKAIK this wasn't a problem in the past. Are docshellIDs something that should persist across sessions? (I would assume so since that's not really any different than restoring a closed tab)
> Is docshellID something new then?

Yes, in bug 462076.  I should have realized that would need session restore changes...

And yes, if we want to preserve the correct history we need to restore the docshell id not only on session history entries but also on the corresponding docshells.  That last could be a bit of a pain...
Or we could just update docshell IDs in the session history entries.

It is still unclear to me how session restore works when frameset page is
restored - or what kinds of assumptions are made about the page.
My guess is that there are similar problem what session history used to have
with dynamically added/removed (i)frames.
(In reply to comment #14)
> My guess is that there are similar problem what session history used to have
> with dynamically added/removed (i)frames.
And seems like that is the case, since the testcase for bug 462076
breaks session restore. (Tested on 1.9.2)
(In reply to comment #15)
> (In reply to comment #14)
> > My guess is that there are similar problem what session history used to have
> > with dynamically added/removed (i)frames.
> And seems like that is the case, since the testcase for bug 462076
> breaks session restore. (Tested on 1.9.2)

I'd be more concerned with is it currently broken with session restore. We don't do anything fancy, just iterate over child entries (as I mentioned in comment #9). So what I see when testing in 3.6 vs 4.0 is that session restore saved & restored the state exactly as it was, broken or correct respectively. Perhaps I'm misinterpreting though.

I'm thinking we would want to do something similar to what was done for docIdentifiers
http://hg.mozilla.org/mozilla-central/file/tip/browser/components/sessionstore/src/nsSessionStore.js#l2697
At least as a starting point anyway... I'm not sure what would need to be done to actually make sure docshells have those ids we set...
Also, an API was added to SHEntry (setUniqueDocIdentifier) to make sure that worked. Would we need to do that here?
(In reply to comment #16)
>  We
> don't do anything fancy, just iterate over child entries
yeah, and that is *a* problem, not doing anything fancy.
Unfortunately session history isn't that simple when
(i)frames are added/modified dynamically.


> At least as a starting point anyway... I'm not sure what would need to be done
> to actually make sure docshells have those ids we set...
There are cases we can support and cases we can't.
If an SHEntry has dynamically added children, it is very hard to
handle all the cases properly.
So perhaps session restore shouldn't even try in those cases.

For non-dynamic cases:
Docshell should update its historyID when loading from 
session history
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8374
So if session restore code would just store the docshellID and give it
back to the shentries when restoring, that might work.
(Well, some tweaking is perhaps needed.)
Attached patch Patch v0.1 (WIP) (obsolete) — Splinter Review
So this doesn't actually work when frames are involved, but I didn't figure out why exactly. There's an NS_ERROR_FAILED when calling GotoIndex, but GDB was doing some bogus stuff on me and I didn't track down why that was happening - we end up going down into LoadEntry at least one additional time via http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1431
> There's an NS_ERROR_FAILED when calling GotoIndex

More specifically, this only happens when the index is not the first or the last. Otherwise it's now working.
Alright, so I've put in a spattering of printfs to help debug and I'm still not completely sure what the problem is.

FWIW, we don't get to http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8406 before we throw, so that could be part of the issue? So the docshell isn't updating mHistoryID. Not sure if that's relevant...

We end up throwing from GotoIndex via http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1373 in LoadEntry - not the first time through, but on one of the recursive calls.

Setting docshellIDs works just fine, however in nsSHistory mRootDocshell has a historyID different than any of the docshellIDs restored - that seems fine, I assume it would be getting updated in nsDocShell::InternalLoad. But we never actually get into InternalLoad (as mentioned above).

So in nsSHistory::LoadEntry, we end up going into CompareFrames, which is perfectly reasonable since we're loading a frame. But we never end up doing any real comparisons. When we try to get the child docshells of aParent (actually mRootDocShell) here: http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1508 - dsCount is 0. So we get into the loop, but dsChild is null so we bail

This is where I'm stuck. I see that I could set rootDocShell (I'm already QIing to nsISHistoryInternal to add the SHistoryListener anyway). I have no idea if that's even remotely close to the right thing to do though or what I would set it to...

Any help here would be appreciated.
So one problem seems to be is that session history assumes that the state of the docshell tree matches _some_ entry in the history list, but in this case that's not true, right?

What index does the session history think it's at before you call GotoIndex?
(In reply to comment #21)
> So one problem seems to be is that session history assumes that the state of
> the docshell tree matches _some_ entry in the history list, but in this case
> that's not true, right?

That's what it seems like

> What index does the session history think it's at before you call GotoIndex?

In a 4 entry session history, mIndex is 3.
Smaug, how do you feel about just adding an API that says "load entry at index N into mRootDocshell" on nsISHistory?

That is, instead of doing a history traversal (which is what we can do now and what we don't actually want to do here), throw away the current state and just call InitiateLoad(entry-at-index-n, mRootDocshell, some-load-type)?
Yeah, makes sense.

I could try to hack that tomorrow.
Actually, I think one should be able to load a specific entry
to root docshell using getEntryAtIndex(index, true) and reload().
Reload must happen on the nsSHistory object, so QI the nsISHistory
object to nsIWebNavigation and call reload() on that.

getEntryAtIndex(index, true) sets the mIndex of the history to index and
reload ends up calling nsSHistory::LoadEntry(mIndex, loadType, HIST_CMD_RELOAD).
LoadEntry then sets mRequestedIndex = aIndex and later checks
  if (mRequestedIndex == mIndex) {
    // Possibly a reload case 
    docShell = mRootDocShell;
  }

If we now have a docshell, 
InitiateLoad(nextEntry, docShell, aLoadType);
Note, QIing docshell to nsIWebNavigation and calling reload on that one
ends up doing something different.
(In reply to comment #25)
> Actually, I think one should be able to load a specific entry
> to root docshell using getEntryAtIndex(index, true) and reload().
> Reload must happen on the nsSHistory object, so QI the nsISHistory
> object to nsIWebNavigation and call reload() on that.

Yes, that works at least in the general case (I haven't tested it here specifically just yet though). I have done this already while trying to work around docshell/shistory before (remember bug 598852). I recall this having it's own issues, but if we're calling reload immediately, then I'm guessing it should be ok. I'll give this a shot in combination with restoring the docshellIDs.

That said, I really don't feel like I should have to keep working around issues like this. The API should work like it's advertised. gotoIndex should load the entry at the specified index. Frames or no frames, it should work. I could understand it not working given that I previously wasn't restoring the other information needed (docshellIDs), but I am now.

Ultimately this is a bug with that API and it should be fixed, or at the very least there should be a comment that says "session history works just fine normally but if you try to build history yourself, things are likely to go awry and you'll have to work around the problem by making calls to other APIs that weren't meant for that." </rant>
As far as I know, and see, session history API wasn't meant for anything
like session restore. It is really for session history.
The API is old, from year 1999 or so, and that shows up.
goToIndex does what it is meant to do when there is just session history.

For FF.next we should merge some interfaces, remove methods which aren't used
and add something which could be useful for session restore.
I say Firefox.next, since we shouldn't be changing interfaces at this point.
(In reply to comment #25)
> Actually, I think one should be able to load a specific entry
> to root docshell using getEntryAtIndex(index, true) and reload().
> Reload must happen on the nsSHistory object, so QI the nsISHistory
> object to nsIWebNavigation and call reload() on that.

Alright, so this sort of works. Things continue to work just peachy for non-frame entries. For frame entries, the right history entry is being loaded and it gets rendered. But going forward and back are broken. I don't see any assertions or anything in the terminal, in fact there's just nothing. Reload works and I can see that we're actually reloading based on console output.

If I go to a non-frame page while my history is broken, close & restore the tab, my history is fixed. I can go back and forward just fine, even though the frame entries which I couldn't get to before.

So this leads me to believe there's some other step that needs to be done. Perhaps what's happening in CompareFrames (since CompareFrames calls directly into InitiateLoad) - http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1508
Attached patch Patch v0.2 (WIP) (obsolete) — Splinter Review
Current WIP. Docshell changes are just for debugging, so just a make in browser is enough.

I've been using http://playground.zpao.com/omgtabs/frames.html as my frame test because it's much more lightweight than java docs.

1. open new tab, go to http://playground.zpao.com/omgtabs/frames.html
2. each frame click on a different number than is currently shown
   this should add 3 entries to history
3. go back 1
4. close tab
5. reopen tab

You should be in a broken state. Now it's usually broken with showing the right thing but not navigating. Occasionally it's also not loading one of the frames.
Attachment #487102 - Attachment is obsolete: true
Attached patch reloadCurrentEntry (obsolete) — Splinter Review
zpao, could you try this.
In the .js code I just replaced the .reload() with QI + .reloadCurrentEntry()
Attachment #490668 - Flags: feedback?(paul)
Comment on attachment 490668 [details] [diff] [review]
reloadCurrentEntry

Seems to be working for me in all the manual cases I had. I'll write a more complete set of mochitests next.

The only question I have is about my restoration of docshellIDs. In the current patch I took out my "sort of ensure that the id is unique" in favor of just setting it back to the value it had. Is this safe? What would happen if docshells were created with (for example) id 10 and then a tab was restored in a different tab/window and we set the docshellID to 10 there as well? (code below, commented out would set a "unique" id and ensure matching ids were mapped to same new value)

>+    if (aEntry.docshellID) {
>+      shEntry.docshellID = aEntry.docshellID;
>+      // get a new unique docshellID for this frame (since the one from the last
>+      // start might already be in use)
>+      //var docshellId = aDocshellIdMap[aEntry.docshellID] || 0;
>+      //if (!docshellId) {
>+        //for (docshellId = Date.now(); docshellId in aDocshellIdMap.used; docshellId++);
>+        //aDocshellIdMap[aEntry.docshellID] = docshellId;
>+        //aDocshellIdMap.used[docshellId] = true;
>+      //}
>+      //shEntry.docshellID = docshellId;
>+    }

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>+        // If we're loading something from session restore, docshell IDs
>+        // may be too small still.
>+        if (mHistoryID > gDocshellIDCounter) {
>+            gDocshellIDCounter = mHistoryID; 
>+        }

I'm guessing this would protect against creating a docshellID that's lower than the lowest id stored in session restore, but not the reverse?

I don't know if you ever saw it or if my testing profile just has something wrong with it, but the very first time I try to restore a tab with frames, I end up with 1 blank frame that never updates. After that everything is peachy. That's not necessarily this bug but I wanted to make sure you saw it in case it was related.
Attachment #490668 - Flags: feedback?(paul) → feedback+
(In reply to comment #32)
> Comment on attachment 490668 [details] [diff] [review]
> reloadCurrentEntry
> 
> Seems to be working for me in all the manual cases I had. I'll write a more
> complete set of mochitests next.
> 
> The only question I have is about my restoration of docshellIDs. In the current
> patch I took out my "sort of ensure that the id is unique" in favor of just
> setting it back to the value it had. Is this safe?
Should be safe. IDs are just IDs. They are global, but different session 
histories don't really know about each others.
And I did make a small change to update the global current ID if session
restore has some larger IDs.


> I don't know if you ever saw it or if my testing profile just has something
> wrong with it, but the very first time I try to restore a tab with frames, I
> end up with 1 blank frame that never updates.
I haven't seen anything like this.
Whiteboard: [has patch][needs test]
Whiteboard: [has patch][needs test] → [has patch][needs test][p?]
Whiteboard: [has patch][needs test][p?] → [has patch][needs test][d?]
Combined Olli's patch into mine, took out the crap, and added a test.

Used the reduced test case here... (licensing issue? - I know it's really simple html so not sure how that works)
Attachment #489848 - Attachment is obsolete: true
Attachment #490668 - Attachment is obsolete: true
Attachment #499169 - Flags: review?(dietrich)
Attachment #499169 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment on attachment 499169 [details] [diff] [review]
Patch v0.3 (combined, with test)

Boris - Olli wrote the shistory part of the patch and so can't review it. Fine by me if there's somebody else appropriate but I was under the impression you were the person here.

(might have been better to put the patch up in 2 parts for clarity but oh well)
Attachment #499169 - Flags: review?(Olli.Pettay) → review?(bzbarsky)
I see.  So which parts do you want me to review, then?
(In reply to comment #36)
> I see.  So which parts do you want me to review, then?

Everything not in browser/ (the last 100ish lines of the patch)
Comment on attachment 499169 [details] [diff] [review]
Patch v0.3 (combined, with test)

r=me on the docshell bits.
Attachment #499169 - Flags: review?(bzbarsky) → review+
Attachment #499169 - Flags: review?(dietrich) → review+
Pushed in 2 parts (for blame's sake)
docshell/ http://hg.mozilla.org/mozilla-central/rev/92104acff2b3
browser/ http://hg.mozilla.org/mozilla-central/rev/98b8d44e0096
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs test][d?]
Target Milestone: --- → mozilla2.0b9
in-testsuite+ for the session restore part.
Flags: in-testsuite+
(In reply to comment #34)
> Used the reduced test case here... (licensing issue? - I know it's really
> simple html so not sure how that works)

If there's any sort of issue, I grant all rights to the test case to the Mozilla Foundation.
Could this bug have caused the symptoms in bug 581349 (i.e., canGoBack failing)?  I'm trying to understand whether that bug is a dupe of this one...
(In reply to comment #39)
Those were the wrong links. Let's try that again...

Pushed in 2 parts (for blame's sake)
docshell/ http://hg.mozilla.org/mozilla-central/rev/93a8fb0292ba
browser/ http://hg.mozilla.org/mozilla-central/rev/e736115286a8

(In reply to comment #42)
> Could this bug have caused the symptoms in bug 581349 (i.e., canGoBack
> failing)?  I'm trying to understand whether that bug is a dupe of this one...

They seem pretty different, but maybe? Olli would know better. There's no mention of a session being restored or frames in that bug.
Blocks: 639771
You need to log in before you can comment on or make changes to this bug.