82653 2012-03-29 13:54:34 -0700 Merge FrameLoader::finishedLoading() into DocumentLoader::finishedLoading() 2012-03-30 14:42:24 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 japhet japhet abarth ap webkit.review.bot oldest_to_newest 591528 0 japhet 2012-03-29 13:54:34 -0700 FrameLoader::finishedLoading(): Calls a bunch of stuff on DocumentLoader (including finishedLoading), then calls FrameLoader::checkLoadComplete. There's no reason for this division, so long as we're careful to handle the fact that DocumentLoader::finishedLoading() is called by FrameLoader::init(), when things aren't yet in a fully consistent state.. 591531 1 134661 japhet 2012-03-29 13:57:42 -0700 Created attachment 134661 patch 591563 2 134661 abarth 2012-03-29 14:25:23 -0700 Comment on attachment 134661 patch View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review > Source/WebCore/loader/FrameLoader.cpp:-1979 > - // Retain because the stop may release the last reference to it. > - RefPtr<Frame> protect(m_frame); Do we not need this protect variable? 591566 3 japhet 2012-03-29 14:27:44 -0700 (In reply to comment #2) > (From update of attachment 134661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review > > > Source/WebCore/loader/FrameLoader.cpp:-1979 > > - // Retain because the stop may release the last reference to it. > > - RefPtr<Frame> protect(m_frame); > > Do we not need this protect variable? Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing. 591684 4 japhet 2012-03-29 16:15:20 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 134661 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review > > > > > Source/WebCore/loader/FrameLoader.cpp:-1979 > > > - // Retain because the stop may release the last reference to it. > > > - RefPtr<Frame> protect(m_frame); > > > > Do we not need this protect variable? > > Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing. I'm not sure there's a good way to tell whether it's necessary. Nothing in the callstack below is protecting the Frame, and DocumentLoader::m_frame is a raw pointer. On the other hand, I don't want to just cargo-cult it, especially when the comment uses a term as vague as "the stop" and was added in r17652. 591693 5 134661 abarth 2012-03-29 16:28:47 -0700 Comment on attachment 134661 patch View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review >>>> Source/WebCore/loader/FrameLoader.cpp:-1979 >>>> - RefPtr<Frame> protect(m_frame); >>> >>> Do we not need this protect variable? >> >> Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing. > > I'm not sure there's a good way to tell whether it's necessary. Nothing in the callstack below is protecting the Frame, and DocumentLoader::m_frame is a raw pointer. On the other hand, I don't want to just cargo-cult it, especially when the comment uses a term as vague as "the stop" and was added in r17652. checkLoadComplete() triggers the load event, right? We must have a test that rips and iframe out of the DOM when the load event fires. If this was necessary, we'd run into trouble on that test. I'm inclined to think it's not necessary, but I'm not sure how to become completely convinced. 592459 6 japhet 2012-03-30 13:43:07 -0700 (In reply to comment #5) > (From update of attachment 134661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134661&action=review > > >>>> Source/WebCore/loader/FrameLoader.cpp:-1979 > >>>> - RefPtr<Frame> protect(m_frame); > >>> > >>> Do we not need this protect variable? > >> > >> Nothing obviously broke, but I don't have proof that we don't need it. Will look more before committing. > > > > I'm not sure there's a good way to tell whether it's necessary. Nothing in the callstack below is protecting the Frame, and DocumentLoader::m_frame is a raw pointer. On the other hand, I don't want to just cargo-cult it, especially when the comment uses a term as vague as "the stop" and was added in r17652. > > checkLoadComplete() triggers the load event, right? We must have a test that rips and iframe out of the DOM when the load event fires. If this was necessary, we'd run into trouble on that test. I'm inclined to think it's not necessary, but I'm not sure how to become completely convinced. Ok, I think I'm convinced. 1. DocumentLoader is protected in MainResourceLoader::didFinishLoading(), so we don't need to worry about it. 2. The load event is fired from within m_writer.end() in DocumentLoader::finishedLoading(). 2a. Inside DocumentWrtier::end(), the callstack passes through the parser, then calls Document::finishedParsing(), which protects the Frame until it exits. 2b. Nothing happens between exiting finishedParsing() and exiting DocumentWriter::end() except nulling the parser. 3. Suppose that the Frame was detached during the load event. We therefore called FrameLoader::detachFromParent(), which called DocumentLoader::detachFromFrame(), which called DocumentLoader::stopLoading(). 4. stopLoading() called DocumentLoader::setMainResourceError() either directly, through mainReceivedError() or through MainResourceLoader::cancel(). 5. Therefore, when control returns to DocumentLoader::finishedLoading(), though m_frame is null, m_mainResourceError is now non-null. We exit finishedLoading() before dereferencing m_frame again. ...and indeed, if I reverse the 2 clauses in "if (!m_mainDocumentError.isNull() || frameLoader()->stateMachine()->creatingInitialEmptyDocument())", I get a null Frame* deref on, among others, fast/dom/Document/document-close-iframe-load.html 592509 7 134661 webkit.review.bot 2012-03-30 14:42:20 -0700 Comment on attachment 134661 patch Clearing flags on attachment: 134661 Committed r112728: <http://trac.webkit.org/changeset/112728> 592510 8 webkit.review.bot 2012-03-30 14:42:24 -0700 All reviewed patches have been landed. Closing bug. 134661 2012-03-29 13:57:42 -0700 2012-03-30 14:42:19 -0700 patch finish.txt text/plain 3611 japhet SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMjU3MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEyLTAzLTI5ICBOYXRlIENo YXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgTWVyZ2UgRnJhbWVMb2FkZXI6 OmZpbmlzaGVkTG9hZGluZygpIGludG8gRG9jdW1lbnRMb2FkZXI6OmZpbmlzaGVkTG9hZGluZygp LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODI2NTMK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcg dGVzdHMsIG5vIGZ1bmN0aW9uYWxpdHkgY2hhbmdlIGludGVuZGVkLgorCisgICAgICAgICogbG9h ZGVyL0RvY3VtZW50TG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50TG9hZGVy OjpmaW5pc2hlZExvYWRpbmcpOgorICAgICAgICAqIGxvYWRlci9GcmFtZUxvYWRlci5jcHA6Cisg ICAgICAgICogbG9hZGVyL0ZyYW1lTG9hZGVyLmg6CisgICAgICAgICogbG9hZGVyL01haW5SZXNv dXJjZUxvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpNYWluUmVzb3VyY2VMb2FkZXI6OmRp ZEZpbmlzaExvYWRpbmcpOgorCiAyMDEyLTAzLTI5ICBNaWNoYWwgTW9jbnkgIDxtbW9jbnlAZ29v Z2xlLmNvbT4KIAogICAgICAgICBbY2hyb21pdW1dIEVuc3VyZSBmcmFtZWJ1ZmZlciBleGlzdHMg YXQgdGhlIHN0YXJ0IG9mIGJlZ2luRHJhd2luZ0ZyYW1lLgpJbmRleDogU291cmNlL1dlYkNvcmUv bG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9sb2Fk ZXIvRnJhbWVMb2FkZXIuY3BwCShyZXZpc2lvbiAxMTI1NzApCisrKyBTb3VyY2UvV2ViQ29yZS9s b2FkZXIvRnJhbWVMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xOTczLDE5ICsxOTczLDYg QEAKICAgICBjYWNoZWRGcmFtZS5yZXN0b3JlKCk7CiB9CiAKLXZvaWQgRnJhbWVMb2FkZXI6OmZp bmlzaGVkTG9hZGluZygpCi17Ci0gICAgLy8gUmV0YWluIGJlY2F1c2UgdGhlIHN0b3AgbWF5IHJl bGVhc2UgdGhlIGxhc3QgcmVmZXJlbmNlIHRvIGl0LgotICAgIFJlZlB0cjxGcmFtZT4gcHJvdGVj dChtX2ZyYW1lKTsKLQotICAgIFJlZlB0cjxEb2N1bWVudExvYWRlcj4gZGwgPSBhY3RpdmVEb2N1 bWVudExvYWRlcigpOwotICAgIGRsLT5maW5pc2hlZExvYWRpbmcoKTsKLSAgICBpZiAoIWRsLT5t YWluRG9jdW1lbnRFcnJvcigpLmlzTnVsbCgpIHx8ICFkbC0+ZnJhbWVMb2FkZXIoKSkKLSAgICAg ICAgcmV0dXJuOwotICAgIGRsLT5zZXRQcmltYXJ5TG9hZENvbXBsZXRlKHRydWUpOwotICAgIGNo ZWNrTG9hZENvbXBsZXRlKCk7Ci19Ci0KIGJvb2wgRnJhbWVMb2FkZXI6OmlzSG9zdGVkQnlPYmpl Y3RFbGVtZW50KCkgY29uc3QKIHsKICAgICBIVE1MRnJhbWVPd25lckVsZW1lbnQqIG93bmVyID0g bV9mcmFtZS0+b3duZXJFbGVtZW50KCk7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvTWFp blJlc291cmNlTG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9sb2FkZXIv TWFpblJlc291cmNlTG9hZGVyLmNwcAkocmV2aXNpb24gMTEyNTcwKQorKysgU291cmNlL1dlYkNv cmUvbG9hZGVyL01haW5SZXNvdXJjZUxvYWRlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQ3Nyw3 ICs0NzcsNyBAQAogICAgICAgICBkbC0+bWF5YmVGaW5pc2hMb2FkaW5nTXVsdGlwYXJ0Q29udGVu dCgpOwogCiAgICAgZG9jdW1lbnRMb2FkZXIoKS0+dGltaW5nKCktPnNldFJlc3BvbnNlRW5kKGZp bmlzaFRpbWUgPyBmaW5pc2hUaW1lIDogKG1fdGltZU9mTGFzdERhdGFSZWNlaXZlZCA/IG1fdGlt ZU9mTGFzdERhdGFSZWNlaXZlZCA6IG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpKSk7Ci0g ICAgZnJhbWVMb2FkZXIoKS0+ZmluaXNoZWRMb2FkaW5nKCk7CisgICAgZG9jdW1lbnRMb2FkZXIo KS0+ZmluaXNoZWRMb2FkaW5nKCk7CiAgICAgUmVzb3VyY2VMb2FkZXI6OmRpZEZpbmlzaExvYWRp bmcoZmluaXNoVGltZSk7CiAKICAgICBkbC0+YXBwbGljYXRpb25DYWNoZUhvc3QoKS0+ZmluaXNo ZWRMb2FkaW5nTWFpblJlc291cmNlKCk7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9j dW1lbnRMb2FkZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1 bWVudExvYWRlci5jcHAJKHJldmlzaW9uIDExMjU3MCkKKysrIFNvdXJjZS9XZWJDb3JlL2xvYWRl ci9Eb2N1bWVudExvYWRlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI4OCwxMCArMjg4LDE0IEBA CiB7CiAgICAgbV9nb3RGaXJzdEJ5dGUgPSB0cnVlOyAgIAogICAgIGNvbW1pdElmUmVhZHkoKTsK LSAgICBpZiAoRnJhbWVMb2FkZXIqIGxvYWRlciA9IGZyYW1lTG9hZGVyKCkpIHsKLSAgICAgICAg bG9hZGVyLT5maW5pc2hlZExvYWRpbmdEb2N1bWVudCh0aGlzKTsKLSAgICAgICAgbV93cml0ZXIu ZW5kKCk7Ci0gICAgfQorICAgIGlmICghZnJhbWVMb2FkZXIoKSkKKyAgICAgICAgcmV0dXJuOwor ICAgIGZyYW1lTG9hZGVyKCktPmZpbmlzaGVkTG9hZGluZ0RvY3VtZW50KHRoaXMpOworICAgIG1f d3JpdGVyLmVuZCgpOworICAgIGlmICghbV9tYWluRG9jdW1lbnRFcnJvci5pc051bGwoKSB8fCBm cmFtZUxvYWRlcigpLT5zdGF0ZU1hY2hpbmUoKS0+Y3JlYXRpbmdJbml0aWFsRW1wdHlEb2N1bWVu dCgpKQorICAgICAgICByZXR1cm47CisgICAgc2V0UHJpbWFyeUxvYWRDb21wbGV0ZSh0cnVlKTsK KyAgICBmcmFtZUxvYWRlcigpLT5jaGVja0xvYWRDb21wbGV0ZSgpOwogfQogCiB2b2lkIERvY3Vt ZW50TG9hZGVyOjpjb21taXRMb2FkKGNvbnN0IGNoYXIqIGRhdGEsIGludCBsZW5ndGgpCkluZGV4 OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvRnJhbWVMb2FkZXIuaAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9sb2FkZXIvRnJhbWVMb2FkZXIuaAkocmV2aXNpb24gMTEyNTcwKQorKysgU291 cmNlL1dlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmgJKHdvcmtpbmcgY29weSkKQEAgLTE1Niw4 ICsxNTYsNiBAQAogCiAgICAgdm9pZCBoYW5kbGVGYWxsYmFja0NvbnRlbnQoKTsKIAotICAgIHZv aWQgZmluaXNoZWRMb2FkaW5nKCk7Ci0KICAgICBSZXNvdXJjZUVycm9yIGNhbmNlbGxlZEVycm9y KGNvbnN0IFJlc291cmNlUmVxdWVzdCYpIGNvbnN0OwogCiAgICAgYm9vbCBpc0hvc3RlZEJ5T2Jq ZWN0RWxlbWVudCgpIGNvbnN0Owo=