Bug 223832

Summary: Safari crashed and lost all tabs, after unlocking sleeping device
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: REOPENED ---    
Severity: Normal CC: bfulgham, ddkilzer, ggaren, jiewen_tan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Jiewen Tan 2021-03-26 17:42:39 PDT
Safari crashed and lost all tabs, after unlocking sleeping device.
Comment 1 Jiewen Tan 2021-03-26 17:42:49 PDT
rdar://75555287
Comment 2 Jiewen Tan 2021-03-26 17:49:31 PDT
Created attachment 424426 [details]
Patch
Comment 3 David Kilzer (:ddkilzer) 2021-03-27 08:43:00 PDT
Comment on attachment 424426 [details]
Patch

r=me
Comment 4 Alexey Proskuryakov 2021-03-27 09:59:12 PDT
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.
Comment 5 Jiewen Tan 2021-03-29 12:00:31 PDT
(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?
Comment 6 Jiewen Tan 2021-03-29 23:31:45 PDT
Comment on attachment 424426 [details]
Patch

Thanks Dave for r+ this patch.
Comment 7 EWS 2021-03-29 23:35:18 PDT
Committed r275197: <https://commits.webkit.org/r275197>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424426 [details].
Comment 8 Geoffrey Garen 2021-03-30 10:03:59 PDT
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.
Comment 9 Geoffrey Garen 2021-03-30 10:04:25 PDT
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.
Comment 10 Geoffrey Garen 2021-03-30 10:07:32 PDT
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.
Comment 11 Jiewen Tan 2021-03-30 11:51:26 PDT
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.
Comment 12 Jiewen Tan 2021-03-30 11:56:11 PDT
Reverted r275197 for reason:

The change is spooky.

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