Bug 44183 – Crash in HistoryController::recursiveGoToItem when navigating in a frame while another frame has a custom window name

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Mihai Parparita]

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)

[Mihai Parparita]

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.

[Darin Fisher (:fishd, Google)]

(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

[Mihai Parparita]

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.

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 65292 [details]
Patch

[Mihai Parparita]

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?

[Darin Fisher (:fishd, Google)]

Comment on attachment 65292 [details]
Patch

WebCore/history/HistoryItem.cpp:458
 +  
nit: only one new line here

R=me

[Darin Fisher (:fishd, Google)]

(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.

[Mihai Parparita]

Created attachment 65592 [details]
Patch

Removed extra newline in HistoryItem.cpp

[WebKit Commit Bot]

Comment on attachment 65592 [details]
Patch

Clearing flags on attachment: 65592

Committed r66238: <http://trac.webkit.org/changeset/66238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66238 might have broken GTK Linux 64-bit Debug and Qt Linux Release

[Mihai Parparita]

(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.

[Yair Yogev]

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.

[Mihai Parparita]

(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).

[Yair Yogev]

> 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?

[Mihai Parparita]

(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.

[Yair Yogev]

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!