Bug 213415 – [AppSSO] Should do null check on m_page in dismissViewController()

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Use protectedThis for dismissViewController().

[Jiewen Tan]

<rdar://problem/59672418>

[Jiewen Tan]

Created attachment 402343 [details]
Patch

[Darin Adler]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 402370 [details]
Patch

[Alexey Proskuryakov]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 402379 [details]
Patch

[Alexey Proskuryakov]

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.

[Jiewen Tan]

(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.

[Jiewen Tan]

Comment on attachment 402379 [details]
Patch

Thanks Alexey for r+ this patch.

[EWS]

Committed r263324: <https://trac.webkit.org/changeset/263324>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402379 [details].