Created attachment 64720 [details] Test case The attached test case crashes WebKit nightlies reliably when following the instructions in the left iframe (click, press back, then forward). Having the other iframe set window.name when it loads is necessary to trigger this. In debug mode the following assert fires: ASSERTION FAILED: fromItem (/Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/loader/HistoryController.cpp:553 void WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType)) Stack trace: WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType) + 123 (HistoryController.cpp:553) WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType) + 857 (HistoryController.cpp:585) WebCore::HistoryController::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 336 (HistoryController.cpp:239) WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 391 (Page.cpp:366) WebCore::Page::goForward() + 65 (Page.cpp:303) -[WebView goForward] + 111 (WebView.mm:3231) The problem isn't going forward per se, things are already screwed when back is pressed (the contents of the iframe do not change). (this was reported on the Chrome side as http://crbug.com/49721 and http://crbug.com/18048)
The problem here isn't the custom window name per se, it's that window.name is changing after the HistoryItem gets created. Then when we try to match things up in HistoryController::recursiveGoToItem, we can't find the frame with the right name in the fromItem tree, and the assert fires once we recurse. In general, HistoryItem/Controller using the window/frame name to match things up between seems like a bad idea given that it can change during the lifetime of a frameset. Having a stable "frame ID" and storing that in HistoryItem (instead of target) seems like the best thing to do.
(In reply to comment #1) > In general, HistoryItem/Controller using the window/frame name to match things up between seems like a bad idea given that it can change during the lifetime of a frameset. Having a stable "frame ID" and storing that in HistoryItem (instead of target) seems like the best thing to do. SGTM
BTW, the reason why this didn't crash before r61207 (which introduced itemSequenceNumber) is because this is the state at the start of the first recursiveGoToItem call: item: +-https://bug-44183-attachments.webkit.org/attachment.cgi?id=64720 (target: , isTargetItem: true) +-data:text/html,%3Ca%20href='data:text/html,press%20back,%20then%20forward'%3Eclick%20me%3C/a%3E (target: <!--framePath //<!--frame0-->-->, isTargetItem: false) +-data:text/html,%3Cscript%3Ewindow.name='foo';%3C/script%3Ewindow%20that%20changes%20its%20name (target: <!--framePath //<!--frame1-->-->, isTargetItem: false) fromItem: +-https://bug-44183-attachments.webkit.org/attachment.cgi?id=64720 (target: , isTargetItem: false) +-data:text/html,press%20back,%20then%20forward (target: <!--framePath //<!--frame0-->-->, isTargetItem: true) +-data:text/html,%3Cscript%3Ewindow.name='foo';%3C/script%3Ewindow%20that%20changes%20its%20name (target: foo, isTargetItem: false) The check before looping over children used to be "!item->isTargetItem()", and so we would just reload the whole frameset instead of trying to figure out which children had changed and which hadn't.
I was hoping to just change the target member of HistoryItem to store the frame ID (to avoid having to modify the port-specific serialization code). However, since the dumpBackForwardList function in the DRT includes the target name, that would change the output of all layout tests that use it (also, the output would no longer be deterministic, since I was generating frame IDs in a similar manner to item and document sequence numbers, which are seeded with the current time). Therefore, I think I need to do this change more incrementally, so that I don't have to fix everything at once. Proposed ordering is: 1) Fix up logic in HistoryController to better handle cases where we can't find frames by name (we'd need to do it when switching to frame IDs too, since those can also appear/disappear). That will fix this bug's crash, but result in more reloading than expected when going back/forward. 2) Add frame ID to Frame and HistoryItem, switch HistoryController (and FrameTree, to some degree) to use it, to fix the extra reloads. 3) Modify Chromium (and Qt?) HistoryItem (de)serialization to persist the frame ID and to not use isTargetItem and target 4) Modify DRT to no longer include the target in the output of dumpBackForwardList (and fix up expected outputs). Since I don't think it'll be feasible to update the DRT-equivalents for all the ports at once, I believe if I make target() from HistoryItem return an empty string, it won't be included in the output of dumpBackForwardList for any port, so I can do that first, then start removing the actual calls to target(). 4) Once all ports no longer use them, remove isTargetItem and target from HistoryItem
Created attachment 65292 [details] Patch
The attached patch implements step 1 from comment 4, to fix this particular bug and clean up matching up of frames between history items. I'll file separate bugs for the rest of the steps (adding frame ID, cleaning up callers of target() and isTargetItem(), etc.) once there's some agreement that that's the right way to go. Brady or Darin, could you take a look at the patch?
Comment on attachment 65292 [details] Patch WebCore/history/HistoryItem.cpp:458 + nit: only one new line here R=me
(In reply to comment #6) > The attached patch implements step 1 from comment 4, to fix this particular bug and clean up matching up of frames between history items. I'll file separate bugs for the rest of the steps (adding frame ID, cleaning up callers of target() and isTargetItem(), etc.) once there's some agreement that that's the right way to go. I like your plan.
Created attachment 65592 [details] Patch Removed extra newline in HistoryItem.cpp
Comment on attachment 65592 [details] Patch Clearing flags on attachment: 65592 Committed r66238: <http://trac.webkit.org/changeset/66238>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/66238 might have broken GTK Linux 64-bit Debug and Qt Linux Release
(In reply to comment #12) > http://trac.webkit.org/changeset/66238 might have broken GTK Linux 64-bit Debug and Qt Linux Release Filed bug 44784 about the GTK failure. I'm not seeing a consistent pattern for the Qt failures (they seem be change for every run), so I'm not filing a bug for that.
were the other steps in comment 4 checked in? because i found that in same case forwarding doesn't work: 1. Visit http://tinyurl.com/38xvefn 2. In the bottom (more or less, depends on how many comments people post below it) there is a place to post comments, just above it there are 3 small pictures of aircrafts (leading to related articles), click on the middle one for example. 3. wait for the page to load and press back 4. wait for the page to load completely (including the original scrolling position) and press forward instead of going to that related article, you will see the top of the first article.
(In reply to comment #14) > were the other steps in comment 4 checked in? > because i found that in same case forwarding doesn't work: > 1. Visit http://tinyurl.com/38xvefn > 2. In the bottom (more or less, depends on how many comments people post below it) there is a place to post comments, just above it there are 3 small pictures of aircrafts (leading to related articles), click on the middle one for example. > 3. wait for the page to load and press back > 4. wait for the page to load completely (including the original scrolling position) and press forward > > instead of going to that related article, you will see the top of the first article. Where were you verifying this? A nightly build of WebKit (at what revision)? Chrome? (which channel/version)? The fix has not reached everywhere (AFAIK it should only be in Chrome dev channel and WebKit nightly builds after r66238 i.e. August 27).
> Where were you verifying this? A nightly build of WebKit (at what revision)? Chrome? (which channel/version)? The fix has not reached everywhere (AFAIK it should only be in Chrome dev channel and WebKit nightly builds after r66238 i.e. August 27). i used chrome 7.0.517.8 (Official Build 59474) dev i wanted to try ToT but it's not usable currently did you try my repro steps from comment 14?
(In reply to comment #16) > > Where were you verifying this? A nightly build of WebKit (at what revision)? Chrome? (which channel/version)? The fix has not reached everywhere (AFAIK it should only be in Chrome dev channel and WebKit nightly builds after r66238 i.e. August 27). > > i used chrome 7.0.517.8 (Official Build 59474) dev > i wanted to try ToT but it's not usable currently > did you try my repro steps from comment 14? Thanks for verifying the version. I see what you mean; going forward doesn't actually load the expected page. However, that's a separate bug from this one (which was about that series of steps crashing WebKit). I've filed bug 45969 about it.
that's why i asked about the rest of the steps (i also guessed that your plan should deal with this issue and therefore wanted to know it was already checked in somewhere) thanks!