WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
175685
REGRESSION (
r220601
): Crash under ProcessAssertion::markAsInvalidated()
https://bugs.webkit.org/show_bug.cgi?id=175685
Summary
REGRESSION (r220601): Crash under ProcessAssertion::markAsInvalidated()
Chris Dumez
Reported
2017-08-17 13:28:30 PDT
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
Attachments
Patch
(1.37 KB, patch)
2017-08-17 13:32 PDT
,
Chris Dumez
sam
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2017-08-17 13:28:46 PDT
<
rdar://problem/33868623
>
Chris Dumez
Comment 2
2017-08-17 13:32:03 PDT
Created
attachment 318410
[details]
Patch
Sam Weinig
Comment 3
2017-08-17 14:20:04 PDT
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
Comment 4
2017-08-17 14:24:32 PDT
(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
Comment 5
2017-08-17 14:30:40 PDT
(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 6
2017-08-17 14:32:20 PDT
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
Comment 7
2017-08-17 14:40:20 PDT
Committed
r220878
: <
http://trac.webkit.org/changeset/220878
>
Darin Adler
Comment 8
2017-08-18 09:14:11 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug