Foreign keys marked as "owned" when processed by DeserializeJson() --> wrong memorySize could be reported

[ewaldc]

• Problem: As of the latest version (versions ?) keys from external JSON strings are marked as owned (KEY_IS_OWNED) when using _deserializeJson() despite the fact that the key does not live in the memory pool. It's OK when adding items or constructing a JsonDocument from scratch in which case e.g. addMember is being used which uses slotSetKey() which implements TAdaptedString::storage_policy(). This may result in wrong memory use being reported and JsonDocument created from other JsonDocuments to be too large or too small.

• How to reproduce ? Example for array.cpp from DeserializeJson tests

  char jsonString[] =
        " [ { \"a\" : 1 , \"b\" : 2 } , { \"c\" : 3 , \"d\" : 4 } ] ";
  DeserializationError err = deserializeJson(doc, jsonString);
  std::cout << doc.memoryUsage() << " - " << doc.as<JsonVariant>().memoryUsage() << " - " << JSON_ARRAY_SIZE(1) << std::endl;

  Displays 192 - 200 - 32

• target platform: Windows 10 (x32/x64) and ESP 8266/32 tested

I don't know exactly at what version this started to go wrong, but I discovered it while testing my v5.x relocatable/compact port to 6.x. Somehow this was not captured by the very elaborate test suite (or at least I have not managed to get that far).

I found it while debugging a stack corruption after a failed deserialize yielding in "out-of-memory". The JsonDocument should have been properly sized but turned out to be too small.

PS. The test suite in "extra" is amazing, it helped me fix at least 30 defects in my port!

[bblanchon]

Hi @ewaldc,

It's been like that as far as I can remember. I knew this was incorrect, but it didn't have any impact at the time I wrote it. But you're right; it now causes an issue when we construct a JsonDocument from a JsonVariant.

This made me realize that we have a bigger issue. Now that strings are deduplicated, the result of JsonVariant::memorySize() can be significantly larger than JsonDocument::memorySize(). I really don't know how I can fix that without heap memory...

Best regards, Benoit

[ewaldc]

For the first issue there is a (simple) solution. For the second "issue", I am not really convinced this is an issue, at least in my simple mind:

• worst case it provides the same size as without dedup --> no extra memory is wasted compared to the base case
• the most likely goal of JsonVariant::memorySize() is IMHO to copy/create new document from it (unless I am missing an important use case). In that case there is no guarantee that your keys/ strings will have duplicates and thus that dedup will be able to save memory: [{"key":"value}, {"key:value}] with variant pointing to the second JsonObject.
• if true memory use is what you care about, it'sJsonDocument::memoryUsage() that matters (since the Variant is a reference to some part of it)

Hence, I think JsonVariant::memorySize() does the right thing to provide a "copy safe" size which might be a little larger than optimal. In that case, calling shrinkToFit() afterwards will do a perfect job to minimize memory usage.