45430 2010-09-08 18:10:44 -0700 http/tests/navigation/anchor-frames.html doesn't pass when not run in isolation 2010-09-09 07:57:29 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 mihaip mihaip dpranke jamesr oldest_to_newest 276234 0 mihaip 2010-09-08 18:10:44 -0700 http/tests/navigation/anchor-frames.html doesn't pass with TestShell 276236 1 66978 mihaip 2010-09-08 18:12:04 -0700 Created attachment 66978 Patch 276243 2 dpranke 2010-09-08 18:22:56 -0700 this change LGTM. James, do you feel like rubber-stamping this? 276246 3 66978 jamesr 2010-09-08 18:28:07 -0700 Comment on attachment 66978 Patch I'd strongly prefer to know when TestShell is updating the scroll offset and having the test code run off of that. Running it in a setTimeout() might be racy depending on exactly when test shell updates the scroll offsets and is unreliable for a test. 276257 4 jamesr 2010-09-08 18:40:41 -0700 Alternately, decide we don't care about this test for TestShell and add it to the expectations. That makes me a little nervous but might a good option if it seems it will take a long time to debug what TestShell is doing. 276261 5 jamesr 2010-09-08 18:48:55 -0700 This test is currently failing on mac's DumpRenderTree as well (leopard and snow leopard). Sure this is a test shell problem? 276270 6 mihaip 2010-09-08 19:11:34 -0700 Re-titling, since this appears to fail with the regular DRT (though I can only reproduce locally when using --iterations 2). The setTimeout wrapper does seem to fix that, but I'd like to understand why better first. 276275 7 mihaip 2010-09-08 19:28:10 -0700 Not sure how the test ever worked. In FrameLoader::finishedParsing() we do things in this order: 1. Call FrameLoader::checkCompleted (which triggers the onload handler via FrameLoader::checkCallImplicitClose -> Document::implicitClose -> Document::dispatchWindowLoadEvent) 2. Call m_frame->view()->scrollToFragment(m_URL) So onload always fires before we scroll to the fragment. I think the original patch on this bug is the way to go, since the setTimeout(0) will definitely happen after both 1 and 2 above, so we'll always be scrolled. I'm going to be offline for a couple of hours. James/Dirk, feel free to either land this patch or back out r67033. 276278 8 jamesr 2010-09-08 19:48:45 -0700 Ahh, I see. So this patch is fine, then, since the setTimeout() will _always_ happen after the scroll. 276279 9 66978 jamesr 2010-09-08 19:48:57 -0700 Comment on attachment 66978 Patch I'll land this to fix the bots. 276280 10 66978 jamesr 2010-09-08 19:50:47 -0700 Comment on attachment 66978 Patch Clearing flags on attachment: 66978 Committed r67051: <http://trac.webkit.org/changeset/67051> 276281 11 jamesr 2010-09-08 19:51:01 -0700 All reviewed patches have been landed. Closing bug. 276459 12 mihaip 2010-09-09 07:57:29 -0700 (In reply to comment #7) > Not sure how the test ever worked. I think I understand this better. The test has external resources (two scripts and a stylesheet). IIRC, stylesheets do not block, so in the uncached case, checkCompleted returns early (because of the numRequests(m_frame->document()) check) and we end up calling scrollToFragment before onload is dispatched. However, in the cached case, the resources are (presumably) loaded synchronously from the cache, so we have no outstanding requests. This is why the test passed when run in isolation (or even when running all of the http/tests/navigation tests, since it's the first one to use those resources in there), but failed when run twice or when being run with all the other tests on the bots. 66978 2010-09-08 18:12:04 -0700 2010-09-08 19:50:47 -0700 Patch bug-45430-20100908181203.patch text/plain 2680 mihaip ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCAwYzhkN2FkYWJlZDMyYTBjNmE4OTI0ODk5NjYwODhjNWE5NjM0NGM0Li5kYzY0NGE3 YjdhZmY2NjNkZGU0NmE1NjEyMjgxNTVmY2E5MjVlYTY4IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0 cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAK KzIwMTAtMDktMDggIE1paGFpIFBhcnBhcml0YSAgPG1paGFpcEBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgaHR0cC90ZXN0cy9u YXZpZ2F0aW9uL2FuY2hvci1mcmFtZXMuaHRtbCBkb2Vzbid0IHBhc3Mgd2l0aCBUZXN0U2hlbGwK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ1NDMwCisK KyAgICAgICAgQ2hlY2sgc2Nyb2xsIHBvc2l0aW9uIGluIGEgdGltZW91dCwgb3RoZXJ3aXNlIFRl c3RTaGVsbCB3aWxsIHJlcG9ydCAwCisgICAgICAgIGZvciBpdC4KKworICAgICAgICAqIGh0dHAv dGVzdHMvbmF2aWdhdGlvbi9yZXNvdXJjZXMvZnJhbWUtd2l0aC1hbmNob3IuaHRtbDoKKwogMjAx MC0wOS0wNyAgTWloYWkgUGFycGFyaXRhICA8bWloYWlwQGNocm9taXVtLm9yZz4KIAogICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2h0 dHAvdGVzdHMvbmF2aWdhdGlvbi9yZXNvdXJjZXMvZnJhbWUtd2l0aC1hbmNob3IuaHRtbCBiL0xh eW91dFRlc3RzL2h0dHAvdGVzdHMvbmF2aWdhdGlvbi9yZXNvdXJjZXMvZnJhbWUtd2l0aC1hbmNo b3IuaHRtbAppbmRleCAxNzdiMTAwNjM2ODI4Y2UzZjI4NTdiYzgyOTc4YmUwM2QyZDRjMzVhLi4z YmU1ZDgxMjQzZjRhYzgwMDQ1N2QwZWY3MGRmYmNmMWI3NTIzNjhhIDEwMDY0NAotLS0gYS9MYXlv dXRUZXN0cy9odHRwL3Rlc3RzL25hdmlnYXRpb24vcmVzb3VyY2VzL2ZyYW1lLXdpdGgtYW5jaG9y Lmh0bWwKKysrIGIvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9uYXZpZ2F0aW9uL3Jlc291cmNlcy9m cmFtZS13aXRoLWFuY2hvci5odG1sCkBAIC03LDE3ICs3LDIxIEBACiAgICAgZnVuY3Rpb24gcnVu VGVzdCgpIHsKICAgICAgIGRlc2NyaXB0aW9uKCdUZXN0cyB0aGF0IGxvYWRpbmcgYSBmcmFtZSB3 aXRoIGEgVVJMIHRoYXQgY29udGFpbnMgYSBmcmFnbWVudCBwb2ludGVkIGF0IGEgbmFtZWQgYW5j aG9yIGFjdHVhbGx5IHNjcm9sbHMgdG8gdGhhdCBhbmNob3IuJyk7CiAKLSAgICAgIC8vIE1ha2Ug c3VyZSB0aGF0IHRoZSBib2R5IGlzIHRhbGxlciB0aGFuIHRoZSB2aWV3cG9ydCAoaS5lLiBzY3Jv bGxpbmcgaXMKLSAgICAgIC8vIHJlcXVpcmVkKS4KLSAgICAgIHNob3VsZEJlVHJ1ZSgnZG9jdW1l bnQuYm9keS5vZmZzZXRIZWlnaHQgPiBkb2N1bWVudC5kb2N1bWVudEVsZW1lbnQuY2xpZW50SGVp Z2h0Jyk7Ci0gICAgICAKLSAgICAgIC8vIFdlIHNob3VsZCBiZSBzY3JvbGxlZCBhdCBsZWFzdCBh IGxpdHRsZSBiaXQKLSAgICAgIHNob3VsZEJlVHJ1ZSgnZG9jdW1lbnQuYm9keS5zY3JvbGxUb3Ag PiAwJyk7Ci0gICAgICAKLSAgICAgIC8vIEFuZCB0aGUgYm90dG9tIG9mIHRoZSB2aWV3YWJsZSBh cmVhIHNob3VsZCBiZSBhdCBsZWFzdCAyMDAwIHBpeGVscyBmcm9tIHRoZSB0b3AsIGR1ZSB0byB0 aGUgc3BhY2VyIGVsZW1lbnQgYWJvdmUuCi0gICAgICBzaG91bGRCZVRydWUoJ2RvY3VtZW50LmJv ZHkuc2Nyb2xsVG9wICsgZG9jdW1lbnQuZG9jdW1lbnRFbGVtZW50LmNsaWVudEhlaWdodCA+IDIw MDAnKTsKLSAgICAgIAotICAgICAgZmluaXNoSlNUZXN0KCk7CisgICAgICAvLyBDaGVjayBzY3Jv bGwgcG9zaXRpb24gaW4gYSB0aW1lb3V0IHRvIG1ha2Ugc3VyZSB0aGF0IHRoZSBhbmNob3IgaGFz CisgICAgICAvLyBiZWVuIHNjcm9sbGVkIHRvLgorICAgICAgc2V0VGltZW91dChmdW5jdGlvbigp IHsKKyAgICAgICAgICAvLyBNYWtlIHN1cmUgdGhhdCB0aGUgYm9keSBpcyB0YWxsZXIgdGhhbiB0 aGUgdmlld3BvcnQgKGkuZS4gc2Nyb2xsaW5nIGlzCisgICAgICAgICAgLy8gcmVxdWlyZWQpLgor ICAgICAgICAgIHNob3VsZEJlVHJ1ZSgnZG9jdW1lbnQuYm9keS5vZmZzZXRIZWlnaHQgPiBkb2N1 bWVudC5kb2N1bWVudEVsZW1lbnQuY2xpZW50SGVpZ2h0Jyk7CisgICAgICAgICAgCisgICAgICAg ICAgLy8gV2Ugc2hvdWxkIGJlIHNjcm9sbGVkIGF0IGxlYXN0IGEgbGl0dGxlIGJpdAorICAgICAg ICAgIHNob3VsZEJlVHJ1ZSgnZG9jdW1lbnQuYm9keS5zY3JvbGxUb3AgPiAwJyk7CisgICAgICAg ICAgCisgICAgICAgICAgLy8gQW5kIHRoZSBib3R0b20gb2YgdGhlIHZpZXdhYmxlIGFyZWEgc2hv dWxkIGJlIGF0IGxlYXN0IDIwMDAgcGl4ZWxzIGZyb20gdGhlIHRvcCwgZHVlIHRvIHRoZSBzcGFj ZXIgZWxlbWVudCBhYm92ZS4KKyAgICAgICAgICBzaG91bGRCZVRydWUoJ2RvY3VtZW50LmJvZHku c2Nyb2xsVG9wICsgZG9jdW1lbnQuZG9jdW1lbnRFbGVtZW50LmNsaWVudEhlaWdodCA+IDIwMDAn KTsKKyAgICAgICAgICAKKyAgICAgICAgICBmaW5pc2hKU1Rlc3QoKTsgICAgICAgICAgCisgICAg ICB9LCAwKTsKICAgICB9CiAgICAgCiAgICAgdmFyIHN1Y2Nlc3NmdWxseVBhcnNlZCA9IHRydWU7 Cg==