Bug 182365

Summary: [JSCOnly] Ensure RunLoop::Timer is robust to being deleted inside its user callback
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: Web Template FrameworkAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED INVALID    
Severity: Normal CC: benjamin, cdumez, cmarcelo, dbates, ews-watchlist, mcatanzaro, ysuzuki
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=182271
Attachments:
Description Flags
Patch mcatanzaro: review-

Michael Catanzaro
Reported 2018-01-31 16:54:47 PST
Ensure RunLoop::Timer is robust to being deleted inside its user callback. This is a theoretical issue that I noticed as the result of an actual use-after-free caught by asan in WPE and GTK. See bug #182271. It's not actually possible to test the original reproducer using JSCOnly, because it was a WebKit-layer problem. I'm going to attach a totally-untested, speculative fix for the theoretical issue. I think it's correct.
Attachments
Patch (1.37 KB, patch)
2018-01-31 17:04 PST, Michael Catanzaro 		mcatanzaro: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2018-01-31 17:04:48 PST
Created attachment 332825 [details] Patch
Michael Catanzaro
Comment 2 2018-01-31 17:08:59 PST
This should be reviewed by Yusuke, because I'm really not sure whether it's necessary or not. If the RunLoop itself is guaranteed to have another ref on the ScheduledTask, then this isn't needed.
Yusuke Suzuki
Comment 3 2018-02-01 00:27:29 PST
(In reply to Michael Catanzaro from comment #2) > This should be reviewed by Yusuke, because I'm really not sure whether it's > necessary or not. If the RunLoop itself is guaranteed to have another ref on > the ScheduledTask, then this isn't needed. I don't think it is necessary. See L173. When calling ScheduledTask::fired(), RunLoop's code always has ref by `RefPtr<ScheduledTask>`. BTW, this problem is why I separate ScheduledTask from TimerBase IIRC :)
Michael Catanzaro
Comment 4 2018-02-01 06:51:20 PST
(In reply to Yusuke Suzuki from comment #3) > I don't think it is necessary. See L173. When calling > ScheduledTask::fired(), RunLoop's code always has ref by > `RefPtr<ScheduledTask>`. > BTW, this problem is why I separate ScheduledTask from TimerBase IIRC :) Good decision ;)
Note You need to log in before you can comment on or make changes to this bug.