Bug 180285 - WebSocketChannel should ensure its client is live when calling it in error case
Summary: WebSocketChannel should ensure its client is live when calling it in error case
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: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-01 14:57 PST by youenn fablet
Modified: 2017-12-04 10:24 PST (History)
5 users (show)

See Also:

Attachments
Patch (1.61 KB, patch)
2017-12-01 14:58 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-12-01 14:57:23 PST 
WebSocketChannel should ensure its client is live when calling it in error case
Comment 1 youenn fablet 2017-12-01 14:58:29 PST 
Created attachment 328164 [details]
Patch
Comment 2 youenn fablet 2017-12-01 14:59:08 PST 
rdar://problem/32896043
Comment 3 Darin Adler 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?
Comment 4 youenn fablet 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.
Comment 5 WebKit Commit Bot 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>
Comment 6 WebKit Commit Bot 2017-12-03 21:40:53 PST 
All reviewed patches have been landed.  Closing bug.
Comment 7 Geoffrey Garen 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.
Comment 8 Geoffrey Garen 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()?
Comment 9 youenn fablet 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.