Implement StaticRange

[pshaughn]

StaticRange seems to be a "lightweight" range that just has a pointer-to-start and pointer-to-end, and no idea about its contents between those two pointers; the idea seems to be that if those are all your script needs, it can run a lot faster than a real Range that needs to stay aware of its actual contents. Since we already have real Ranges, implementing it is mostly boilerplate. Some code Servo has in Range now belongs in https://dom.spec.whatwg.org/#interface-abstractrange, and then StaticRange is a very simple interface that derives from AbstractRange by just holding cells for its readonly members and exposing the getters. Maybe this would make a good first step for someone who wants to start working with Servo's IDLs!

There are WPT tests in dom/ranges/StaticRange-constructor.html, but it's not a fully cross-browser API yet: https://wpt.fyi/results/dom/ranges/StaticRange-constructor.html?label=master&label=experimental&aligned

[nipunG314]

I would like to work on this if that's not a problem.

[jdm]

Not a problem! You will need to follow the steps at https://doc.servo.org/script/dom/index.html#adding-a-new-dom-interface for the new AbstractRange and StaticRange interfaces that come from https://dom.spec.whatwg.org/#interface-abstractrange and https://dom.spec.whatwg.org/#ref-for-abstractrange%E2%91%A1 respectively. You can test the effects of your changes with ./mach test-wpt tests/wpt/web-platform-tests/dom/ranges.

[nipunG314]

I've noticed that there are two Extended Attributes in the IDL code in components/script/dom/webidls/Range.webidl that are not present from the IDL code in the https://dom.spec.whatwg.org/#interface-range. Specifically, [Throws] and [Pure].

I also couldn't find them at https://heycam.github.io/webidl/. Are they specific to Servo? How will I know when to use them? I've noticed that the [Throws] Extended Attribute is used to define the constructors.

[pshaughn]

Information about Mozilla-specific WebIDL annotations can be found here: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

[pshaughn]

To summarize, and to translate from that page's C++ perspective to Rust: [Throws] means you're returning a Fallible or ErrorResult, which allows you to cover in Rust cases where the spec says to throw a Javascript exception. [Pure] is used by the Javascript compiler for optimizations; it means getting and setting the attribute is effectively "side-effect-free" in certain senses.

Direct links to those sections:

• https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Throws
• https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Pure

[nipunG314]

Alright, I'll keep those in mind. Thanks!

[nipunG314]

I have added the AbstractRange DOM interface on my fork. But I'm facing a problem. In the range.rs file, there is the WeakRangeVec struct that depends upon Range. Should I modify it to depend upon AbstractRange instead? Or should it only be used for live ranges?

In the new method of Range, WeakRefs are pushed into a WeakRangeVec owned by start_container and end_container. Are those weak references in the AbstractRange as well?

[pshaughn]

My reading of the spec might be wrong, but it looks to me like StaticRange holds strong refs to its start and end nodes and holds no weak refs, so there's no need for AbstractRange to have any interaction with weak refs.

[nipunG314]

I stashed my changes regarding StaticRange, so I could test the AbstractRange and Range interfaces. However, all of my tests crashed. All of them with the same error. This error is produced below.

  ▶ CRASH [expected OK] /dom/ranges/Range-attributes.html
  │ 
  │ gl function was not loaded (thread main, at C:\Users\Nipun\servo\target\debug\build\gleam-654ce9c6ee8eb034\out/gl_bindings.rs:1513)
  │ stack backtrace:
  │    0: backtrace::backtrace::trace_unsynchronized<closure-1>
  │              at C:\Users\Nipun\.cargo\git\checkouts\backtrace-rs-96ebaf1bcb788384\91a0aa4\src\backtrace\mod.rs:66
  │    1: servo::backtrace::{{impl}}::fmt
  │              at C:\Users\Nipun\servo\ports\glutin\backtrace.rs:49
  │    2: core::fmt::write
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libcore\fmt\mod.rs:1057
  │    3: std::io::stdio::{{impl}}::write_fmt
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libstd\io\stdio.rs:533
  │    4: std::io::stdio::_print
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libstd\io\stdio.rs:802
  │    5: servo::backtrace::print
  │              at C:\Users\Nipun\servo\ports\glutin\backtrace.rs:17
  │    6: servo::main::{{closure}}
  │              at C:\Users\Nipun\servo\ports\glutin\main2.rs:146
  │    7: std::panicking::rust_panic_with_hook
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libstd\panicking.rs:475
  │    8: std::panicking::begin_panic<str*>
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\src\libstd\panicking.rs:404
  │    9: gleam::ffi_gl::missing_fn_panic
  │              at C:\Users\Nipun\servo\target\debug\build\gleam-654ce9c6ee8eb034\out\gl_bindings.rs:1513
  │   10: gleam::ffi_gl::Gl::GetString
  │              at C:\Users\Nipun\servo\target\debug\build\gleam-654ce9c6ee8eb034\out\gl_bindings.rs:3960
  │   11: gleam::gl::{{impl}}::get_string
  │              at C:\Users\Nipun\.cargo\registry\src\github.com-1ecc6299db9ec823\gleam-0.6.18\src\gl_fns.rs:1757
  │   12: servo::headless_window::Window::new
  │              at C:\Users\Nipun\servo\ports\glutin\headless_window.rs:107
  │   13: servo::app::App::run
  │              at C:\Users\Nipun\servo\ports\glutin\app.rs:48
  │   14: servo::main
  │              at C:\Users\Nipun\servo\ports\glutin\main2.rs:174
  │   15: std::rt::lang_start::{{closure}}<()>
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\src\libstd\rt.rs:67
  │   16: std::panicking::try::do_call<closure-0,i32>
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libstd\panicking.rs:292
  │   17: panic_unwind::__rust_maybe_catch_panic
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libpanic_unwind\lib.rs:78
  │   18: std::rt::lang_start_internal
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\/src\libstd\rt.rs:51
  │   19: std::rt::lang_start<()>
  │              at /rustc/9b98af84c4aa66392236fff59c86da2130d46d46\src\libstd\rt.rs:67
  │   20: main
  │   21: __scrt_common_main_seh
  │              at d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
  │   22: BaseThreadInitThunk
  │   23: RtlUserThreadStart
  └ 

