186081 2018-05-29 21:09:31 -0700 REGRESSION(r216119): DocumentLoader::detachFromFrame still encounters nullptr frame 2018-05-31 14:20:03 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar, Regression P2 Normal --- 171604 1 bfulgham bfulgham cdumez commit-queue darin dbates ddkilzer ews-watchlist japhet oldest_to_newest 1428343 0 bfulgham 2018-05-29 21:09:31 -0700 In Bug 171604, we removed a nullptr check that we deemed unnecessary due to reentrancy checks and assertions that confirmed that the current frame is valid. However, long term monitoring of crash data indicates that we are still encountering nullptr frames in this call stack. It's unclear if PolicyChecker::stopCheck() can cause m_frame to be nulled out somehow. Since m_frame is Ref’d in this method, it doesn’t seem like FrameDestructionObserver could be causing this. Could one of these be happening? (1) DocumentLoader is being told to observe a different frame? (2) DocumentLoader is being destroyed, which would set m_frame to nullptr? We should put the nullptr check back, and add additional assertions to help catch this case in the wild. 1428346 1 341557 bfulgham 2018-05-29 21:25:59 -0700 Created attachment 341557 Patch 1428357 2 341557 darin 2018-05-29 21:59:48 -0700 Comment on attachment 341557 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341557&action=review > Source/WebCore/loader/DocumentLoader.cpp:1187 > + ASSERT(m_frame && m_frame->refCount() >= 1); No need to include "m_frame &&" in this second assertion since the one above will already have terminated the process if m_frame is nullptr. I don’t think this refCount assertion is valuable, because RefCountedBase never sets a reference count to 0, so that assertion will only fail if there is some kind of memory smashing bug. Instead we want to write something more like: ASSERT(!m_frame->deletionHasBegun()); > Source/WebCore/loader/DocumentLoader.cpp:1192 > + ASSERT(m_frame && m_frame->refCount() >= 1); Ditto. 1428436 3 341557 bfulgham 2018-05-30 08:27:19 -0700 Comment on attachment 341557 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341557&action=review >> Source/WebCore/loader/DocumentLoader.cpp:1187 >> + ASSERT(m_frame && m_frame->refCount() >= 1); > > No need to include "m_frame &&" in this second assertion since the one above will already have terminated the process if m_frame is nullptr. > > I don’t think this refCount assertion is valuable, because RefCountedBase never sets a reference count to 0, so that assertion will only fail if there is some kind of memory smashing bug. Instead we want to write something more like: > > ASSERT(!m_frame->deletionHasBegun()); I originally did try that, only to realize that m_frame is a ThreadSafeRefCounted, not RefCounted, and does not have a 'deletionHasBegun' member. Instead, if deletion has begun refCount() will return 0. 1428581 4 bfulgham 2018-05-30 13:45:12 -0700 <rdar://problem/34918109> 1428588 5 bfulgham 2018-05-30 14:02:39 -0700 I sat down wit Dan Bates and discussed the issue, and learned the following: 1. If the DocumentLoader is in 'm_waitingForNavigationPolicy', and we call 'policyChecker().stopCheck()', we will clear the frame as part of the cleanup phase. 2. If we are in 'm_waitingForContentPolicy', and we call stopCheck, we will also null out the m_frame. So the two most common crash traces are fully explained, since 'cancelPolicyCheckIfneeded()' can cause m_frame to become nullptr. Instead, we should pass 'protectedFrame' to the InspectorInstrumentation::loaderDetachedFromFrame, since that is guaranteed by the release assert to be non-null at the start of the method, and should stay non-null even while m_frame might be cleared out. 1428631 6 341610 bfulgham 2018-05-30 15:19:32 -0700 Created attachment 341610 Patch 1428638 7 dbates 2018-05-30 16:03:19 -0700 (In reply to Brent Fulgham from comment #5) > I sat down wit Dan Bates and discussed the issue, and learned the following: > > 1. If the DocumentLoader is in 'm_waitingForNavigationPolicy', and we call > 'policyChecker().stopCheck()', we will clear the frame as part of the > cleanup phase. > > 2. If we are in 'm_waitingForContentPolicy', and we call stopCheck, we will > also null out the m_frame. > Notice that DocumentLoader::detachFromFrame() is the only function that nullifies DocumentLoader::m_frame (via observeFrame(nullptr)). m_frame is being cleared because cancelPolicyCheckIfNeeded() is causing us to re-enter this function. (Note that it cannot be code above cancelPolicyCheckIfNeeded() that re-enters this function because we would see a RELEASE_ASSERT() crash in cancelPolicyCheckIfNeeded() at <https://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentLoader.cpp?rev=232147#L1810> and I have not seen reports for such crashes). > So the two most common crash traces are fully explained, since > 'cancelPolicyCheckIfneeded()' can cause m_frame to become nullptr. > > Instead, we should pass 'protectedFrame' to the > InspectorInstrumentation::loaderDetachedFromFrame, since that is guaranteed > by the release assert to be non-null at the start of the method, and should > stay non-null even while m_frame might be cleared out. I inadvertently suggested using protectedFrame. Instead we should just bail out early after cancelPolicyCheckIfneeded() is called if m_frame is null. Any chance we can write a test for this change? 1428975 8 341686 bfulgham 2018-05-31 12:55:35 -0700 Created attachment 341686 Patch 1428985 9 341686 ddkilzer 2018-05-31 13:33:45 -0700 Comment on attachment 341686 Patch rs=me, but please file a bugs.webkit.org bug to track creation of a test. 1428987 10 ddkilzer 2018-05-31 13:36:07 -0700 (In reply to David Kilzer (:ddkilzer) from comment #9) > Comment on attachment 341686 [details] > Patch > > rs=me, but please file a bugs.webkit.org bug to track creation of a test. And/or to implement a better fix, then add a FIXME comment to that code when you file the bug. 1428992 11 341686 commit-queue 2018-05-31 13:54:31 -0700 Comment on attachment 341686 Patch Clearing flags on attachment: 341686 Committed r232370: <https://trac.webkit.org/changeset/232370> 1428993 12 commit-queue 2018-05-31 13:54:32 -0700 All reviewed patches have been landed. Closing bug. 341557 2018-05-29 21:25:59 -0700 2018-05-30 15:19:30 -0700 Patch bug-186081-20180529212558.patch text/plain 1728 bfulgham U3VidmVyc2lvbiBSZXZpc2lvbjogMjMyMjc0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDZkYjk1OWViZjQxMTk3 OTBiNTZjNzFiOWI1ZTI4MzgzN2VhZjhmMC4uNjQ2MTUxN2FjMGNmMDZkMWUxNzhiMWJhMDM1MjEw ZDYxNjNhMjc5YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE4LTA1LTI5ICBCcmVu dCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04ocjIx NjExOSk6IERvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9tRnJhbWUgc3RpbGwgZW5jb3VudGVycyBu dWxscHRyIGZyYW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xODYwODEKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzM0OTE4MTA5PgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJldml2ZSB0aGUgbnVsbHB0 ciBjaGVjayB0aGF0IHdhcyByZW1vdmVkIGluIHIyMTYxMTkgc2luY2UgY3Jhc2ggZGF0YSBpbmRp Y2F0ZXMgdGhhdAorICAgICAgICBpdCBpcyBzdGlsbCBwb3NzaWJsZSB0byBoYXZlIGEgbnVsbHB0 ciBtX2ZyYW1lIGluIHRoaXMgbWV0aG9kLgorCisgICAgICAgICogbG9hZGVyL0RvY3VtZW50TG9h ZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9tRnJh bWUpOgorCiAyMDE4LTA1LTI5ICBSeW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgogCiAg ICAgICAgIGlPUyBXSzE6IE9jY2FzaW9uYWwgY3Jhc2ggaW4gc2FuaXRpemVkTWFya3VwRm9yRnJh Z21lbnRJbkRvY3VtZW50CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1l bnRMb2FkZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcApp bmRleCA3ZWQ0ZjY0YzkyMTE4MWU0OGFmOWJkNjAzMjU5ZWVkNTVkMWI4ODNhLi4zY2MyNjYxYTUw ZjZlMDE4ZDRmYjA3NDMyNzQ3ZmI1NTZiMjEzMjVkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9E b2N1bWVudExvYWRlci5jcHAKQEAgLTExODMsOSArMTE4MywxNiBAQCB2b2lkIERvY3VtZW50TG9h ZGVyOjpkZXRhY2hGcm9tRnJhbWUoKQogCiAgICAgbV9hcHBsaWNhdGlvbkNhY2hlSG9zdC0+c2V0 RE9NQXBwbGljYXRpb25DYWNoZShudWxscHRyKTsKIAorICAgIEFTU0VSVChtX2ZyYW1lKTsKKyAg ICBBU1NFUlQobV9mcmFtZSAmJiBtX2ZyYW1lLT5yZWZDb3VudCgpID49IDEpOworCiAgICAgY2Fu Y2VsUG9saWN5Q2hlY2tJZk5lZWRlZCgpOwogCi0gICAgSW5zcGVjdG9ySW5zdHJ1bWVudGF0aW9u Ojpsb2FkZXJEZXRhY2hlZEZyb21GcmFtZSgqbV9mcmFtZSwgKnRoaXMpOworICAgIEFTU0VSVCht X2ZyYW1lKTsKKyAgICBBU1NFUlQobV9mcmFtZSAmJiBtX2ZyYW1lLT5yZWZDb3VudCgpID49IDEp OworCisgICAgaWYgKG1fZnJhbWUpCisgICAgICAgIEluc3BlY3Rvckluc3RydW1lbnRhdGlvbjo6 bG9hZGVyRGV0YWNoZWRGcm9tRnJhbWUoKm1fZnJhbWUsICp0aGlzKTsKIAogICAgIG9ic2VydmVG cmFtZShudWxscHRyKTsKIH0K 341610 2018-05-30 15:19:32 -0700 2018-05-31 12:55:34 -0700 Patch bug-186081-20180530151931.patch text/plain 2039 bfulgham U3VidmVyc2lvbiBSZXZpc2lvbjogMjMyMzAxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjNlMjMwOGYyOWEwZjRm MTA5MTliZGNkNmU0NTg0NjVlNzhlYjY2ZC4uMGI5Mjc1OTk3ZTZlODNlZTg5YWY5NzUxOTJmOTdk NjA4YmUzNjI1NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDE4LTA1LTI5ICBCcmVu dCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04ocjIx NjExOSk6IERvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9tRnJhbWUgc3RpbGwgZW5jb3VudGVycyBu dWxscHRyIGZyYW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xODYwODEKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzM0OTE4MTA5PgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBudWxscHRyIGNoZWNr IHRoYXQgd2FzIHJlbW92ZWQgaW4gcjIxNjExOSB3YXMgc3RpbGwgbmVlZGVkIGJlY2F1c2Ugd2hl biB0aGUgRG9jdW1lbnRMb2FkZXIKKyAgICAgICAgaXMgaW4gJ21fd2FpdGluZ0Zvck5hdmlnYXRp b25Qb2xpY3knIG9yIGluICdtX3dhaXRpbmdGb3JDb250ZW50UG9saWN5JywgYSBjYWxsIHRvIHRo ZSBwb2xpY3kgY2hlY2tlcgorICAgICAgICB0byBzdG9wQ2hlY2sgd2Ugd2lsbCBjbGVhciB0aGUg ZnJhbWUgYXMgcGFydCBvZiB0aGUgY2xlYW51cCBwaGFzZS4KKworICAgICAgICBJbnN0ZWFkIG9m IHBhc3NpbmcgbV9mcmFtZSB0byBJbnNwZWN0b3JJbnN0cnVtZW50YXRpb246OmxvYWRlckRldGFj aGVkRnJvbUZyYW1lLCB3ZSBzb3VsZCB1c2UKKyAgICAgICAgdGhlIHByb3RlY3RlZEZyYW1lIHdo aWNoIGlzIGd1YXJhbnRlZWQgdG8gc3RpbGwgYmUgdmFsaWQgaW4gdGhlIGN1cnJlbnQgY2FsbHN0 YWNrLgorCisgICAgICAgICogbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OkRvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9tRnJhbWUpOgorCiAyMDE4LTA1LTMwICBK ZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgogCiAgICAgICAgIE1lZGlhIGVsZW1lbnRz IG91dHNpZGUgZnVsbHNjcmVlbiBzaG91bGQgbm90IGJlIGNvbnNpZGVyZWQgbWFpbiBjb250ZW50 LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcCBi L1NvdXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5jcHAKaW5kZXggN2VkNGY2NGM5 MjExODFlNDhhZjliZDYwMzI1OWVlZDU1ZDFiODgzYS4uN2Y2OWYzNDg0NDZjYjc3NWIyZDA4MjEy ODVlZGJjNjk2YmQzZTVlOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3Vt ZW50TG9hZGVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIu Y3BwCkBAIC0xMTg1LDcgKzExODUsMTAgQEAgdm9pZCBEb2N1bWVudExvYWRlcjo6ZGV0YWNoRnJv bUZyYW1lKCkKIAogICAgIGNhbmNlbFBvbGljeUNoZWNrSWZOZWVkZWQoKTsKIAotICAgIEluc3Bl Y3Rvckluc3RydW1lbnRhdGlvbjo6bG9hZGVyRGV0YWNoZWRGcm9tRnJhbWUoKm1fZnJhbWUsICp0 aGlzKTsKKyAgICAvLyBjYW5jZWxQb2xpY3lDaGVja0lmTmVlZGVkIGNhbiBzZXQgY2xlYXIgbV9m cmFtZSBpZiB0aGUgbG9hZAorICAgIC8vIGlzIHN0b3BwZWQsIHNvIHdlIG5lZWQgdG8gdXNlIHRo ZSBwcm90ZWN0ZWRGcmFtZSBoZXJlLCBzaW5jZSB3ZQorICAgIC8vIHN0aWxsIHdhbnQgdGhlIGlu c3BlY3RvciB0byBrbm93IHRoYXQgdGhlIGZyYW1lIHdhcyBkZXRhY2hlZC4KKyAgICBJbnNwZWN0 b3JJbnN0cnVtZW50YXRpb246OmxvYWRlckRldGFjaGVkRnJvbUZyYW1lKCpwcm90ZWN0ZWRGcmFt ZSwgKnRoaXMpOwogCiAgICAgb2JzZXJ2ZUZyYW1lKG51bGxwdHIpOwogfQo= 341686 2018-05-31 12:55:35 -0700 2018-05-31 13:54:31 -0700 Patch bug-186081-20180531125535.patch text/plain 1963 bfulgham U3VidmVyc2lvbiBSZXZpc2lvbjogMjMyMzY1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWExZTJkYzM5MGMxMTkx ZDdlZjU1YTJiMmRhY2MxOGZjNGY4M2Y1Ny4uYmIxNjI1ZjI5NzY3NjBiYjVhNmQ3NDg4NGVlODU3 YmJhYzk5ZWMzZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE4LTA1LTMxICBCcmVu dCBGdWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04ocjIx NjExOSk6IERvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9tRnJhbWUgc3RpbGwgZW5jb3VudGVycyBu dWxscHRyIGZyYW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xODYwODEKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzM0OTE4MTA5PgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBudWxscHRyIGNoZWNr IHRoYXQgd2FzIHJlbW92ZWQgaW4gcjIxNjExOSB3YXMgc3RpbGwgbmVlZGVkIGJlY2F1c2Ugd2hl biB0aGUgRG9jdW1lbnRMb2FkZXIKKyAgICAgICAgaXMgaW4gJ21fd2FpdGluZ0Zvck5hdmlnYXRp b25Qb2xpY3knIG9yIGluICdtX3dhaXRpbmdGb3JDb250ZW50UG9saWN5JywgYSBjYWxsIHRvIHRo ZSBwb2xpY3kgY2hlY2tlcgorICAgICAgICB0byBzdG9wQ2hlY2sgd2Ugd2lsbCBjbGVhciB0aGUg ZnJhbWUgYXMgcGFydCBvZiB0aGUgY2xlYW51cCBwaGFzZS4KKworICAgICAgICBJZiBtX2ZyYW1l IGlzIG51bGxwdHIgYWZ0ZXIgY2FuY2VsUG9saWN5Q2hlY2tJZk5lZWRlZCwgb3VyIHdvcmsgaXMg YWxyZWFkeSBkb25lIHNvIGp1c3QgcmV0dXJuLgorCisgICAgICAgICogbG9hZGVyL0RvY3VtZW50 TG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9t RnJhbWUpOgorCiAyMDE4LTA1LTMxICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+ CiAKICAgICAgICAgSW1wbGVtZW50IGNoZWNrZWQgY2FzdCBmb3IgRERSZXN1bHRSZWYgb25jZSBE RFJlc3VsdEdldENGVHlwZUlEKCkgaXMgYXZhaWxhYmxlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0Rv Y3VtZW50TG9hZGVyLmNwcAppbmRleCBhNmU0YTAxZGJjNGMzZDk1NjRhZmQyMGFiODgyZWY1ZDI3 NWNkNDNkLi4wYzEwYjliNTVlN2NlYTdhMzZlMzljOTI4YmE1MDg4M2MwYjg1MTI0IDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5jcHAKQEAgLTExODUsNiArMTE4NSwxMyBA QCB2b2lkIERvY3VtZW50TG9hZGVyOjpkZXRhY2hGcm9tRnJhbWUoKQogCiAgICAgY2FuY2VsUG9s aWN5Q2hlY2tJZk5lZWRlZCgpOwogCisgICAgLy8gY2FuY2VsUG9saWN5Q2hlY2tJZk5lZWRlZCBj YW4gY2xlYXIgbV9mcmFtZSBpZiB0aGUgcG9saWN5IGNoZWNrCisgICAgLy8gaXMgc3RvcHBlZCwg cmVzdWx0aW5nIGluIGEgcmVjdXJzaXZlIGNhbGwgaW50byB0aGlzIGRldGFjaEZyb21GcmFtZS4K KyAgICAvLyBJZiBtX2ZyYW1lIGlzIG51bGxwdHIgYWZ0ZXIgY2FuY2VsUG9saWN5Q2hlY2tJZk5l ZWRlZCwgb3VyIHdvcmsgaXMKKyAgICAvLyBhbHJlYWR5IGRvbmUgc28ganVzdCByZXR1cm4uCisg ICAgaWYgKCFtX2ZyYW1lKQorICAgICAgICByZXR1cm47CisKICAgICBJbnNwZWN0b3JJbnN0cnVt ZW50YXRpb246OmxvYWRlckRldGFjaGVkRnJvbUZyYW1lKCptX2ZyYW1lLCAqdGhpcyk7CiAKICAg ICBvYnNlcnZlRnJhbWUobnVsbHB0cik7Cg==