213415 2020-06-19 16:30:48 -0700 [AppSSO] Should do null check on m_page in dismissViewController() 2020-06-20 23:10:42 -0700 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jiewen_tan jiewen_tan bfulgham darin jiewen_tan webkit-bug-importer oldest_to_newest 1664595 0 jiewen_tan 2020-06-19 16:30:48 -0700 Use protectedThis for dismissViewController(). 1664596 1 jiewen_tan 2020-06-19 16:31:00 -0700 <rdar://problem/59672418> 1664597 2 402343 jiewen_tan 2020-06-19 16:34:28 -0700 Created attachment 402343 Patch 1664627 3 402343 darin 2020-06-19 17:10:12 -0700 Comment on attachment 402343 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. 1664674 4 402343 jiewen_tan 2020-06-19 18:58:55 -0700 Comment on attachment 402343 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. 1664680 5 402370 jiewen_tan 2020-06-19 19:07:38 -0700 Created attachment 402370 Patch 1664689 6 402370 ap 2020-06-19 19:38:50 -0700 Comment on attachment 402370 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. 1664715 7 402370 jiewen_tan 2020-06-20 00:40:56 -0700 Comment on attachment 402370 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. 1664716 8 402379 jiewen_tan 2020-06-20 00:41:19 -0700 Created attachment 402379 Patch 1664770 9 402379 ap 2020-06-20 12:39:38 -0700 Comment on attachment 402379 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. 1664825 10 jiewen_tan 2020-06-20 23:06:06 -0700 (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. 1664826 11 402379 jiewen_tan 2020-06-20 23:06:41 -0700 Comment on attachment 402379 Patch Thanks Alexey for r+ this patch. 1664827 12 ews-feeder 2020-06-20 23:10:41 -0700 Committed r263324: <https://trac.webkit.org/changeset/263324> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402379. 402343 2020-06-19 16:34:28 -0700 2020-06-19 19:07:36 -0700 Patch bug-213415-20200619163428.patch text/plain 2532 jiewen_tan U3VidmVyc2lvbiBSZXZpc2lvbjogMjYzMjIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGM1MDk0M2FmMzlhMzUyZDBl ZWM5NzhhZjNmZmE1MDg5MjlmYjVlMmMuLjhjNDFlYmQwNjYyYTIyYTU1YzlhNWVkOGQyYmE3YzI3 NGZhNGEwOGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMjAtMDYtMTkgIEppZXdlbiBU YW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KKworICAgICAgICBbQXBwU1NPXSBVc2UgcHJvdGVj dGVkVGhpcyBmb3IgZGlzbWlzc1ZpZXdDb250cm9sbGVyKCkKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMzQxNQorICAgICAgICA8cmRhcjovL3Byb2Js ZW0vNTk2NzI0MTg+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBVSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlvblNl c3Npb24ubW06CisgICAgICAgIChXZWJLaXQ6OlNPQXV0aG9yaXphdGlvblNlc3Npb246OmRpc21p c3NWaWV3Q29udHJvbGxlcik6CisKIDIwMjAtMDYtMTggIERhdmlkIEtpbHplciAgPGRka2lsemVy QGFwcGxlLmNvbT4KIAogICAgICAgICBbSVBDIGhhcmRlbmluZ10gT3B0aW9uU2V0PD4gdmFsdWVz IHNob3VsZCBiZSB2YWxpZGF0ZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNz L0NvY29hL1NPQXV0aG9yaXphdGlvbi9TT0F1dGhvcml6YXRpb25TZXNzaW9uLm1tIGIvU291cmNl L1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlvblNl c3Npb24ubW0KaW5kZXggYmM2MWE2OWU4ZDFiOGI3NDE4YmJkM2UxMjE2ZDBmYmFhOGU5MWE3NC4u MWMwMGQ1YmMyNDkxYTQ2OTI0M2Q4ZTNmMzE0Yzk1Y2Q2OWU1NzRkZCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlv blNlc3Npb24ubW0KKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3Jp emF0aW9uL1NPQXV0aG9yaXphdGlvblNlc3Npb24ubW0KQEAgLTMwMiw3ICszMDIsNyBAQCB2b2lk IFNPQXV0aG9yaXphdGlvblNlc3Npb246OmRpc21pc3NWaWV3Q29udHJvbGxlcigpCiAgICAgICAg ICAgICBpZiAobV9wcmVzZW50aW5nV2luZG93RGlkRGVtaW5pYXR1cml6ZU9ic2VydmVyKQogICAg ICAgICAgICAgICAgIHJldHVybjsKICAgICAgICAgICAgIG1fcHJlc2VudGluZ1dpbmRvd0RpZERl bWluaWF0dXJpemVPYnNlcnZlciA9IFtbTlNOb3RpZmljYXRpb25DZW50ZXIgZGVmYXVsdENlbnRl cl0gYWRkT2JzZXJ2ZXJGb3JOYW1lOk5TV2luZG93RGlkRGVtaW5pYXR1cml6ZU5vdGlmaWNhdGlv biBvYmplY3Q6cHJlc2VudGluZ1dpbmRvdyBxdWV1ZTpuaWwgdXNpbmdCbG9jazpbcHJvdGVjdGVk VGhpcyA9IG1ha2VSZWZQdHIodGhpcyksIHRoaXNdIChOU05vdGlmaWNhdGlvbiAqKSB7Ci0gICAg ICAgICAgICAgICAgZGlzbWlzc1ZpZXdDb250cm9sbGVyKCk7CisgICAgICAgICAgICAgICAgcHJv dGVjdGVkVGhpcy0+ZGlzbWlzc1ZpZXdDb250cm9sbGVyKCk7CiAgICAgICAgICAgICAgICAgW1tO U05vdGlmaWNhdGlvbkNlbnRlciBkZWZhdWx0Q2VudGVyXSByZW1vdmVPYnNlcnZlcjptX3ByZXNl bnRpbmdXaW5kb3dEaWREZW1pbmlhdHVyaXplT2JzZXJ2ZXIuZ2V0KCldOwogICAgICAgICAgICAg ICAgIG1fcHJlc2VudGluZ1dpbmRvd0RpZERlbWluaWF0dXJpemVPYnNlcnZlciA9IG51bGxwdHI7 CiAgICAgICAgICAgICB9XTsKQEAgLTMxNCw3ICszMTQsNyBAQCB2b2lkIFNPQXV0aG9yaXphdGlv blNlc3Npb246OmRpc21pc3NWaWV3Q29udHJvbGxlcigpCiAgICAgICAgIGlmIChtX2FwcGxpY2F0 aW9uRGlkVW5oaWRlT2JzZXJ2ZXIpCiAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgIG1fYXBw bGljYXRpb25EaWRVbmhpZGVPYnNlcnZlciA9IFtbTlNOb3RpZmljYXRpb25DZW50ZXIgZGVmYXVs dENlbnRlcl0gYWRkT2JzZXJ2ZXJGb3JOYW1lOk5TQXBwbGljYXRpb25EaWRVbmhpZGVOb3RpZmlj YXRpb24gb2JqZWN0Ok5TQXBwIHF1ZXVlOm5pbCB1c2luZ0Jsb2NrOltwcm90ZWN0ZWRUaGlzID0g bWFrZVJlZlB0cih0aGlzKSwgdGhpc10gKE5TTm90aWZpY2F0aW9uICopIHsKLSAgICAgICAgICAg IGRpc21pc3NWaWV3Q29udHJvbGxlcigpOworICAgICAgICAgICAgcHJvdGVjdGVkVGhpcy0+ZGlz bWlzc1ZpZXdDb250cm9sbGVyKCk7CiAgICAgICAgICAgICBbW05TTm90aWZpY2F0aW9uQ2VudGVy IGRlZmF1bHRDZW50ZXJdIHJlbW92ZU9ic2VydmVyOm1fYXBwbGljYXRpb25EaWRVbmhpZGVPYnNl cnZlci5nZXQoKV07CiAgICAgICAgICAgICBtX2FwcGxpY2F0aW9uRGlkVW5oaWRlT2JzZXJ2ZXIg PSBudWxscHRyOwogICAgICAgICB9XTsK 402370 2020-06-19 19:07:38 -0700 2020-06-20 00:41:18 -0700 Patch bug-213415-20200619190737.patch text/plain 1724 jiewen_tan U3VidmVyc2lvbiBSZXZpc2lvbjogMjYzMjIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGM1MDk0M2FmMzlhMzUyZDBl ZWM5NzhhZjNmZmE1MDg5MjlmYjVlMmMuLjhjNDFlYmQwNjYyYTIyYTU1YzlhNWVkOGQyYmE3YzI3 NGZhNGEwOGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMjAtMDYtMTkgIEppZXdlbiBU YW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KKworICAgICAgICBbQXBwU1NPXSBVc2UgcHJvdGVj dGVkVGhpcyBmb3IgZGlzbWlzc1ZpZXdDb250cm9sbGVyKCkKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMzQxNQorICAgICAgICA8cmRhcjovL3Byb2Js ZW0vNTk2NzI0MTg+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBVSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlvblNl c3Npb24ubW06CisgICAgICAgIChXZWJLaXQ6OlNPQXV0aG9yaXphdGlvblNlc3Npb246OmRpc21p c3NWaWV3Q29udHJvbGxlcik6CisKIDIwMjAtMDYtMTggIERhdmlkIEtpbHplciAgPGRka2lsemVy QGFwcGxlLmNvbT4KIAogICAgICAgICBbSVBDIGhhcmRlbmluZ10gT3B0aW9uU2V0PD4gdmFsdWVz IHNob3VsZCBiZSB2YWxpZGF0ZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNz L0NvY29hL1NPQXV0aG9yaXphdGlvbi9TT0F1dGhvcml6YXRpb25TZXNzaW9uLm1tIGIvU291cmNl L1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlvblNl c3Npb24ubW0KaW5kZXggYmM2MWE2OWU4ZDFiOGI3NDE4YmJkM2UxMjE2ZDBmYmFhOGU5MWE3NC4u YzA0MzhiZGRjMWZkMDMwODNhNDJlOTM4NjE5YjQ5Y2U2ODVlYTU4ZCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlv blNlc3Npb24ubW0KKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3Jp emF0aW9uL1NPQXV0aG9yaXphdGlvblNlc3Npb24ubW0KQEAgLTI5Nyw3ICsyOTcsOCBAQCB2b2lk IFNPQXV0aG9yaXphdGlvblNlc3Npb246OmRpc21pc3NWaWV3Q29udHJvbGxlcigpCiAKICAgICAv LyBUaGlzIGlzIGEgd29ya2Fyb3VuZCBmb3IgYW4gQXBwS2l0IGlzc3VlOiA8cmRhcjovL3Byb2Js ZW0vNTkxMjUzMjk+LgogICAgIC8vIFttX3NoZWV0V2luZG93IHNoZWV0UGFyZW50XSBpcyBudWxs IGlmIHRoZSBwYXJlbnQgaXMgbWluaW1pemVkIG9yIHRoZSBob3N0IGFwcCBpcyBoaWRkZW4uCi0g ICAgaWYgKGF1dG8gKnByZXNlbnRpbmdXaW5kb3cgPSBtX3BhZ2UtPnBsYXRmb3JtV2luZG93KCkp IHsKKyAgICBpZiAobV9wYWdlICYmIG1fcGFnZS0+cGxhdGZvcm1XaW5kb3coKSkgeworICAgICAg ICBhdXRvICpwcmVzZW50aW5nV2luZG93ID0gbV9wYWdlLT5wbGF0Zm9ybVdpbmRvdygpOwogICAg ICAgICBpZiAocHJlc2VudGluZ1dpbmRvdy5taW5pYXR1cml6ZWQpIHsKICAgICAgICAgICAgIGlm IChtX3ByZXNlbnRpbmdXaW5kb3dEaWREZW1pbmlhdHVyaXplT2JzZXJ2ZXIpCiAgICAgICAgICAg ICAgICAgcmV0dXJuOwo= 402379 2020-06-20 00:41:19 -0700 2020-06-20 23:10:41 -0700 Patch bug-213415-20200620004118.patch text/plain 1736 jiewen_tan U3VidmVyc2lvbiBSZXZpc2lvbjogMjYzMjIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGM1MDk0M2FmMzlhMzUyZDBl ZWM5NzhhZjNmZmE1MDg5MjlmYjVlMmMuLmEwYzVlODA1OGFhY2QzYjVlZjFlMGJmNDEzZGIzMDY2 NzNkNzkzZGMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMjAtMDYtMTkgIEppZXdlbiBU YW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KKworICAgICAgICBbQXBwU1NPXSBTaG91bGQgZG8g bnVsbCBjaGVjayBvbiBtX3BhZ2UgaW4gZGlzbWlzc1ZpZXdDb250cm9sbGVyKCkKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMzQxNQorICAgICAgICA8 cmRhcjovL3Byb2JsZW0vNTk2NzI0MTg+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0 aG9yaXphdGlvblNlc3Npb24ubW06CisgICAgICAgIChXZWJLaXQ6OlNPQXV0aG9yaXphdGlvblNl c3Npb246OmRpc21pc3NWaWV3Q29udHJvbGxlcik6CisKIDIwMjAtMDYtMTggIERhdmlkIEtpbHpl ciAgPGRka2lsemVyQGFwcGxlLmNvbT4KIAogICAgICAgICBbSVBDIGhhcmRlbmluZ10gT3B0aW9u U2V0PD4gdmFsdWVzIHNob3VsZCBiZSB2YWxpZGF0ZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJL aXQvVUlQcm9jZXNzL0NvY29hL1NPQXV0aG9yaXphdGlvbi9TT0F1dGhvcml6YXRpb25TZXNzaW9u Lm1tIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NPQXV0 aG9yaXphdGlvblNlc3Npb24ubW0KaW5kZXggYmM2MWE2OWU4ZDFiOGI3NDE4YmJkM2UxMjE2ZDBm YmFhOGU5MWE3NC4uYzA0MzhiZGRjMWZkMDMwODNhNDJlOTM4NjE5YjQ5Y2U2ODVlYTU4ZCAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0aW9uL1NP QXV0aG9yaXphdGlvblNlc3Npb24ubW0KKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29j b2EvU09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlvblNlc3Npb24ubW0KQEAgLTI5Nyw3ICsy OTcsOCBAQCB2b2lkIFNPQXV0aG9yaXphdGlvblNlc3Npb246OmRpc21pc3NWaWV3Q29udHJvbGxl cigpCiAKICAgICAvLyBUaGlzIGlzIGEgd29ya2Fyb3VuZCBmb3IgYW4gQXBwS2l0IGlzc3VlOiA8 cmRhcjovL3Byb2JsZW0vNTkxMjUzMjk+LgogICAgIC8vIFttX3NoZWV0V2luZG93IHNoZWV0UGFy ZW50XSBpcyBudWxsIGlmIHRoZSBwYXJlbnQgaXMgbWluaW1pemVkIG9yIHRoZSBob3N0IGFwcCBp cyBoaWRkZW4uCi0gICAgaWYgKGF1dG8gKnByZXNlbnRpbmdXaW5kb3cgPSBtX3BhZ2UtPnBsYXRm b3JtV2luZG93KCkpIHsKKyAgICBpZiAobV9wYWdlICYmIG1fcGFnZS0+cGxhdGZvcm1XaW5kb3co KSkgeworICAgICAgICBhdXRvICpwcmVzZW50aW5nV2luZG93ID0gbV9wYWdlLT5wbGF0Zm9ybVdp bmRvdygpOwogICAgICAgICBpZiAocHJlc2VudGluZ1dpbmRvdy5taW5pYXR1cml6ZWQpIHsKICAg ICAgICAgICAgIGlmIChtX3ByZXNlbnRpbmdXaW5kb3dEaWREZW1pbmlhdHVyaXplT2JzZXJ2ZXIp CiAgICAgICAgICAgICAgICAgcmV0dXJuOwo=