[gnome-builder] xml-pack: various correctness and leak fixes
- From: Christian Hergert <chergert src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [gnome-builder] xml-pack: various correctness and leak fixes
- Date: Sat, 13 Jan 2018 03:13:51 +0000 (UTC)
commit 8a54185e65f09a43839907dee3dc3b5575bc9049
Author: Christian Hergert <chergert redhat com>
Date: Fri Jan 12 19:12:58 2018 -0800
xml-pack: various correctness and leak fixes
This fixes various reference leaks and correctness issues that could load
to crashes under some scenarios. Much of the async task ownership has been
changed to use the paterns we use else where in Builder.
src/plugins/xml-pack/ide-xml-tree-builder.c | 322 ++++++++++++++++------------
src/plugins/xml-pack/ide-xml-tree-builder.h | 2 -
2 files changed, 182 insertions(+), 142 deletions(-)
---
diff --git a/src/plugins/xml-pack/ide-xml-tree-builder.c b/src/plugins/xml-pack/ide-xml-tree-builder.c
index d4ea3bd69..37af9a951 100644
--- a/src/plugins/xml-pack/ide-xml-tree-builder.c
+++ b/src/plugins/xml-pack/ide-xml-tree-builder.c
@@ -25,14 +25,13 @@
#include "ide-xml-parser.h"
#include "ide-xml-rng-parser.h"
#include "ide-xml-sax.h"
+#include "ide-xml-schema-cache-entry.h"
#include "ide-xml-schema.h"
#include "ide-xml-service.h"
-#include "ide-xml-schema-cache-entry.h"
+#include "ide-xml-tree-builder.h"
#include "ide-xml-tree-builder-utils-private.h"
#include "ide-xml-validator.h"
-#include "ide-xml-tree-builder.h"
-
struct _IdeXmlTreeBuilder
{
IdeObject parent_instance;
@@ -49,38 +48,54 @@ typedef struct
gint64 sequence;
} TreeBuilderState;
+typedef struct
+{
+ IdeXmlTreeBuilder *self;
+ GTask *task;
+ GPtrArray *schemas;
+ guint index;
+} FetchSchemasState;
+
+static void tree_builder_state_free (TreeBuilderState *state);
+static void fetch_schema_state_free (FetchSchemasState *state);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC (FetchSchemasState, fetch_schema_state_free)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC (TreeBuilderState, tree_builder_state_free)
+
+G_DEFINE_TYPE (IdeXmlTreeBuilder, ide_xml_tree_builder, IDE_TYPE_OBJECT)
+
static void
tree_builder_state_free (TreeBuilderState *state)
{
+ g_assert (state != NULL);
+
g_clear_pointer (&state->content, g_bytes_unref);
g_clear_pointer (&state->analysis, ide_xml_analysis_unref);
g_clear_object (&state->file);
g_slice_free (TreeBuilderState, state);
}
-G_DEFINE_TYPE (IdeXmlTreeBuilder, ide_xml_tree_builder, IDE_TYPE_OBJECT)
-
static IdeDiagnostic *
-create_diagnostic (IdeContext *context,
- const gchar *msg,
- GFile *file,
- gint line,
- gint col,
- IdeDiagnosticSeverity severity)
+create_diagnostic (IdeContext *context,
+ const gchar *msg,
+ GFile *file,
+ gint line,
+ gint col,
+ IdeDiagnosticSeverity severity)
{
- IdeDiagnostic *diagnostic;
g_autoptr(IdeSourceLocation) loc = NULL;
g_autoptr(IdeFile) ifile = NULL;
+ g_assert (IDE_IS_CONTEXT (context));
+ g_assert (G_IS_FILE (file));
+
ifile = ide_file_new (context, file);
loc = ide_source_location_new (ifile,
line - 1,
col - 1,
0);
- diagnostic = ide_diagnostic_new (severity, msg, loc);
-
- return diagnostic;
+ return ide_diagnostic_new (severity, msg, loc);
}
static GBytes *
@@ -88,50 +103,39 @@ ide_xml_tree_builder_get_file_content (IdeXmlTreeBuilder *self,
GFile *file,
gint64 *sequence)
{
- IdeContext *context;
+ g_autoptr(GBytes) content = NULL;
IdeBufferManager *manager;
- IdeUnsavedFiles *unsaved_files;
- IdeUnsavedFile *uf;
+ IdeContext *context;
IdeBuffer *buffer;
gint64 sequence_tmp = -1;
- GBytes *content = NULL;
g_assert (IDE_IS_XML_TREE_BUILDER (self));
g_assert (G_IS_FILE (file));
context = ide_object_get_context (IDE_OBJECT (self));
manager = ide_context_get_buffer_manager (context);
+ buffer = ide_buffer_manager_find_buffer (manager, file);
- if (NULL != (buffer = ide_buffer_manager_find_buffer (manager, file)))
+ if (buffer != NULL)
{
content = ide_buffer_get_content (buffer);
-
- unsaved_files = ide_context_get_unsaved_files (context);
- if (NULL != (uf = ide_unsaved_files_get_unsaved_file (unsaved_files, file)))
- sequence_tmp = ide_unsaved_file_get_sequence (uf);
+ sequence_tmp = ide_buffer_get_change_count (buffer);
}
if (sequence != NULL)
*sequence = sequence_tmp;
- return content;
+ return g_steal_pointer (&content);
}
-typedef struct _FetchSchemasState
-{
- IdeXmlTreeBuilder *self;
- GTask *task;
- GPtrArray *schemas;
- guint index;
-} FetchSchemasState;
-
static void
fetch_schema_state_free (FetchSchemasState *state)
{
- g_clear_object (&state->self);
- g_clear_pointer (&state->schemas, g_ptr_array_unref);
- self->task = NULL;
+ g_assert (state != NULL);
+ g_clear_pointer (&state->schemas, g_ptr_array_unref);
+ g_clear_object (&state->self);
+ g_clear_object (&state->task);
g_slice_free (FetchSchemasState, state);
}
@@ -141,92 +145,114 @@ fetch_schemas_cb (GObject *object,
gpointer user_data)
{
DzlTaskCache *schemas_cache = (DzlTaskCache *)object;
- FetchSchemasState *state = (FetchSchemasState *)user_data;
- g_autoptr (IdeXmlSchemaCacheEntry) cache_entry = NULL;
- GTask *task = state->task;
- guint count;
+ g_autoptr(FetchSchemasState) state = user_data;
+ g_autoptr(IdeXmlSchemaCacheEntry) cache_entry = NULL;
IdeXmlSchemaCacheEntry *entry;
- GError *error = NULL;
+ g_autoptr(GError) error = NULL;
+ guint *count;
g_assert (DZL_IS_TASK_CACHE (schemas_cache));
- g_assert (G_IS_TASK (result));
- g_assert (G_IS_TASK (task));
+ g_assert (G_IS_ASYNC_RESULT (result));
+ g_assert (state != NULL);
+ g_assert (G_IS_TASK (state->task));
+ g_assert (IDE_IS_XML_TREE_BUILDER (state->self));
+ g_assert (state->schemas != NULL);
+ g_assert (state->index < state->schemas->len);
- cache_entry = dzl_task_cache_get_finish (schemas_cache, result, &error);
entry = g_ptr_array_index (state->schemas, state->index);
+ cache_entry = dzl_task_cache_get_finish (schemas_cache, result, &error);
- if (cache_entry->content != NULL)
- entry->content = g_bytes_ref (cache_entry->content);
+ if (cache_entry != NULL)
+ {
+ if (cache_entry->content != NULL &&
+ entry->content != cache_entry->content)
+ {
+ g_clear_pointer (&entry->content, g_bytes_unref);
+ entry->content = g_bytes_ref (cache_entry->content);
+ }
+
+ if (cache_entry->error_message != NULL &&
+ entry->error_message != cache_entry->error_message)
+ {
+ g_free (error->message);
+ entry->error_message = g_strdup (cache_entry->error_message);
+ }
- if (cache_entry->error_message != NULL)
- entry->error_message = g_strdup (cache_entry->error_message);
+ if (cache_entry->schema != NULL &&
+ entry->schema != cache_entry->schema)
+ {
+ g_clear_pointer (&entry->schema, ide_xml_schema_unref);
+ entry->schema = ide_xml_schema_ref (cache_entry->schema);
+ }
- if (cache_entry->schema != NULL)
- entry->schema = ide_xml_schema_ref (cache_entry->schema);
+ entry->state = cache_entry->state;
+ entry->mtime = cache_entry->mtime;
+ }
- entry->state = cache_entry->state;
- entry->mtime = cache_entry->mtime;
+ count = g_task_get_task_data (state->task);
+ g_assert (count != NULL);
- fetch_schema_state_free (state);
- count = GPOINTER_TO_UINT (g_task_get_task_data (task));
- --count;
- g_task_set_task_data (task, GUINT_TO_POINTER (count), NULL);
+ (*count)--;
- if (count == 0)
- {
- g_task_return_boolean (task, TRUE);
- g_object_unref (task);
- }
+ if (*count == 0)
+ g_task_return_boolean (state->task, TRUE);
}
-static gboolean
+static void
fetch_schemas_async (IdeXmlTreeBuilder *self,
GPtrArray *schemas,
GCancellable *cancellable,
GAsyncReadyCallback callback,
gpointer user_data)
{
- IdeContext *context;
+ g_autoptr(GTask) task = NULL;
+ g_autoptr(GPtrArray) schemas_copy = NULL;
IdeXmlService *service;
DzlTaskCache *schemas_cache;
- GTask *task;
- guint count = 0;
- gboolean has_external_schemas = FALSE;
+ IdeContext *context;
+ guint *count = 0;
g_assert (IDE_IS_XML_TREE_BUILDER (self));
g_assert (schemas != NULL);
g_assert (!cancellable || G_IS_CANCELLABLE (cancellable));
- if (schemas->len == 0)
- return FALSE;
-
/* TODO: use a worker thread */
task = g_task_new (self, cancellable, callback, user_data);
+ g_task_set_source_tag (task, fetch_schemas_async);
+ g_task_set_priority (task, G_PRIORITY_LOW);
+
+ count = g_new0 (guint, 1);
+ g_task_set_task_data (task, count, g_free);
+
+ /* Make a copy of schemas to ensure they cannot be changed
+ * during the lifetime of the operation, as the index within
+ * the array is stashed by the FetchSchemasState.
+ */
+ schemas_copy = g_ptr_array_new_with_free_func ((GDestroyNotify)ide_xml_schema_cache_entry_unref);
context = ide_object_get_context (IDE_OBJECT (self));
service = ide_context_get_service_typed (context, IDE_TYPE_XML_SERVICE);
schemas_cache = ide_xml_service_get_schemas_cache (service);
- for (gint i = 0; i < schemas->len; ++i)
+ for (guint i = 0; i < schemas->len; i++)
{
- IdeXmlSchemaCacheEntry *entry;
+ IdeXmlSchemaCacheEntry *entry = g_ptr_array_index (schemas, i);
FetchSchemasState *state;
- entry = g_ptr_array_index (schemas, i);
/* Check if it's an internal schema */
if (entry->file == NULL)
continue;
state = g_slice_new0 (FetchSchemasState);
state->self = g_object_ref (self);
- state->schemas = g_ptr_array_ref (schemas);
- state->task = task;
+ state->schemas = g_ptr_array_ref (schemas_copy);
+ state->task = g_object_ref (task);
+
+ (*count)++;
- ++count;
- g_task_set_task_data (task, GUINT_TO_POINTER (count), NULL);
- has_external_schemas = TRUE;
+ g_ptr_array_add (schemas_copy, ide_xml_schema_cache_entry_ref (entry));
+ state->index = schemas_copy->len - 1;
- state->index = i;
/* TODO: peek schemas if it's already in cache */
dzl_task_cache_get_async (schemas_cache,
entry->file,
@@ -236,10 +262,8 @@ fetch_schemas_async (IdeXmlTreeBuilder *self,
state);
}
- if (!has_external_schemas)
+ if (*count == 0)
g_task_return_boolean (task, TRUE);
-
- return TRUE;
}
static gboolean
@@ -269,7 +293,6 @@ ide_xml_tree_builder_parse_worker (GTask *task,
const gchar *doc_data;
xmlDoc *doc;
gsize doc_size;
- IdeXmlSchemaKind kind;
gint parser_flags;
g_assert (IDE_IS_XML_TREE_BUILDER (self));
@@ -281,46 +304,48 @@ ide_xml_tree_builder_parse_worker (GTask *task,
return;
state = g_task_get_task_data (task);
+ g_assert (state != NULL);
+ g_assert (state->analysis != NULL);
+
schemas = ide_xml_analysis_get_schemas (state->analysis);
+ g_assert (schemas != NULL);
+
context = ide_object_get_context (IDE_OBJECT (self));
+ g_assert (IDE_IS_CONTEXT (context));
xmlInitParser ();
doc_data = g_bytes_get_data (state->content, &doc_size);
parser_flags = XML_PARSE_RECOVER | XML_PARSE_NOERROR | XML_PARSE_NOWARNING | XML_PARSE_COMPACT;
- if (NULL != (doc = xmlReadMemory (doc_data,
- doc_size,
- NULL, NULL,
- parser_flags)))
+
+ doc = xmlReadMemory (doc_data, doc_size, NULL, NULL, parser_flags);
+
+ if (doc != NULL)
{
doc->URL = (guchar *)g_file_get_uri (state->file);
- for (gint i = 0; i < schemas->len; ++i)
+ for (guint i = 0; i < schemas->len; ++i)
{
- IdeXmlSchemaCacheEntry *entry;
+ IdeXmlSchemaCacheEntry *entry = g_ptr_array_index (schemas, i);
const gchar *schema_data;
gsize schema_size;
g_autoptr (IdeDiagnostics) diagnostics = NULL;
- g_autoptr (IdeDiagnostic) diagnostic = NULL;
- g_autofree gchar *uri = NULL;
- g_autofree gchar *msg = NULL;
gboolean schema_ret;
- entry = g_ptr_array_index (schemas, i);
- kind = entry->kind;
-
- if (kind == SCHEMA_KIND_RNG || kind == SCHEMA_KIND_XML_SCHEMA)
+ if (entry->kind == SCHEMA_KIND_RNG || entry->kind == SCHEMA_KIND_XML_SCHEMA)
{
if (entry->content != NULL)
{
schema_data = g_bytes_get_data (entry->content, &schema_size);
schema_ret = ide_xml_validator_set_schema (self->validator,
- kind,
+ entry->kind,
schema_data,
schema_size);
}
else
{
+ g_autoptr(IdeDiagnostic) diagnostic = NULL;
+
g_assert (entry->error_message != NULL);
diagnostic = create_diagnostic (context,
@@ -333,17 +358,22 @@ ide_xml_tree_builder_parse_worker (GTask *task,
continue;
}
}
- else if (kind == SCHEMA_KIND_DTD)
+ else if (entry->kind == SCHEMA_KIND_DTD)
{
- schema_ret = ide_xml_validator_set_schema (self->validator, SCHEMA_KIND_DTD, NULL, 0);
+ schema_ret = ide_xml_validator_set_schema (self->validator, SCHEMA_KIND_DTD, NULL, 0);
}
else
g_assert_not_reached ();
if (!schema_ret)
{
+ g_autoptr(IdeDiagnostic) diagnostic = NULL;
+ g_autofree gchar *uri = NULL;
+ g_autofree gchar *msg = NULL;
+
uri = g_file_get_uri (entry->file);
msg = g_strdup_printf ("Can't parse the schema: '%s'", uri);
+
diagnostic = create_diagnostic (context,
msg,
state->file,
@@ -356,6 +386,10 @@ ide_xml_tree_builder_parse_worker (GTask *task,
if (ide_xml_validator_validate (self->validator, doc, &diagnostics) != 0)
{
+ g_autoptr(IdeDiagnostic) diagnostic = NULL;
+ g_autofree gchar *uri = NULL;
+ g_autofree gchar *msg = NULL;
+
if (entry->file == NULL)
msg = g_strdup_printf ("Can't validate the internal schema");
else
@@ -384,7 +418,9 @@ ide_xml_tree_builder_parse_worker (GTask *task,
g_debug ("can't create xmlDoc\n");
}
- g_task_return_pointer (task, state->analysis, (GDestroyNotify)ide_xml_analysis_unref);
+ g_task_return_pointer (task,
+ g_steal_pointer (&state->analysis),
+ (GDestroyNotify)ide_xml_analysis_unref);
}
static void
@@ -392,23 +428,18 @@ ide_xml_tree_builder_build_tree_cb2 (GObject *object,
GAsyncResult *result,
gpointer user_data)
{
- g_autoptr(GTask) task = user_data;
+ IdeXmlTreeBuilder *self = (IdeXmlTreeBuilder *)object;
g_autoptr(GError) error = NULL;
- IdeXmlTreeBuilder *self;
-
- g_assert (G_IS_TASK (result));
- g_assert (G_IS_TASK (task));
+ g_autoptr(GTask) task = user_data;
- self = g_task_get_source_object (task);
g_assert (IDE_IS_XML_TREE_BUILDER (self));
+ g_assert (G_IS_ASYNC_RESULT (result));
+ g_assert (G_IS_TASK (task));
if (!fetch_schemas_finish (self, result, &error))
- {
- g_task_return_error (task, g_steal_pointer (&error));
- return;
- }
-
- g_task_run_in_thread (task, ide_xml_tree_builder_parse_worker);
+ g_task_return_error (task, g_steal_pointer (&error));
+ else
+ g_task_run_in_thread (task, ide_xml_tree_builder_parse_worker);
}
static void
@@ -416,37 +447,51 @@ ide_xml_tree_builder_build_tree_cb (GObject *object,
GAsyncResult *result,
gpointer user_data)
{
- IdeXmlTreeBuilder *self;
+ IdeXmlParser *parser = (IdeXmlParser *)object;
+ g_autoptr(IdeXmlAnalysis) analysis = NULL;
g_autoptr(GTask) task = user_data;
g_autoptr(GError) error = NULL;
+ IdeXmlTreeBuilder *self;
TreeBuilderState *state;
- IdeXmlAnalysis *analysis;
+ GCancellable *cancellable;
- g_assert (G_IS_TASK (result));
+ g_assert (IDE_IS_XML_PARSER (parser));
+ g_assert (G_IS_ASYNC_RESULT (result));
g_assert (G_IS_TASK (task));
self = g_task_get_source_object (task);
g_assert (IDE_IS_XML_TREE_BUILDER (self));
- if (NULL == (analysis = ide_xml_parser_get_analysis_finish (self->parser, result, &error)))
+ state = g_task_get_task_data (task);
+ g_assert (state != NULL);
+
+ cancellable = g_task_get_cancellable (task);
+ g_assert (!cancellable || G_IS_CANCELLABLE (cancellable));
+
+ analysis = ide_xml_parser_get_analysis_finish (self->parser, result, &error);
+
+ if (analysis == NULL)
{
g_task_return_error (task, g_steal_pointer (&error));
return;
}
- state = g_task_get_task_data (task);
state->analysis = ide_xml_analysis_ref (analysis);
ide_xml_analysis_set_sequence (analysis, state->sequence);
- if (analysis->schemas != NULL &&
- fetch_schemas_async (self,
- analysis->schemas,
- g_task_get_cancellable (task),
- ide_xml_tree_builder_build_tree_cb2,
- g_object_ref (task)))
- return;
+ if (analysis->schemas == NULL)
+ {
+ g_task_return_pointer (task,
+ g_steal_pointer (&analysis),
+ (GDestroyNotify)ide_xml_analysis_unref);
+ return;
+ }
- g_task_return_pointer (task, analysis, (GDestroyNotify)ide_xml_analysis_unref);
+ fetch_schemas_async (self,
+ analysis->schemas,
+ cancellable,
+ ide_xml_tree_builder_build_tree_cb2,
+ g_steal_pointer (&task));
}
void
@@ -457,9 +502,9 @@ ide_xml_tree_builder_build_tree_async (IdeXmlTreeBuilder *self,
gpointer user_data)
{
g_autoptr(GTask) task = NULL;
- TreeBuilderState *state;
- GBytes *content = NULL;
- gint64 sequence;
+ g_autoptr(TreeBuilderState) state = NULL;
+ g_autoptr(GBytes) content = NULL;
+ gint64 sequence = 0;
g_return_if_fail (IDE_IS_XML_TREE_BUILDER (self));
g_return_if_fail (G_IS_FILE (file));
@@ -467,8 +512,11 @@ ide_xml_tree_builder_build_tree_async (IdeXmlTreeBuilder *self,
task = g_task_new (self, cancellable, callback, user_data);
g_task_set_source_tag (task, ide_xml_tree_builder_build_tree_async);
+ g_task_set_priority (task, G_PRIORITY_LOW);
- if (NULL == (content = ide_xml_tree_builder_get_file_content (self, file, &sequence)))
+ content = ide_xml_tree_builder_get_file_content (self, file, &sequence);
+
+ if (content == NULL)
{
g_task_return_new_error (task,
G_IO_ERROR,
@@ -481,7 +529,10 @@ ide_xml_tree_builder_build_tree_async (IdeXmlTreeBuilder *self,
state->file = g_object_ref (file);
state->content = g_bytes_ref (content);
state->sequence = sequence;
- g_task_set_task_data (task, state, (GDestroyNotify)tree_builder_state_free);
+
+ g_task_set_task_data (task,
+ g_steal_pointer (&state),
+ (GDestroyNotify)tree_builder_state_free);
ide_xml_parser_get_analysis_async (self->parser,
file,
@@ -497,19 +548,10 @@ ide_xml_tree_builder_build_tree_finish (IdeXmlTreeBuilder *self,
GAsyncResult *result,
GError **error)
{
- GTask *task = (GTask *)result;
-
g_return_val_if_fail (IDE_IS_XML_TREE_BUILDER (self), NULL);
- g_return_val_if_fail (G_IS_TASK (task), NULL);
+ g_return_val_if_fail (G_IS_ASYNC_RESULT (result), NULL);
- return g_task_propagate_pointer (task, error);
-}
-
-IdeXmlTreeBuilder *
-ide_xml_tree_builder_new (DzlTaskCache *schemas)
-{
- return g_object_new (IDE_TYPE_XML_TREE_BUILDER,
- NULL);
+ return g_task_propagate_pointer (G_TASK (result), error);
}
static void
diff --git a/src/plugins/xml-pack/ide-xml-tree-builder.h b/src/plugins/xml-pack/ide-xml-tree-builder.h
index 812650ed9..57ba49323 100644
--- a/src/plugins/xml-pack/ide-xml-tree-builder.h
+++ b/src/plugins/xml-pack/ide-xml-tree-builder.h
@@ -18,7 +18,6 @@
#pragma once
-#include <glib-object.h>
#include <ide.h>
#include "ide-xml-analysis.h"
@@ -30,7 +29,6 @@ G_BEGIN_DECLS
G_DECLARE_FINAL_TYPE (IdeXmlTreeBuilder, ide_xml_tree_builder, IDE, XML_TREE_BUILDER, IdeObject)
-IdeXmlTreeBuilder *ide_xml_tree_builder_new (DzlTaskCache *schemas);
void ide_xml_tree_builder_build_tree_async (IdeXmlTreeBuilder *self,
GFile *file,
GCancellable *cancellable,
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
