Closed
Bug 597315
Opened 14 years ago
Closed 14 years ago
Frameset history does not work properly when restoring a tab
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
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
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
And the regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=095418dfa2e6
Assignee | ||
Comment 3•14 years ago
|
||
That's certainly special. Thanks for hunting down a regression range!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•14 years ago
|
||
Regressed by Bug 462076.
Assignee | ||
Comment 5•14 years ago
|
||
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
Reporter | ||
Comment 6•14 years ago
|
||
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
Updated•14 years ago
|
Blocks: 462076
Component: Session Restore → Document Navigation
Product: Firefox → Core
QA Contact: session.restore → docshell
Comment 7•14 years ago
|
||
Hmm. Paul, how does undo close tab handle subframes, exactly?
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Comment 10•14 years ago
|
||
Hmm, I wonder if session restore should handle docshellID somehow.
Comment 11•14 years ago
|
||
Ah, yes. That could cause things to load in the wrong subframe....
Assignee | ||
Comment 12•14 years ago
|
||
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)
Comment 13•14 years ago
|
||
> 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...
Comment 14•14 years ago
|
||
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.
Comment 15•14 years ago
|
||
(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)
Assignee | ||
Comment 16•14 years ago
|
||
(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?
Comment 17•14 years ago
|
||
(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.)
Assignee | ||
Comment 18•14 years ago
|
||
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
Assignee | ||
Comment 19•14 years ago
|
||
> 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.
Assignee | ||
Comment 20•14 years ago
|
||
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.
Comment 21•14 years ago
|
||
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?
Assignee | ||
Comment 22•14 years ago
|
||
(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.
Comment 23•14 years ago
|
||
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)?
Comment 24•14 years ago
|
||
Yeah, makes sense. I could try to hack that tomorrow.
Comment 25•14 years ago
|
||
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);
Comment 26•14 years ago
|
||
Note, QIing docshell to nsIWebNavigation and calling reload on that one ends up doing something different.
Assignee | ||
Comment 27•14 years ago
|
||
(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>
Comment 28•14 years ago
|
||
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.
Assignee | ||
Comment 29•14 years ago
|
||
(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
Assignee | ||
Comment 30•14 years ago
|
||
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
Comment 31•14 years ago
|
||
zpao, could you try this. In the .js code I just replaced the .reload() with QI + .reloadCurrentEntry()
Attachment #490668 -
Flags: feedback?(paul)
Assignee | ||
Comment 32•14 years ago
|
||
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+
Comment 33•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch][needs test]
Updated•14 years ago
|
Whiteboard: [has patch][needs test] → [has patch][needs test][p?]
Updated•14 years ago
|
Whiteboard: [has patch][needs test][p?] → [has patch][needs test][d?]
Assignee | ||
Comment 34•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #499169 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #499169 -
Flags: review?(bzbarsky) → review?(Olli.Pettay)
Assignee | ||
Comment 35•14 years ago
|
||
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)
Comment 36•14 years ago
|
||
I see. So which parts do you want me to review, then?
Assignee | ||
Comment 37•14 years ago
|
||
(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 38•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #499169 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 39•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs test][d?]
Target Milestone: --- → mozilla2.0b9
Reporter | ||
Comment 41•14 years ago
|
||
(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.
Comment 42•14 years ago
|
||
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...
Assignee | ||
Comment 43•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•