26230 2009-06-05 18:52:49 -0700 JavascriptURL as frame src crasher. 2009-06-09 13:03:48 -0700 1 1 1 Unclassified WebKit Frames 528+ (Nightly build) All All RESOLVED FIXED P1 Normal --- 1 michaeln webkit-unassigned beidson darin fishd levin mjs oldest_to_newest 124606 0 michaeln 2009-06-05 18:52:49 -0700 This is a blocking bug for chrome. See http://code.google.com/p/chromium/issues/detail?id=12161 The incorrect javascript handling is a secondary issue. The blocker is the crasher. This webkit bug was split out of bug 16855, which is an umbrella for all manner of issues related to javascript urls. I hijacked that umbrella too much for this particular issue already, so have opened this new more focused bug. There are comments (and preliminary patches) specific to chrome's issue in bug 16855 that I have not copied here (so please see them there). As mentioned in the comments over there, I'm not so sure about the extra call to stopLoader(false) in FrameLoader::executeIfJavaScriptURL. What seems like is really missing is a call to cache()->loader()->cancelRequests(docLoader) for the document being torn down in the act of replacing the document. The call to stopLoader(false) gets that done and much much more. I'm concerned about what side effects the much much more may have (although all layout tests do pass). I took a few other stabs at it. 1) In DocumentLoader::stopLoading() call cancelRequests(). The method comments sound like this would be appropiate for that method. // Cancels the data source's pending loads. Conceptually, a data source only loads // one document at a time, but one document may have many related resources. // stopLoading will stop all loads initiated by the data source, // but not loads initiated by child frames' data sources -- that's the WebFrame's job. But... a handful of layout tests fail as a result. 2) In FrameLoader::executeIfJavaScriptURL() replace the call to stopLoading(false) with the more focused call to cancelRequests(). This alone resolves the very specific crashing bug that I'm seeing with javascript urls that construct the document programatically. I'm concerned there are other scenarios where the Document/DocLoader will get blown away while requests are still pending. We're seeing a number of stack traces that indicate that has occurred sometime after the fact. I think its unlikely that javascript urls in frameset documents are responsible for all of them. We think differences in protective refcounting between jsc and v8 bindings and or differences in gc strategies may be involved. Given the uncertainty, and the sevirity (CRASH!!!)... i took another stab at it that casts a wider net. 3) In DocLoader::~DocLoader(), dont just ASSERT(!m_requestCount), proactively cancel them... behind a PLATFORM(CHROMIUM) flag. This should not affect other platforms as they should never have active requests at dtor time (per the existing assertion). I'm leaning towards option 3 (for m3) unless we can determine a better fix real soon now. 124616 1 mrowe 2009-06-05 20:51:40 -0700 Do you have a test case or instructions on how to reproduce this bug? 124691 2 michaeln 2009-06-07 09:09:55 -0700 (In reply to comment #1) > Do you have a test case or instructions on how to reproduce this bug? Yes, see comment #9 in http://code.google.com/p/chromium/issues/detail?id=12161 124734 3 levin 2009-06-08 00:15:22 -0700 Independently, I was thinking that #3 is the best thing to do here (without the ifdef and remove the assert -- just do if (m_requestCount) cancelLoads.... The assert was added in: https://bugs.webkit.org/show_bug.cgi?id=23736 So far the fixes for that assert have been to call stop loader in various places in FrameLoader. So the fixes have only stopped the loading in a different place, but in ~DocLoader is the most robust place to stop the loading. 124782 4 31055 michaeln 2009-06-08 11:21:32 -0700 Created attachment 31055 Cancel requests in ~DocLoader Patch for option 3 as outlined above. I left the ASSERT in as well, after the call to cancel requests. 124783 5 31056 michaeln 2009-06-08 11:36:24 -0700 Created attachment 31056 Cancel requests in ~DocLoader (take2) Only call cancelRequests if the loader has pending requests as that method can be somewhat expensive. 124789 6 31056 eric 2009-06-08 13:01:57 -0700 Comment on attachment 31056 Cancel requests in ~DocLoader (take2) Your patch needs to include a test case. Even if that test case only crashes in Chromium. Your ChangeLog (or at least the bug) should document under which conditions this crash occurs. From reading the bug it seems it only occurs in Chromium? 124794 7 michaeln 2009-06-08 13:22:06 -0700 (In reply to comment #6) > From reading the bug it seems it only occurs in Chromium? I can't speak to other platforms (although per david's comments it seems there have been patches creeping in to cancel these pending requests at other callsites). I also don't know all of the circumstances in which chrome may trip on this. The one I know of for certain is the javascript url that constructs a document which has some resource loads initiated on its behalf. After that doc is torn down (in the act of replacing it with the simple string), chrome crashes. I think the number of crashes we're seeing indicate there are other circumstances as well. I didn't know a one line crashing bug fix needed a new test case (as no new functionality is being added). "If no layout test can be (or needs to be) constructed for the fix, you must explain why a new test isn't necessary to the reviewer." Is it not readily apparent that pending requests at DocLoader dtor time are a recipe for crashes? Will add one, I haven't added a layout-test yet (ever), may take me some hunting around to figure out how to plug what in where. 124801 8 michaeln 2009-06-08 13:56:36 -0700 I think i've found the right place for a test... fast/loader/ javascript-url-as-framesrc-crash.html An odd thing about this test is that the expected results at this time are not really correct. The document generated by the script url should be respected, but webcore doesn't do that yet and this patch doesn't address that correctness issue. 124821 9 31066 michaeln 2009-06-08 15:19:36 -0700 Created attachment 31066 Patch with tests Added a layout test in take 3. 124840 10 michaeln 2009-06-08 16:31:16 -0700 The expected output in this test will be the same regardless of which of the two possible documents webcore decides to populate the child frame with (simple string return value vs document.written data). So if/when the correctness issues with javascript urls are resolved, this test should still pass. 124864 11 eric 2009-06-08 18:11:02 -0700 (In reply to comment #7) > I didn't know a one line crashing bug fix needed a new test case (as no new > functionality is being added). "If no layout test can be (or needs to be) > constructed for the fix, you must explain why a new test isn't necessary to the > reviewer." Is it not readily apparent that pending requests at DocLoader dtor > time are a recipe for crashes? Of course crashing fixes need test cases. :) I guess we should fix our docs to make that more clear. We don't want to start crashing again in the future. If you can't make a test case for the crash fix, then we don't add one... but in this case, we can. 124865 12 eric 2009-06-08 18:12:21 -0700 CCing loader people. 124867 13 31066 eric 2009-06-08 18:14:07 -0700 Comment on attachment 31066 Patch with tests The question which remains to be answered is if this is the right place to do this cancelRequests. Clearly either the destructor should ASSERT(!m_requestCount) or it should cancel any pending requests. I think this looks OK to me. Please yell, any of you CC'd loader folks, if you disagree. 124997 14 bfulgham 2009-06-09 13:03:48 -0700 Landed in @r44541 31055 2009-06-08 11:21:32 -0700 2009-06-08 11:36:24 -0700 Cancel requests in ~DocLoader 26230.1.txt text/plain 1117 michaeln SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDUwMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDYtMDggIE1pY2hhZWwgTm9yZG1hbiAgPG1pY2hhZWxuQGdv b2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgUHJvYWN0aXZlbHkgY2FuY2VsIHBlbmRpbmcgcmVxdWVzdHMgYSBEb2NMb2FkZXIgZHRvciB0 aW1lLAorICAgICAgICBvdGhlcndpc2UgY3Jhc2hlcyBjYW4gb2NjdXIuCisKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI2MjMwCisKKyAgICAgICAgKiBs b2FkZXIvRG9jTG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRvY0xvYWRlcjo6fkRvY0xv YWRlcik6CisKIDIwMDktMDYtMDggIEFkYW0gTGFuZ2xleSAgPGFnbEBnb29nbGUuY29tPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IEVyaWMgU2llZGVsLgpJbmRleDogV2ViQ29yZS9sb2FkZXIvRG9j TG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2xvYWRlci9Eb2NMb2FkZXIuY3BwCShy ZXZpc2lvbiA0NDQzMSkKKysrIFdlYkNvcmUvbG9hZGVyL0RvY0xvYWRlci5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTYxLDYgKzYxLDggQEAgRG9jTG9hZGVyOjpEb2NMb2FkZXIoRG9jdW1lbnQqIGRv YykKIAogRG9jTG9hZGVyOjp+RG9jTG9hZGVyKCkKIHsKKyAgICBtX2NhY2hlLT5sb2FkZXIoKS0+ Y2FuY2VsUmVxdWVzdHModGhpcyk7CisKICAgICBjbGVhclByZWxvYWRzKCk7CiAgICAgRG9jdW1l bnRSZXNvdXJjZU1hcDo6aXRlcmF0b3IgZW5kID0gbV9kb2N1bWVudFJlc291cmNlcy5lbmQoKTsK ICAgICBmb3IgKERvY3VtZW50UmVzb3VyY2VNYXA6Oml0ZXJhdG9yIGl0ID0gbV9kb2N1bWVudFJl c291cmNlcy5iZWdpbigpOyBpdCAhPSBlbmQ7ICsraXQpCg== 31056 2009-06-08 11:36:24 -0700 2009-06-08 15:19:36 -0700 Cancel requests in ~DocLoader (take2) 26230.2.txt text/plain 1147 michaeln SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDUwMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDYtMDggIE1pY2hhZWwgTm9yZG1hbiAgPG1pY2hhZWxuQGdv b2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgUHJvYWN0aXZlbHkgY2FuY2VsIHBlbmRpbmcgcmVxdWVzdHMgYXQgRG9jTG9hZGVyIGR0b3Ig dGltZSwKKyAgICAgICAgb3RoZXJ3aXNlIGNyYXNoZXMgY2FuIG9jY3VyLgorCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNjIzMAorCisgICAgICAgICog bG9hZGVyL0RvY0xvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEb2NMb2FkZXI6On5Eb2NM b2FkZXIpOgorCiAyMDA5LTA2LTA4ICBBZGFtIExhbmdsZXkgIDxhZ2xAZ29vZ2xlLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBFcmljIFNpZWRlbC4KSW5kZXg6IFdlYkNvcmUvbG9hZGVyL0Rv Y0xvYWRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9sb2FkZXIvRG9jTG9hZGVyLmNwcAko cmV2aXNpb24gNDQ0MzEpCisrKyBXZWJDb3JlL2xvYWRlci9Eb2NMb2FkZXIuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC02MSw2ICs2MSw5IEBAIERvY0xvYWRlcjo6RG9jTG9hZGVyKERvY3VtZW50KiBk b2MpCiAKIERvY0xvYWRlcjo6fkRvY0xvYWRlcigpCiB7CisgICAgaWYgKG1fcmVxdWVzdENvdW50 KQorICAgICAgICBtX2NhY2hlLT5sb2FkZXIoKS0+Y2FuY2VsUmVxdWVzdHModGhpcyk7CisKICAg ICBjbGVhclByZWxvYWRzKCk7CiAgICAgRG9jdW1lbnRSZXNvdXJjZU1hcDo6aXRlcmF0b3IgZW5k ID0gbV9kb2N1bWVudFJlc291cmNlcy5lbmQoKTsKICAgICBmb3IgKERvY3VtZW50UmVzb3VyY2VN YXA6Oml0ZXJhdG9yIGl0ID0gbV9kb2N1bWVudFJlc291cmNlcy5iZWdpbigpOyBpdCAhPSBlbmQ7 ICsraXQpCg== 31066 2009-06-08 15:19:36 -0700 2009-06-08 18:14:07 -0700 Patch with tests 26230.3.txt text/plain 3407 michaeln SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDUwNykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMDktMDYtMDggIE1pY2hhZWwgTm9yZG1hbiAgPG1pY2hhZWxuQGdv b2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgUHJvYWN0aXZlbHkgY2FuY2VsIHBlbmRpbmcgcmVxdWVzdHMgYXQgRG9jTG9hZGVyIGR0b3Ig dGltZSwKKyAgICAgICAgb3RoZXJ3aXNlIGNyYXNoZXMgY2FuIG9jY3VyLiAKKworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjYyMzAKKyAgICAgICAgaHR0 cDovL2NvZGUuZ29vZ2xlLmNvbS9wL2Nocm9taXVtL2lzc3Vlcy9kZXRhaWw/aWQ9MTIxNjEKKwor ICAgICAgICBUZXN0OiBmYXN0L2ZyYW1lcy9qYXZhc2NyaXB0LXVybC1hcy1mcmFtZXNyYy1jcmFz aC5odG1sCisKKyAgICAgICAgKiBsb2FkZXIvRG9jTG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkRvY0xvYWRlcjo6fkRvY0xvYWRlcik6CisKIDIwMDktMDYtMDggIEFkYW0gTGFuZ2xleSAg PGFnbEBnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEVyaWMgU2llZGVsLgpJbmRl eDogV2ViQ29yZS9sb2FkZXIvRG9jTG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2xv YWRlci9Eb2NMb2FkZXIuY3BwCShyZXZpc2lvbiA0NDQzMSkKKysrIFdlYkNvcmUvbG9hZGVyL0Rv Y0xvYWRlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTYxLDYgKzYxLDkgQEAgRG9jTG9hZGVyOjpE b2NMb2FkZXIoRG9jdW1lbnQqIGRvYykKIAogRG9jTG9hZGVyOjp+RG9jTG9hZGVyKCkKIHsKKyAg ICBpZiAobV9yZXF1ZXN0Q291bnQpCisgICAgICAgIG1fY2FjaGUtPmxvYWRlcigpLT5jYW5jZWxS ZXF1ZXN0cyh0aGlzKTsKKwogICAgIGNsZWFyUHJlbG9hZHMoKTsKICAgICBEb2N1bWVudFJlc291 cmNlTWFwOjppdGVyYXRvciBlbmQgPSBtX2RvY3VtZW50UmVzb3VyY2VzLmVuZCgpOwogICAgIGZv ciAoRG9jdW1lbnRSZXNvdXJjZU1hcDo6aXRlcmF0b3IgaXQgPSBtX2RvY3VtZW50UmVzb3VyY2Vz LmJlZ2luKCk7IGl0ICE9IGVuZDsgKytpdCkKSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDQ0NTA3KQorKysg TGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIw MDktMDYtMDggIE1pY2hhZWwgTm9yZG1hbiAgPG1pY2hhZWxuQGdvb2dsZS5jb20+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUHJvYWN0aXZlbHkgY2Fu Y2VsIHBlbmRpbmcgcmVxdWVzdHMgYXQgRG9jTG9hZGVyIGR0b3IgdGltZSwKKyAgICAgICAgb3Ro ZXJ3aXNlIGNyYXNoZXMgY2FuIG9jY3VyLiAKKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjYyMzAKKyAgICAgICAgaHR0cDovL2NvZGUuZ29vZ2xlLmNv bS9wL2Nocm9taXVtL2lzc3Vlcy9kZXRhaWw/aWQ9MTIxNjEKKworICAgICAgICAqIGZhc3QvZnJh bWVzL2phdmFzY3JpcHQtdXJsLWFzLWZyYW1lc3JjLWNyYXNoLWV4cGVjdGVkLnR4dDogQWRkZWQu CisgICAgICAgICogZmFzdC9mcmFtZXMvamF2YXNjcmlwdC11cmwtYXMtZnJhbWVzcmMtY3Jhc2gu aHRtbDogQWRkZWQuCisKIDIwMDktMDYtMDYgIE1hcmsgUm93ZSAgPG1yb3dlQGFwcGxlLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBTYW0gV2VpbmlnLgpJbmRleDogTGF5b3V0VGVzdHMvZmFz dC9mcmFtZXMvamF2YXNjcmlwdC11cmwtYXMtZnJhbWVzcmMtY3Jhc2gtZXhwZWN0ZWQudHh0Cj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZnJhbWVzL2phdmFzY3JpcHQtdXJsLWFzLWZy YW1lc3JjLWNyYXNoLWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2Zh c3QvZnJhbWVzL2phdmFzY3JpcHQtdXJsLWFzLWZyYW1lc3JjLWNyYXNoLWV4cGVjdGVkLnR4dAko cmV2aXNpb24gMCkKQEAgLTAsMCArMSBAQAorCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2ZyYW1l cy9qYXZhc2NyaXB0LXVybC1hcy1mcmFtZXNyYy1jcmFzaC5odG1sCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExh eW91dFRlc3RzL2Zhc3QvZnJhbWVzL2phdmFzY3JpcHQtdXJsLWFzLWZyYW1lc3JjLWNyYXNoLmh0 bWwJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9mYXN0L2ZyYW1lcy9qYXZhc2NyaXB0LXVy bC1hcy1mcmFtZXNyYy1jcmFzaC5odG1sCShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDIxIEBACis8 aGVhZD4NCis8c2NyaXB0IGxhbmd1YWdlPSJKYXZhU2NyaXB0Ij4NCisvLyBTZWUgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI2MjMwDQorDQoraWYgKHdpbmRvdy5sYXlv dXRUZXN0Q29udHJvbGxlcikgew0KKyAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNUZXh0 KCk7DQorfQ0KKw0KK2Z1bmN0aW9uIG1ha2VGcmFtZUNvbnRlbnRzKCkgew0KKyAgICB2YXIgZG9j ID0gdGhlRnJhbWUuZG9jdW1lbnQ7DQorICAgIGRvYy5vcGVuKCk7DQorICAgIGRvYy53cml0ZSgn PGltZyBzcmM9bm8taW1hZ2UtcmVzb3VyY2UtcmVxdWlyZWQucG5nPicpOw0KKyAgICBkb2MuY2xv c2UoKTsNCisgICAgcmV0dXJuICJTVUNDRVNTIChhcyBsb25nIGFzIHdlIGRpZG4ndCBjcmFzaCwg Y2xhaW0gdmljdG9yeSkiOw0KK30NCis8L3NjcmlwdD4NCis8L2hlYWQ+DQorDQorPGZyYW1lc2V0 Pg0KKyAgICA8ZnJhbWUgbmFtZT0idGhlRnJhbWUiIHNyYz0iamF2YXNjcmlwdDpwYXJlbnQubWFr ZUZyYW1lQ29udGVudHMoKSI+DQorPC9mcmFtZXNldD4NCgpQcm9wZXJ0eSBjaGFuZ2VzIG9uOiBM YXlvdXRUZXN0cy9mYXN0L2ZyYW1lcy9qYXZhc2NyaXB0LXVybC1hcy1mcmFtZXNyYy1jcmFzaC5o dG1sCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KTmFtZTogc3ZuOmV4ZWN1dGFibGUKICAgKyAqCgo=