RESOLVED FIXED 180285
WebSocketChannel should ensure its client is live when calling it in error case 
https://bugs.webkit.org/show_bug.cgi?id=180285
Summary WebSocketChannel should ensure its client is live when calling it in error case
youenn fablet
Reported 2017-12-01 14:57:23 PST
WebSocketChannel should ensure its client is live when calling it in error case
Attachments
Patch (1.61 KB, patch)
2017-12-01 14:58 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2017-12-01 14:58:29 PST
Created attachment 328164 [details] Patch
youenn fablet
Comment 2 2017-12-01 14:59:08 PST
rdar://problem/32896043
Darin Adler
Comment 3 2017-12-03 14:15:48 PST
Comment on attachment 328164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328164&action=review > Source/WebCore/ChangeLog:8 > + No observable change of behavior. That is strange. Can the pointer be null or not?
youenn fablet
Comment 4 2017-12-03 21:20:33 PST
(In reply to Darin Adler from comment #3) > Comment on attachment 328164 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328164&action=review > > > Source/WebCore/ChangeLog:8 > > + No observable change of behavior. > > That is strange. Can the pointer be null or not? I believe it does. What I mean by no observable change of behavior is that this will not remove any error event trigger at JS level.
WebKit Commit Bot
Comment 5 2017-12-03 21:40:51 PST
Comment on attachment 328164 [details] Patch Clearing flags on attachment: 328164 Committed r225469: <https://trac.webkit.org/changeset/225469>
WebKit Commit Bot
Comment 6 2017-12-03 21:40:53 PST
All reviewed patches have been landed. Closing bug.
Geoffrey Garen
Comment 7 2017-12-04 10:21:00 PST
Comment on attachment 328164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328164&action=review >>> Source/WebCore/ChangeLog:8 >>> + No observable change of behavior. >> >> That is strange. Can the pointer be null or not? > > I believe it does. > What I mean by no observable change of behavior is that this will not remove any error event trigger at JS level. I think a clearer statement here is that we believe this fixes a common crash seen in diagnostic logging. (That is an observable change in behavior.) But we don't have a reproducible test case for the crash. The pointer can be null because disconnect() and didCloseSocketStream() set it to null.
Geoffrey Garen
Comment 8 2017-12-04 10:21:32 PST
> I think a clearer statement here is that we believe this fixes a common > crash seen in diagnostic logging. (That is an observable change in > behavior.) But we don't have a reproducible test case for the crash. The > pointer can be null because disconnect() and didCloseSocketStream() set it > to null. ... Youenn, have you tried to write a test case that invokes disconnect() or didCloseSocketStream()?
youenn fablet
Comment 9 2017-12-04 10:24:00 PST
(In reply to Geoffrey Garen from comment #8) > > I think a clearer statement here is that we believe this fixes a common > > crash seen in diagnostic logging. (That is an observable change in > > behavior.) But we don't have a reproducible test case for the crash. The > > pointer can be null because disconnect() and didCloseSocketStream() set it > > to null. > > ... Youenn, have you tried to write a test case that invokes disconnect() or > didCloseSocketStream()? I haven't tried but will do.
Note You need to log in before you can comment on or make changes to this bug.