value_source_location() returns a bogus span for valueless attributes, and whether it does depends on chunk size

[krickert]

For an attribute written without a value, Attribute::value_source_location()
either returns None or returns a zero-length span pointing at the start of the
parser's current buffer. Which one you get depends on where the tag falls
relative to a write() boundary, so the same document parsed two ways gives two
different answers. name_source_location() is correct when present, but absent
in the None case.

Failing test

Drops into tests/ as-is, or into a fresh crate with lol_html = "3". Two
tests fail; the third is a control showing valued attributes are already fine.

use lol_html::{element, HtmlRewriter, Settings};
use std::cell::RefCell;
use std::ops::Range;
use std::rc::Rc;

/// `<!--pad-->` exists only to push the `<div>` off byte 0, which is what
/// makes the parser's buffer base non-zero for small chunk sizes.
///
/// In `PAD_THEN_BARE`, `hidden` occupies bytes 15..21.
const PAD_THEN_BARE: &[u8] = b"<!--pad--><div hidden></div>";
/// In `PAD_THEN_VALUED`, `id` occupies 15..17 and its value `x` occupies 19..20.
const PAD_THEN_VALUED: &[u8] = b"<!--pad--><div id=\"x\"></div>";

type Spans = Vec<(Option<Range<usize>>, Option<Range<usize>>)>;

/// Every (name span, value span) pair on the `<div>`, parsed in `chunk` byte writes.
fn spans(html: &[u8], chunk: usize) -> Spans {
    let collected = Rc::new(RefCell::new(Spans::new()));
    let sink = Rc::clone(&collected);

    let mut rewriter = HtmlRewriter::new(
        Settings::new().append_element_content_handler(element!("div", move |el| {
            sink.borrow_mut().extend(el.attributes().iter().map(|a| {
                (
                    a.name_source_location().map(|l| l.bytes()),
                    a.value_source_location().map(|l| l.bytes()),
                )
            }));
            Ok(())
        })),
        |_: &[u8]| {},
    );

    for part in html.chunks(chunk) {
        rewriter.write(part).unwrap();
    }
    rewriter.end().unwrap();

    let out = collected.borrow().clone();
    out
}

/// Source locations describe the input, so they cannot depend on how the
/// caller happened to split it.
#[test]
fn attribute_spans_do_not_depend_on_chunk_size() {
    let whole = spans(PAD_THEN_BARE, PAD_THEN_BARE.len());
    for chunk in [1, 4, 8, 16] {
        assert_eq!(
            spans(PAD_THEN_BARE, chunk),
            whole,
            "spans changed at chunk size {chunk}"
        );
    }
}

/// A value is written after its own name, so a value span starting before the
/// name ends is not pointing at a value.
#[test]
fn a_value_span_never_precedes_its_own_name() {
    for chunk in [1, 4, 8, 16, PAD_THEN_BARE.len()] {
        for (name, value) in spans(PAD_THEN_BARE, chunk) {
            if let (Some(name), Some(value)) = (name, value) {
                assert!(
                    value.start >= name.end,
                    "chunk size {chunk}: value span {value:?} starts before the end \
                     of its own name span {name:?}"
                );
            }
        }
    }
}

/// Control. An attribute that does have a value is already correct and already
/// chunk-independent, so the two tests above are about the valueless case only.
#[test]
fn a_valued_attribute_is_correct_and_chunk_independent() {
    let whole = spans(PAD_THEN_VALUED, PAD_THEN_VALUED.len());
    assert_eq!(whole.len(), 1);

    let (name, value) = whole[0].clone();
    assert_eq!(&PAD_THEN_VALUED[name.unwrap()], b"id");
    assert_eq!(&PAD_THEN_VALUED[value.unwrap()], b"x");

    for chunk in [1, 4, 8, 16] {
        assert_eq!(
            spans(PAD_THEN_VALUED, chunk),
            whole,
            "spans changed at chunk size {chunk}"
        );
    }
}

test a_valued_attribute_is_correct_and_chunk_independent ... ok
test attribute_spans_do_not_depend_on_chunk_size ... FAILED
test a_value_span_never_precedes_its_own_name ... FAILED

---- attribute_spans_do_not_depend_on_chunk_size ----
assertion `left == right` failed: spans changed at chunk size 1
  left: [(Some(15..21), Some(10..10))]
 right: [(None, None)]

---- a_value_span_never_precedes_its_own_name ----
chunk size 1: value span 10..10 starts before the end of its own name span 15..21

Byte 10 is where <div> starts. It has nothing to do with the attribute.

Cause

AttributeOutline::value is a Range defaulting to 0..0
(token_outline.rs:8). start_attr initialises the outline with that default
(actions.rs:297), and finish_attr_value (actions.rs:317) is its only
writer, so a valueless attribute keeps 0..0.

iter_attrs then infers "did this attribute have a value" from the offset being
nonzero (attributes.rs:290):

NonZero::new(base + a.value.start).map(|val| (base + a.name.start, val)),

With a.value.start == 0 that reduces to NonZero::new(base), so the result is
None only when the tag happens to sit at buffer offset 0, and otherwise a span
at base.

Suggested fix

Carry presence explicitly rather than deriving it from a nonzero offset, for
example AttributeOutline { value: Option<Range>, .. } set in
finish_attr_value. That would also let Attribute::name_value_start drop the
NonZero.

Versions

3.0.0 from crates.io, and current main (02f139c). Same three results on both.

Found while building a gRPC service over lol-html, by a test that replays each
document at several chunk sizes and asserts the emitted events are identical.