Skip to content

Fix reference tracking in ObjWriter::add_element - #117

Open
CUB3D wants to merge 6 commits into
masterfrom
fix_incref
Open

CUB3D wants to merge 6 commits into
masterfrom
fix_incref

Conversation

@CUB3D

@CUB3D CUB3D commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Description

When writing primitive values, we currently increment the reference value for some primitive types. We shouldn't do that. Lets also support CacheKeys for XML and Date kinds as well as they can be referenced. As long as add_element is only used for adding primitives this is correct, it would be ideal to enforce this somehow but I don't see a way to handle that while also not duplicating the logic of date() and xml() for each writer.
Also added tests for the XML and Date cases.

Lets check if the Value kind in add_element is something that should be reference-able instead.

Checklist

@CUB3D CUB3D changed the title Dont incref for primitives Remove inc_ref from ObjWriter::add_element Jul 14, 2026
@evilpie

evilpie commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Could we assert that the Value type is primitive?

@danielhjacobs

Copy link
Copy Markdown
Contributor

Thinking more about it, it feels like there may be a possibility that reference indexes may be used for XML Documents and Dates. I feel like that may have to be tested.

@evilpie

evilpie commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Thinking more about it, it feels like there may be a possibility that reference indexes may be used for XML Documents and Dates. I feel like that may have to be tested.

#72

@danielhjacobs

danielhjacobs commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Thinking more about it, it feels like there may be a possibility that reference indexes may be used for XML Documents and Dates. I feel like that may have to be tested.

#72

Well, that proves the Date case, XML Document shows this:

var payload = new Object();
payload.n_xml = new XML("<root><child id='avm1'>test</child></root>");
payload.n_xml_ref = payload.n_xml;
var nc = new NetConnection();
nc.connect("http://localhost:8000/");
var responder = new Object();
nc.call("test.avm1", responder, payload);

Flash output:

Navigator::fetch:
  URL: http://localhost:8000/
  Method: POST
  Mime-Type: application/x-amf
  Body: [00, 00, 00, 00, 00, 01, 00, 09, 74, 65, 73, 74, 2E, 61, 76, 6D, 31, 00, 02, 2F, 31, 00, 00, 00, 4D, 0A, 00, 00, 00, 01, 03, 00, 05, 6E, 5F, 78, 6D, 6C, 0F, 00, 00, 00, 2A, 3C, 72, 6F, 6F, 74, 3E, 3C, 63, 68, 69, 6C, 64, 20, 69, 64, 3D, 22, 61, 76, 6D, 31, 22, 3E, 74, 65, 73, 74, 3C, 2F, 63, 68, 69, 6C, 64, 3E, 3C, 2F, 72, 6F, 6F, 74, 3E, 00, 09, 6E, 5F, 78, 6D, 6C, 5F, 72, 65, 66, 07, 00, 02, 00, 00, 09]

@danielhjacobs

Copy link
Copy Markdown
Contributor

That said, this isn't correct in Ruffle currently either:

running 1 test
2026-07-28T18:52:38.084378Z ERROR ruffle_core::net_connection: Couldn't submit AMF Packet to http://localhost:8000/: FetchError("Could not get metadata for '/localhost': The file or directory could not be found")
test avm1/xml_ref ... FAILED

failures:

---- avm1/xml_ref ----
assertion failed: `(ruffle_actual == flash_expected)`

Diff < left / right > :
 Navigator::fetch:
   URL: http://localhost:8000/
   Method: POST
   Mime-Type: application/x-amf
<  Body: [00, 00, 00, 00, 00, 01, 00, 09, 74, 65, 73, 74, 2E, 61, 76, 6D, 31, 00, 02, 2F, 31, 00, 00, 00, 79, 0A, 00, 00, 00, 01, 03, 00, 05, 6E, 5F, 78, 6D, 6C, 0F, 00, 00, 00, 2A, 3C, 72, 6F, 6F, 74, 3E, 3C, 63, 68, 69, 6C, 64, 20, 69, 64, 3D, 22, 61, 76, 6D, 31, 22, 3E, 74, 65, 73, 74, 3C, 2F, 63, 68, 69, 6C, 64, 3E, 3C, 2F, 72, 6F, 6F, 74, 3E, 00, 09, 6E, 5F, 78, 6D, 6C, 5F, 72, 65, 66, 0F, 00, 00, 00, 2A, 3C, 72, 6F, 6F, 74, 3E, 3C, 63, 68, 69, 6C, 64, 20, 69, 64, 3D, 22, 61, 76, 6D, 31, 22, 3E, 74, 65, 73, 74, 3C, 2F, 63, 68, 69, 6C, 64, 3E, 3C, 2F, 72, 6F, 6F, 74, 3E, 00, 00, 09]
>  Body: [00, 00, 00, 00, 00, 01, 00, 09, 74, 65, 73, 74, 2E, 61, 76, 6D, 31, 00, 02, 2F, 31, 00, 00, 00, 4D, 0A, 00, 00, 00, 01, 03, 00, 05, 6E, 5F, 78, 6D, 6C, 0F, 00, 00, 00, 2A, 3C, 72, 6F, 6F, 74, 3E, 3C, 63, 68, 69, 6C, 64, 20, 69, 64, 3D, 22, 61, 76, 6D, 31, 22, 3E, 74, 65, 73, 74, 3C, 2F, 63, 68, 69, 6C, 64, 3E, 3C, 2F, 72, 6F, 6F, 74, 3E, 00, 09, 6E, 5F, 78, 6D, 6C, 5F, 72, 65, 66, 07, 00, 02, 00, 00, 09]

failures:
    avm1/xml_ref

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 5 filtered out; finished in 0.20s

error: test failed, to rerun pass `--test tests`
var payload = new Object();
payload.n_xml = new XML("<root><child id='avm1'>test</child></root>");
payload.n_xml_ref = payload.n_xml;
var nc = new NetConnection();
nc.connect("http://localhost:8000/");
var responder = new Object();
nc.call("test.avm1", responder, payload);

@danielhjacobs

Copy link
Copy Markdown
Contributor

#124

@CUB3D CUB3D changed the title Remove inc_ref from ObjWriter::add_element Fix reference tracking in ObjWriter::add_element Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AMF0 obj_writer increments its reference index for every property

3 participants