| 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: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
youenn fablet
2017-12-01 14:57:23 PST
Created attachment 328164 [details]
Patch
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? (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 on attachment 328164 [details] Patch Clearing flags on attachment: 328164 Committed r225469: <https://trac.webkit.org/changeset/225469> All reviewed patches have been landed. Closing bug. 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. > 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()?
(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. |