Summary: | REGRESSION (r220601): Crash under ProcessAssertion::markAsInvalidated() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | beidson, commit-queue, darin, ggaren, sam, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 175244 | ||||||
Attachments: |
|
Description
Chris Dumez
2017-08-17 13:28:30 PDT
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. |