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
<rdar://problem/33868623>
Created attachment 318410 [details] Patch
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; }
(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).
(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.
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
Committed r220878: <http://trac.webkit.org/changeset/220878>
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.