Bug 175685 – REGRESSION (r220601): Crash under ProcessAssertion::markAsInvalidated()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Crash under ProcessAssertion::markAsInvalidated():
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   WebKit                        	0x000000010270fe70 ___ZN6WebKit16ProcessAssertionC2EiNS_14AssertionStateEON3WTF8FunctionIFvvEEE_block_invoke_2.52 + 188 (Function.h:56)
1   WebKit                        	0x000000010270fe58 ___ZN6WebKit16ProcessAssertionC2EiNS_14AssertionStateEON3WTF8FunctionIFvvEEE_block_invoke_2.52 + 164 (ProcessAssertionIOS.mm:183)
2   libdispatch.dylib             	0x0000000183cd5124 _dispatch_call_block_and_release + 24 (init.c:994)
3   libdispatch.dylib             	0x0000000183cd50e4 _dispatch_client_callout + 16 (object.m:502)
4   libdispatch.dylib             	0x0000000183ce1bc4 _dispatch_main_queue_callback_4CF$VARIANT$mp + 1016 (inline_internal.h:2500)
5   CoreFoundation                	0x00000001842f7f20 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12 (CFRunLoop.c:1815)
6   CoreFoundation                	0x00000001842f5afc __CFRunLoopRun + 2012 (CFRunLoop.c:3111)
7   CoreFoundation                	0x00000001842162d8 CFRunLoopRunSpecific + 436 (CFRunLoop.c:3245)
8   GraphicsServices              	0x00000001860a8f84 GSEventRunModal + 100 (GSEvent.c:2245)
9   UIKit                         	0x000000018d7c2560 UIApplicationMain + 208 (UIApplication.m:3948)
10  MobileMail                    	0x000000010123df08 main + 160 (MobileMail.m:31)
11  libdyld.dylib                 	0x0000000183d3a56c start + 4

[Chris Dumez]

<rdar://problem/33868623>

[Chris Dumez]

Created attachment 318410 [details]
Patch

[Sam Weinig]

Comment on attachment 318410 [details]
Patch

I think it would be cleaner to set an empty callback as the default value in the constructor.

Index: UIProcess/ProcessAssertion.h
===================================================================
--- UIProcess/ProcessAssertion.h	(revision 220812)
+++ UIProcess/ProcessAssertion.h	(working copy)
@@ -51,7 +51,7 @@
 
 class ProcessAssertion {
 public:
-    ProcessAssertion(pid_t, AssertionState, Function<void()>&& invalidationCallback = { });
+    ProcessAssertion(pid_t, AssertionState, Function<void()>&& invalidationCallback = []() { });
     virtual ~ProcessAssertion();
 
     virtual void setClient(ProcessAssertionClient& client) { m_client = &client; }

[Chris Dumez]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 318410 [details]
> Patch
> 
> I think it would be cleaner to set an empty callback as the default value in
> the constructor.
> 
> Index: UIProcess/ProcessAssertion.h
> ===================================================================
> --- UIProcess/ProcessAssertion.h	(revision 220812)
> +++ UIProcess/ProcessAssertion.h	(working copy)
> @@ -51,7 +51,7 @@
>  
>  class ProcessAssertion {
>  public:
> -    ProcessAssertion(pid_t, AssertionState, Function<void()>&&
> invalidationCallback = { });
> +    ProcessAssertion(pid_t, AssertionState, Function<void()>&&
> invalidationCallback = []() { });
>      virtual ~ProcessAssertion();
>  
>      virtual void setClient(ProcessAssertionClient& client) { m_client =
> &client; }

I think this is a matter of taste. I personally do not like that we pretend there is a callback when there isn't. While this is not the case here, there are places (like in WebPage::markLayersVolatile() where this would mean more work too (like adding to a Vector and then iterating over that vector later on).

[Sam Weinig]

(In reply to Chris Dumez from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 318410 [details]
> > Patch
> > 
> > I think it would be cleaner to set an empty callback as the default value in
> > the constructor.
> > 
> > Index: UIProcess/ProcessAssertion.h
> > ===================================================================
> > --- UIProcess/ProcessAssertion.h	(revision 220812)
> > +++ UIProcess/ProcessAssertion.h	(working copy)
> > @@ -51,7 +51,7 @@
> >  
> >  class ProcessAssertion {
> >  public:
> > -    ProcessAssertion(pid_t, AssertionState, Function<void()>&&
> > invalidationCallback = { });
> > +    ProcessAssertion(pid_t, AssertionState, Function<void()>&&
> > invalidationCallback = []() { });
> >      virtual ~ProcessAssertion();
> >  
> >      virtual void setClient(ProcessAssertionClient& client) { m_client =
> > &client; }
> 
> I think this is a matter of taste. I personally do not like that we pretend
> there is a callback when there isn't. While this is not the case here, there
> are places (like in WebPage::markLayersVolatile() where this would mean more
> work too (like adding to a Vector and then iterating over that vector later
> on).

Ok. I think of it less as pretending there is callback when there isn't, more as the default callback is a no-op. But I don't have any real objections.

[WebKit Commit Bot]

Comment on attachment 318410 [details]
Patch

Rejecting attachment 318410 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 318410, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/4332591

[Chris Dumez]

Committed r220878: <http://trac.webkit.org/changeset/220878>

[Darin Adler]

To rephrase what Chris was saying, I think the issue is that since we don’t have any way to detect a no-op callback, in cases where we want to optimize the case where a callback isn’t needed, we have to use something other than a straight WTF::Function.