Use protectedThis for dismissViewController().
<rdar://problem/59672418>
Created attachment 402343 [details] Patch
Comment on attachment 402343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402343&action=review > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:305 > - dismissViewController(); > + protectedThis->dismissViewController(); There are tons of cases where we capture "this" in protectedThis, but then use "this" relying on the protection indirectly. Why is it important to change this one? I see no reason to make this change, but to leave the code two lines later using this to null out m_presentingWindowDidDeminiaturizeObserver. I like the idea of always using protectedThis, but I would do this and not even capture this, and even then, I am not sure we need to make a change. It’s a repeating pattern in a lot of different code in WebKit.
Comment on attachment 402343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402343&action=review >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:305 >> + protectedThis->dismissViewController(); > > There are tons of cases where we capture "this" in protectedThis, but then use "this" relying on the protection indirectly. Why is it important to change this one? I see no reason to make this change, but to leave the code two lines later using this to null out m_presentingWindowDidDeminiaturizeObserver. > > I like the idea of always using protectedThis, but I would do this and not even capture this, and even then, I am not sure we need to make a change. It’s a repeating pattern in a lot of different code in WebKit. Thanks for pointing it out. I was confused as well. I have received a crash report indicating Line 317 could crash, and therefore this is the most reasonable patch I could come out with. Now after reading other crash logs, it turns out that those crashed at a different place which actually makes sense. Will upload another patch.
Created attachment 402370 [details] Patch
Comment on attachment 402370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402370&action=review > Source/WebKit/ChangeLog:3 > + [AppSSO] Use protectedThis for dismissViewController() This is not what this patch is about.
Comment on attachment 402370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402370&action=review >> Source/WebKit/ChangeLog:3 >> + [AppSSO] Use protectedThis for dismissViewController() > > This is not what this patch is about. Oops. Forgot to update the change log.
Created attachment 402379 [details] Patch
Comment on attachment 402379 [details] Patch That's one of the reasons why it's better to put the symptom/motivation into radar title, not a description of what change is being made.
(In reply to Alexey Proskuryakov from comment #9) > Comment on attachment 402379 [details] > Patch > > That's one of the reasons why it's better to put the symptom/motivation into > radar title, not a description of what change is being made. Got you. Will do that next time.
Comment on attachment 402379 [details] Patch Thanks Alexey for r+ this patch.
Committed r263324: <https://trac.webkit.org/changeset/263324> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402379 [details].