13422 2007-04-20 16:08:44 -0700 REGRESSION: Page reload loses page position 2007-08-22 18:12:46 -0700 1 1 1 Unclassified WebKit Page Loading 523.x (Safari 3) Mac (PowerPC) OS X 10.4 VERIFIED FIXED any. e.g. http://www.apple.com/airportextreme/sharing.html InRadar, Regression P1 Normal --- 1 justinwebkit webkit-unassigned beidson ddkilzer dev+webkit zwarich oldest_to_newest 13695 0 justinwebkit 2007-04-20 16:08:44 -0700 In the last several nightly builds (a few weeks, I believe), page reload behavior has changed slightly (in my opinion, it has broken). Reloading the page usually has normally returned the user to the position on the page the user was viewing prior to reloading, but now, the page is reloaded and the very top is displayed. I did find an open bug (http://bugs.webkit.org/show_bug.cgi?id=10223) related to seemingly sporadic reload behavior scroll position on pages with frames, but that bug is much older than this one, and this one occurs on pages without frames. 13694 1 beidson 2007-04-20 16:19:15 -0700 Oh no, this is not good! Can you regress this using the nightlies to find out when this broke? 13688 2 justinwebkit 2007-04-20 16:48:53 -0700 Why certainly. It worked up through revision 17354, and stopped working in 17388. 13686 3 beidson 2007-04-20 17:02:04 -0700 Thanks! That'll probably be a big help. Unfortunately, I know off the top of my hand thats when major loader refactoring was going on - many large patches completely upsetting the balance. I'll take a look at that range 13687 4 beidson 2007-04-20 17:08:34 -0700 Yup, my fears confirmed http://trac.webkit.org/projects/webkit/changeset/17365 seems to be an extremely likely culrpit, though a few others in that range might be as well... These "moving and renaming" patches often do more than just that (unintentionally of course) 13183 5 darin 2007-04-23 08:39:50 -0700 <rdar://problem/5153026> 2977 6 zwarich 2007-08-05 02:13:58 -0700 I can confirm that this regression is indeed caused by r17365. I will try to hunt down the exact change that causes it and hopefully come up with a patch. 2992 7 15846 zwarich 2007-08-06 01:10:53 -0700 Created attachment 15846 Proposed patch Here is a patch that fixes the bug. In revision 17365, many Objective C method bodies in WebFrame were moved to the C++ wrapper class WebFrameLoaderClient for performance reasons. However, in copying the updateHistoryForReload() method, a small error was introduced. In the original method, currItem is made the current history item of the frame: WebHistoryItem *currItem = _private->currentItem; In the copied version, this is changed to WebHistoryItem *currItem = m_webFrame->_private->previousItem; This patch corrects that change, which was carried over into the new loader code. 2995 8 15846 mrowe 2007-08-06 02:34:42 -0700 Comment on attachment 15846 Proposed patch This feels like it should be testable in a layout test. It'd be handy to have a test around so we can be sure we don't reintroduce this regression in the future. 2890 9 15846 beidson 2007-08-07 13:06:12 -0700 Comment on attachment 15846 Proposed patch Looks like a good fix, and good catch on the mistake in the patch that caused the original regression. r- because there is no layout test and this should be layout-testable We'd be happy to help you write one (#webkit on irc.freenode.net) but you can also refer to the other layout tests for guidance 2855 10 15863 zwarich 2007-08-07 20:28:11 -0700 Created attachment 15863 Proposed patch with test case Here is the patch with a test case. It turns out that the success200-reload test was designed to scroll the document and record the change, but nobody ever checked if it scrolled in the test output when it was made. I always run the layout tests after adding a fix, so I must have accidentally been running them for the older version of the tree I was using to find the bug. 2820 11 15863 beidson 2007-08-08 09:38:02 -0700 Comment on attachment 15863 Proposed patch with test case HAH. Very cool. 2817 12 beidson 2007-08-08 09:44:04 -0700 Landed in http://trac.webkit.org/projects/webkit/changeset/24934 2747 13 justinwebkit 2007-08-09 11:24:08 -0700 Thanks all, I've confirmed that this is working now. It's not clear to me whether I'm supposed to change the status, but I consider the bug fixed. 2741 14 beidson 2007-08-09 11:27:46 -0700 Mark it as verified, since you've VERIFIED it is fixed =D 2728 15 justinwebkit 2007-08-09 12:42:02 -0700 Verified :) 2431 16 zwarich 2007-08-12 22:36:05 -0700 I should note that this bug was actually noticed the day it was introduced by the person who introduced it: http://trac.webkit.org/projects/webkit/changeset/17368 We should probably be more careful to check changed test results in the future. 1670 17 darin 2007-08-22 18:12:46 -0700 (In reply to comment #16) > I should note that this bug was actually noticed the day it was introduced by > the person who introduced it: > > http://trac.webkit.org/projects/webkit/changeset/17368 > > We should probably be more careful to check changed test results in the future. My bad! 15846 2007-08-06 01:10:53 -0700 2007-08-07 20:28:11 -0700 Proposed patch reload.diff text/plain 1850 zwarich SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNDg4MSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDctMDgtMDYgIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1 d2F0ZXJsb28uY2E+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgRml4ZXMgPGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzNDIy PgorCisgICAgICAgIEJ1ZyAxMzQyMjogUkVHUkVTU0lPTjogUGFnZSByZWxvYWQgbG9zZXMgcGFn ZSBwb3NpdGlvbgorCisgICAgICAgICogbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OkZyYW1lTG9hZGVyOjp1cGRhdGVIaXN0b3J5Rm9yUmVsb2FkKToKKwogMjAwNy0w OC0wNiAgQW50dGkgIDxhbnR0aUBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGFy aW4uCkluZGV4OiBXZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViQ29yZS9sb2FkZXIvRnJhbWVMb2FkZXIuY3BwCShyZXZpc2lvbiAyNDg3NSkKKysrIFdlYkNv cmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDE1NSwxNSArNDE1 NSwxNSBAQCB2b2lkIEZyYW1lTG9hZGVyOjp1cGRhdGVIaXN0b3J5Rm9yUmVsb2FkCiAgICAgICAg IExPRyhIaXN0b3J5LCAiV2ViQ29yZUhpc3Rvcnk6IFVwZGF0aW5nIEhpc3RvcnkgZm9yIHJlbG9h ZCBpbiBmcmFtZSAlcyIsIGRvY3VtZW50TG9hZGVyKCktPnRpdGxlKCkudXRmOCgpLmRhdGEoKSk7 CiAjZW5kaWYKIAotICAgIGlmIChtX3ByZXZpb3VzSGlzdG9yeUl0ZW0pIHsKLSAgICAgICAgcGFn ZUNhY2hlKCktPnJlbW92ZShtX3ByZXZpb3VzSGlzdG9yeUl0ZW0uZ2V0KCkpOworICAgIGlmICht X2N1cnJlbnRIaXN0b3J5SXRlbSkgeworICAgICAgICBwYWdlQ2FjaGUoKS0+cmVtb3ZlKG1fY3Vy cmVudEhpc3RvcnlJdGVtLmdldCgpKTsKICAgICAKICAgICAgICAgaWYgKGxvYWRUeXBlKCkgPT0g RnJhbWVMb2FkVHlwZVJlbG9hZCkKLSAgICAgICAgICAgIHNhdmVTY3JvbGxQb3NpdGlvbkFuZFZp ZXdTdGF0ZVRvSXRlbShtX3ByZXZpb3VzSGlzdG9yeUl0ZW0uZ2V0KCkpOworICAgICAgICAgICAg c2F2ZVNjcm9sbFBvc2l0aW9uQW5kVmlld1N0YXRlVG9JdGVtKG1fY3VycmVudEhpc3RvcnlJdGVt LmdldCgpKTsKICAgICAKICAgICAgICAgLy8gU29tZXRpbWVzIGxvYWRpbmcgYSBwYWdlIGFnYWlu IGxlYWRzIHRvIGEgZGlmZmVyZW50IHJlc3VsdCBiZWNhdXNlIG9mIGNvb2tpZXMuIEJ1Z3ppbGxh IDQwNzIKICAgICAgICAgaWYgKGRvY3VtZW50TG9hZGVyKCktPnVucmVhY2hhYmxlVVJMKCkuaXNF bXB0eSgpKQotICAgICAgICAgICAgbV9wcmV2aW91c0hpc3RvcnlJdGVtLT5zZXRVUkwoZG9jdW1l bnRMb2FkZXIoKS0+cmVxdWVzdFVSTCgpKTsKKyAgICAgICAgICAgIG1fY3VycmVudEhpc3RvcnlJ dGVtLT5zZXRVUkwoZG9jdW1lbnRMb2FkZXIoKS0+cmVxdWVzdFVSTCgpKTsKICAgICB9CiAgICAg CiAgICAgLy8gRklYTUU6IDxyZGFyOi8vcHJvYmxlbS80ODgwMDY1PiAtIFRoaXMgd2lsbCBiZSBh IGhvb2sgaW50byB0aGUgV2ViQ29yZSBnbG9iYWwgaGlzdG9yeSwgYW5kIHRoaXMgbG9hZGVyL2Ns aWVudCBjYWxsIHdpbGwgYmUgcmVtb3ZlZAo= 15863 2007-08-07 20:28:11 -0700 2007-08-08 09:38:02 -0700 Proposed patch with test case reload.diff text/plain 3139 zwarich SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNDkyMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDctMDgtMDcgIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1 d2F0ZXJsb28uY2E+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgRml4ZXMgPGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzNDIy PgorCisgICAgICAgIEJ1ZyAxMzQyMjogUkVHUkVTU0lPTjogUGFnZSByZWxvYWQgbG9zZXMgcGFn ZSBwb3NpdGlvbgorCisgICAgICAgICogbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OkZyYW1lTG9hZGVyOjp1cGRhdGVIaXN0b3J5Rm9yUmVsb2FkKToKKwogMjAwNy0w OC0wOCAgSG9sZ2VyIEhhbnMgUGV0ZXIgRnJleXRoZXIgIDx6ZWNrZUBzZWxmaXNoLm9yZz4KIAog ICAgICAgICBSZXZpZXdlZCBieSBBZGFtLgpJbmRleDogV2ViQ29yZS9sb2FkZXIvRnJhbWVMb2Fk ZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAkocmV2 aXNpb24gMjQ4NzUpCisrKyBXZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTQxNTUsMTUgKzQxNTUsMTUgQEAgdm9pZCBGcmFtZUxvYWRlcjo6dXBkYXRlSGlz dG9yeUZvclJlbG9hZAogICAgICAgICBMT0coSGlzdG9yeSwgIldlYkNvcmVIaXN0b3J5OiBVcGRh dGluZyBIaXN0b3J5IGZvciByZWxvYWQgaW4gZnJhbWUgJXMiLCBkb2N1bWVudExvYWRlcigpLT50 aXRsZSgpLnV0ZjgoKS5kYXRhKCkpOwogI2VuZGlmCiAKLSAgICBpZiAobV9wcmV2aW91c0hpc3Rv cnlJdGVtKSB7Ci0gICAgICAgIHBhZ2VDYWNoZSgpLT5yZW1vdmUobV9wcmV2aW91c0hpc3RvcnlJ dGVtLmdldCgpKTsKKyAgICBpZiAobV9jdXJyZW50SGlzdG9yeUl0ZW0pIHsKKyAgICAgICAgcGFn ZUNhY2hlKCktPnJlbW92ZShtX2N1cnJlbnRIaXN0b3J5SXRlbS5nZXQoKSk7CiAgICAgCiAgICAg ICAgIGlmIChsb2FkVHlwZSgpID09IEZyYW1lTG9hZFR5cGVSZWxvYWQpCi0gICAgICAgICAgICBz YXZlU2Nyb2xsUG9zaXRpb25BbmRWaWV3U3RhdGVUb0l0ZW0obV9wcmV2aW91c0hpc3RvcnlJdGVt LmdldCgpKTsKKyAgICAgICAgICAgIHNhdmVTY3JvbGxQb3NpdGlvbkFuZFZpZXdTdGF0ZVRvSXRl bShtX2N1cnJlbnRIaXN0b3J5SXRlbS5nZXQoKSk7CiAgICAgCiAgICAgICAgIC8vIFNvbWV0aW1l cyBsb2FkaW5nIGEgcGFnZSBhZ2FpbiBsZWFkcyB0byBhIGRpZmZlcmVudCByZXN1bHQgYmVjYXVz ZSBvZiBjb29raWVzLiBCdWd6aWxsYSA0MDcyCiAgICAgICAgIGlmIChkb2N1bWVudExvYWRlcigp LT51bnJlYWNoYWJsZVVSTCgpLmlzRW1wdHkoKSkKLSAgICAgICAgICAgIG1fcHJldmlvdXNIaXN0 b3J5SXRlbS0+c2V0VVJMKGRvY3VtZW50TG9hZGVyKCktPnJlcXVlc3RVUkwoKSk7CisgICAgICAg ICAgICBtX2N1cnJlbnRIaXN0b3J5SXRlbS0+c2V0VVJMKGRvY3VtZW50TG9hZGVyKCktPnJlcXVl c3RVUkwoKSk7CiAgICAgfQogICAgIAogICAgIC8vIEZJWE1FOiA8cmRhcjovL3Byb2JsZW0vNDg4 MDA2NT4gLSBUaGlzIHdpbGwgYmUgYSBob29rIGludG8gdGhlIFdlYkNvcmUgZ2xvYmFsIGhpc3Rv cnksIGFuZCB0aGlzIGxvYWRlci9jbGllbnQgY2FsbCB3aWxsIGJlIHJlbW92ZWQKSW5kZXg6IExh eW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJ KHJldmlzaW9uIDI0OTIzKQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkp CkBAIC0xLDMgKzEsMTMgQEAKKzIwMDctMDgtMDcgIENhbWVyb24gWndhcmljaCAgPGN3endhcmlj aEB1d2F0ZXJsb28uY2E+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgVXBkYXRlZCB0ZXN0IGNhc2UgYmVjYXVzZSBvZiBmaXggZm9yIDxodHRwOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzQyMj4KKworICAgICAgICBCdWcgMTM0MjI6 IFJFR1JFU1NJT046IFBhZ2UgcmVsb2FkIGxvc2VzIHBhZ2UgcG9zaXRpb24KKworICAgICAgICAq IGh0dHAvdGVzdHMvbmF2aWdhdGlvbi9zdWNjZXNzMjAwLXJlbG9hZC1leHBlY3RlZC50eHQ6CisK IDIwMDctMDgtMDcgIERhdmlkIEtpbHplciAgPGRka2lsemVyQHdlYmtpdC5vcmc+CiAKICAgICAg ICAgUmV2aWV3ZWQgYnkgT2xpdmVyLgpJbmRleDogTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9uYXZp Z2F0aW9uL3N1Y2Nlc3MyMDAtcmVsb2FkLWV4cGVjdGVkLnR4dAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlv dXRUZXN0cy9odHRwL3Rlc3RzL25hdmlnYXRpb24vc3VjY2VzczIwMC1yZWxvYWQtZXhwZWN0ZWQu dHh0CShyZXZpc2lvbiAyNDg3NSkKKysrIExheW91dFRlc3RzL2h0dHAvdGVzdHMvbmF2aWdhdGlv bi9zdWNjZXNzMjAwLXJlbG9hZC1leHBlY3RlZC50eHQJKHdvcmtpbmcgY29weSkKQEAgLTEwNCw2 ICsxMDQsNyBAQCBsYXllciBhdCAoMTEsNTkyKSBzaXplIDIzMHgxMzAKICAgICAgIHRleHQgcnVu IGF0ICgzLDApIHdpZHRoIDE4NTogIk1vcmUgaW5pdGlhbCB0ZXh0IGJlZm9yZSB1c2VyIGlucHV0 LiIKICAgICAgIHRleHQgcnVuIGF0ICgxODgsMCkgd2lkdGggMDogIiAiCiAgICAgUmVuZGVyQlIg e0JSfSBhdCAoMywxMykgc2l6ZSAweDEzCitzY3JvbGxlZCB0byAwLDUwCiAKID09PT09PT09PT09 PT09IEJhY2sgRm9yd2FyZCBMaXN0ID09PT09PT09PT09PT09CiAgICAgICAgIGh0dHA6Ly8xMjcu MC4wLjE6ODAwMC9uYXZpZ2F0aW9uL3N1Y2Nlc3MyMDAtcmVsb2FkLmh0bWwgICoqbmF2IHRhcmdl dCoqCg==