Bug 174166 - FileMonitor should not be ref counted
Summary: FileMonitor should not be ref counted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-05 11:30 PDT by Brent Fulgham
Modified: 2017-07-06 14:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (16.24 KB, patch)
2017-07-06 13:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.27 KB, patch)
2017-07-06 13:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-07-05 11:30:14 PDT
The current API for FileMonitor is fragile, because it requires the user to manually stop the monitor to make sure it is properly cleaned up.

This was done to make sure the event handler would never be left with a dangling this pointer. Instead, we should vend a WeakPtr to the closure, and check it when processing events to see if the FileMonitor was destroyed. If it was, we don't need to take any action.

This will allow the object to properly terminate monitoring when the object goes out of scope.
Comment 1 Chris Dumez 2017-07-06 13:03:39 PDT
Created attachment 314745 [details]
Patch
Comment 2 Chris Dumez 2017-07-06 13:07:35 PDT
Created attachment 314746 [details]
Patch
Comment 3 Brent Fulgham 2017-07-06 14:42:42 PDT
Comment on attachment 314746 [details]
Patch

Looks good! Nice cleanup.
Comment 4 Brent Fulgham 2017-07-06 14:43:15 PDT
Comment on attachment 314746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314746&action=review

> Source/WebCore/platform/FileMonitor.cpp:-46
> -}

Much simpler!
Comment 5 Chris Dumez 2017-07-06 14:45:08 PDT
Comment on attachment 314746 [details]
Patch

Clearing flags on attachment: 314746

Committed r219219: <http://trac.webkit.org/changeset/219219>
Comment 6 Chris Dumez 2017-07-06 14:45:10 PDT
All reviewed patches have been landed.  Closing bug.