Bug 180285

Summary:

WebSocketChannel should ensure its client is live when calling it in error case

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

WebCore Misc.

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, commit-queue, darin, ggaren, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none

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	Details Formatted Diff Diff
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.