From what I can gather, the GL function GetString is missing. I also ran other tests to ensure that the crash wasn't caused due to my changes. Specifically, I ran the tests tests\wpt\web-platform-tests\dom\traversal and tests\wpt\web-platform-tests\dom\nodes. All those tests crashed as well with the same error.

I'm working on a 64-bit Windows 10 machine if that is relevant. Any ideas on what went wrong?

[jdm]

Oh, that happens because we don't support running headless Servo on Windows yet. You can work around it by running each test in the directory individually, rather than passing the directory as the argument to the test-wpt command. Sorry :/

[nipunG314]

The tests work fine now. Thanks a lot.

I was implementing the Constructor for StaticRange and I noticed that the WebIDL for the Attr interface in our code has a mistake. In the spec, Attr derives from Node. But the WebIDL in our code seems to have missed that.

components/script/dom/webidls/Attr.webidl

[Exposed=Window]
interface Attr {
  [Constant]
  readonly attribute DOMString? namespaceURI;

...

};

The DOM struct for Attr does not hold a Node value either like inherited DOM interfaces should. I need to check whether a given node is an Attr node while constructing the StaticRange, so this needs to be fixed before I can write its constructor.

Is there a particular reason for this? If not, I'll try and make it match the spec.

[jdm]

That's #25310.

[nipunG314]

I have added the StaticRange DOM interface on my local machine as well. Since #25310 hasn't been merged into master yet, I have chosen not to add the check for the Attr Node yet. Hence, it doesn't completely match the spec yet. When #25310 gets merged, I'll add the check for the Attr Node as well.

As far as WPT tests are concerned, all the Range-related WPT tests are giving expected results. In the case of the StaticRange-constructor.html test, things are a bit more complicated.

The BoundaryPoint struct has two debug_assert in its new() implementation, the 2nd of which crashes when the test is run. The assertion offset <= node.len() fails.

    fn new(node: &Node, offset: u32) -> BoundaryPoint {
        debug_assert!(!node.is_doctype());
        debug_assert!(offset <= node.len());
        BoundaryPoint {
            node: MutDom::new(node),
            offset: Cell::new(offset),
        }
    }

When I disabled the 2nd debug_assert and compiled, the StaticRange-constructor.html test worked. It gave UNEXPECTED-PASS to 16 of the 17 tests, and FAIL on the one test that depends upon Attr being a Node.

In order to find the problem, I diffed the tests in our repository and https://github.com/web-platform-tests/wpt. But there was no difference between them. So I decided to look through the test. I'm most likely reading this incorrectly, but I think the problem is with the test itself.

...
<div id='testDiv'>abc<span>def</span>ghi</div>
<script>
'use strict';

const testDiv = document.getElementById('testDiv');
const testTextNode = testDiv.firstChild;
const testPINode = document.createProcessingInstruction('foo', 'abc');
const testCommentNode =  document.createComment('abc');
document.body.append(testPINode, testCommentNode);

test(function() {
    const staticRange = new StaticRange({startContainer: testDiv, startOffset: 1, endContainer: testDiv, endOffset: 2});
    assert_equals(staticRange.startContainer, testDiv, 'valid startContainer');
    assert_equals(staticRange.startOffset, 1, 'valid startOffset');
    assert_equals(staticRange.endContainer, testDiv, 'valid endContainer');
    assert_equals(staticRange.endOffset, 2, 'valid endOffset');
    assert_false(staticRange.collapsed, 'not collapsed');
}, 'Construct static range with Element container');
...

The testDiv node has only child - the span node. So, its length should be 1. However, when the first StaticRange is constructed, testDiv is passed as the endContainer and the endOffset as 2. Since the endOffset is greater than the length of the endContainer, it fails the debug_assert.

I've never worked with WPT tests before, so I'm most likely making a mistake here. Any suggestions where I should look next to fix the assertion failure?

[pshaughn]

I'm not 100% sure, but I think it may be the case that a StaticRange is allowed to hold invalid data, unlike a real Range, so the debug_assert shouldn't apply to it.

[jdm]

Yes, it's totally possible to pass incorrect data to the constructor.

[nipunG314]

I see. In that case, I'll move that assert out of Boundary point and into the Range Constructor.

Are there any other tests that I need to satisfy or would this be sufficient for a PR?

[jdm]

As far as I know that's the only test for it.