jacobdufault
commented
6 years ago Thanks, should be fixed in 94a1a5c9d1911e919da3f5ab085fa3b938854505.
Open jpeach opened 6 years ago
jacobdufault
commented
6 years ago Thanks, should be fixed in 94a1a5c9d1911e919da3f5ab085fa3b938854505.
jpeach
commented
6 years ago @jacobdufault I still reproduce the same crash after this commit:
(lldb) down
frame #4: 0x000000010f738151 cquery`(anonymous namespace)::Handler_TextDocumentDefinition::Run(this=0x000000010fae2758, request=0x00007fdc4441d3a0)::In_TextDocumentDefinition*) at text_document_definition.cc:121
118 if (!has_symbol) {
119 lsPosition position = request->params.position;
120 const std::string& buffer = working_file->buffer_content;
-> 121 std::string_view query = LexIdentifierAroundPos(position, buffer);
122 std::string_view short_query = query;
123 {
124 auto pos = query.rfind(':');
(lldb) p working_file
(WorkingFile *) $2 = 0x0000000000000000
(lldb) p *request
((anonymous namespace)::In_TextDocumentDefinition) $4 = {
RequestInMessage = {
id = (type = kInt, value = 8)
}
params = {
textDocument = {
uri = (raw_uri_ = "file:///Users/jpeach/src/mesos.git/src/master/master.cpp")
}
position = (line = 40, character = 40)
}
}
(lldb) p *working_files
(WorkingFiles) $5 = {
files = size=0 {}
files_mutex = {
__m_ = (__sig = 1297437786, __opaque = char [56] @ 0x00007fc950fbb050)
}
}I don't think it is a race condition, since it seems related to the Position I send in the "textDocument/definition" request.
This request is OK:
Content-Length: 192
{"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///Users/jpeach/src/mesos.git/src/master/master.cpp"},"position":{"line":400,"character":40}},"id":12,"jsonrpc":"2.0"}This request crashes cquery:
Content-Length: 191
{"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///Users/jpeach/src/mesos.git/src/master/master.cpp"},"position":{"line":439,"character":9}},"id":16,"jsonrpc":"2.0"}Debugged some more. The reason some Positions don't crash is because, has_symbol in Handler_TextDocumentDefinition::Run ends up being true (the Position resolves as a header file). The reason that there are no files in WorkingFiles is that my client code never sends the "textDocument/didOpen" message.
jacobdufault
commented
6 years ago Yea, I think cquery should emit an error if the working file fails to resolve, since it is a client error. Leaving open as a TODO for that
jpeach
commented
6 years ago On May 24, 2018, at 8:55 PM, Jacob Dufault notifications@github.com wrote:
Yea, I think cquery should emit an error if the working file fails to resolve, since it is a client error. Leaving open as a TODO for that
Yes, that behavior makes sense to me. Is this really a client error though? I didn't see anything in the LSP spec that requires a client to sent an open notification for a file that it wants to query.
jacobdufault
commented
6 years ago I suppose you are right, didOpen/didClose is more about where the document content source of truth is.
Reproduced with a debug build of aa74ae57.
I'm playing with some toy client code that calls the "textDocument/definition". Under most circumstances, query crashes when it attempts to handle my query. Here's the sequence of input requests:
Request ID 3 succeeds (it's a hard-coded test call), but request ID 4 crashes cquery.
The crash happens because
working_fileis NULL, and we dereference it here.I guess that
working_fileis NULL becauseworking_filesis empty:I'm not really sure why
working_fileswould be empty depending on the requested position, but it seems like cquery should emit a diagnostic in this case.Here's the startup log in case that provides any clues: