Bug 223832 – Safari crashed and lost all tabs, after unlocking sleeping device

• Component: WebKit Misc.
• Status: REOPENED

[Jiewen Tan]

Safari crashed and lost all tabs, after unlocking sleeping device.

[Jiewen Tan]

rdar://75555287

[Jiewen Tan]

Created attachment 424426 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 424426 [details]
Patch

r=me

[Alexey Proskuryakov]

Comment on attachment 424426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424426&action=review

> Source/WebKit/ChangeLog:11
> +        A speculative fix for this crash. A possible explanation from the crash log suggests that the protectedThis
> +        could be elided because of compiler optimization given it is not used in the block. To prevent such optimization,
> +        protectedThis is therefore used explicitly in the block.

Is there a reason to stop at speculation? This would be visible in disassembly of the build where this occurred.

[Jiewen Tan]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 424426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424426&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        A speculative fix for this crash. A possible explanation from the crash log suggests that the protectedThis
> > +        could be elided because of compiler optimization given it is not used in the block. To prevent such optimization,
> > +        protectedThis is therefore used explicitly in the block.
> 
> Is there a reason to stop at speculation? This would be visible in
> disassembly of the build where this occurred.

Can we get this info just from the crash reports?

[Jiewen Tan]

Comment on attachment 424426 [details]
Patch

Thanks Dave for r+ this patch.

[EWS]

Committed r275197: <https://commits.webkit.org/r275197>

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

[Geoffrey Garen]

Comment on attachment 424426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424426&action=review

>>> Source/WebKit/ChangeLog:11
>>> +        protectedThis is therefore used explicitly in the block.
>> 
>> Is there a reason to stop at speculation? This would be visible in disassembly of the build where this occurred.
> 
> Can we get this info just from the crash reports?

You can get this information as follows:

(1) otool -tvV /path/to/WebKit

(2) type '/' to do a regex search

(3) search for dismissViewController (the function names will be mangled; you can use c++filt to unmangle a name and be sure you've got the right one)

(4) see if the disassembled code contains calls to ref/deref or, more likely, since ref/deref are usually inlined, see if there's a call to the SOAuthorizationSession operator delete or destructor (which would happen in the unlikely deref path).

But also:

This theory of the bug is definitely wrong. The compiler has no basis to eliminate a capture when the act of capturing obviously has side effects (by calling ref() in the capture and deref() when out of scope). Also, if the compiler did have such a bug, other WebKit code would crash all the time.

[Geoffrey Garen]

I think we should revert this patch and continue investigating. If you'd like to do the otool exercise first, to prove the patch had no effect, that's OK too.

[Geoffrey Garen]

To explain more, the reason I think we should revert is that it is harmful to have "spooky" code that is not verified by any test, and that contradicts our mental model of the computer. Once we enter that spooky universe / weird machine state, it is no longer possible to reason about our code, or understand whether future changes are OK or not.

[Jiewen Tan]

Comment on attachment 424426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424426&action=review

>>>> Source/WebKit/ChangeLog:11
>>>> +        protectedThis is therefore used explicitly in the block.
>>> 
>>> Is there a reason to stop at speculation? This would be visible in disassembly of the build where this occurred.
>> 
>> Can we get this info just from the crash reports?
> 
> You can get this information as follows:
> 
> (1) otool -tvV /path/to/WebKit
> 
> (2) type '/' to do a regex search
> 
> (3) search for dismissViewController (the function names will be mangled; you can use c++filt to unmangle a name and be sure you've got the right one)
> 
> (4) see if the disassembled code contains calls to ref/deref or, more likely, since ref/deref are usually inlined, see if there's a call to the SOAuthorizationSession operator delete or destructor (which would happen in the unlikely deref path).
> 
> But also:
> 
> This theory of the bug is definitely wrong. The compiler has no basis to eliminate a capture when the act of capturing obviously has side effects (by calling ref() in the capture and deref() when out of scope). Also, if the compiler did have such a bug, other WebKit code would crash all the time.

Let's revert the code change then.

[Jiewen Tan]

Reverted r275197 for reason:

The change is spooky.

Committed r275219 (235913@main): <https://commits.webkit.org/235913@main>