Implement named access on the window object

[pylbrecht]

---

• [ ] ./mach build -d does not report any errors
• [ ] ./mach test-tidy does not report any errors
• [ ] These changes fix #27949

• [ ] There are tests for these changes

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/document.rs, components/script/script_module.rs, components/script/dom/eventtarget.rs, components/script/dom/htmlscriptelement.rs and 9 more
• @kichjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/document.rs, components/script/script_module.rs, components/script/dom/eventtarget.rs, components/script/dom/htmlscriptelement.rs and 9 more

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!
• These commits modify script code, but no tests are modified. Please consider adding a test!

[pylbrecht]

This is based on https://github.com/servo/servo/tree/named-getter.

I opened this PR to keep track of changes and related discussions (..and run CI at some point).

[pylbrecht]

The build currently fails (which I kind of expected for a WIP):

error[E0609]: no field `name_map` on type `&dom::window::Window`
    --> components/script/dom/window.rs:1483:29
     |
1483 |         let name_map = self.name_map.borrow();
     |                             ^^^^^^^^ unknown field
     |
     = note: available fields are: `globalscope`, `script_chan`, `task_manager`, `navigator`, `image_cache` ... and 56 others

error[E0609]: no field `id_map` on type `&dom::window::Window`
    --> components/script/dom/window.rs:1495:27
     |
1495 |         let id_map = self.id_map.borrow();
     |                           ^^^^^^ unknown field
     |
     = note: available fields are: `globalscope`, `script_chan`, `task_manager`, `navigator`, `image_cache` ... and 56 others

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0609`.
error: could not compile `script`.

I'm going to have a look at the related spec(s) to figure out what id_map and name_map are supposed to be.

[jdm]

I wonder if those should be referring to Document instead of Window (https://github.com/servo/servo/blob/3bf1085c72d1bf05507c4755709e9c25c6ca4e7f/components/script/dom/document.rs#L247-L248)?

[pylbrecht]

I implemented getters for id_map and name_map to see if the build succeeds, but that got me into scary territory:

error[E0133]: use of extern static is unsafe and requires unsafe function or block
   --> components/script/window_named_properties.rs:174:11
    |
174 |     cOps: &ProxyClassOps,
    |           ^^^^^^^^^^^^^^ use of extern static
    |
    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

error[E0133]: use of extern static is unsafe and requires unsafe function or block
   --> components/script/window_named_properties.rs:176:10
    |
176 |     ext: &ProxyClassExtension,
    |          ^^^^^^^^^^^^^^^^^^^^ use of extern static
    |
    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

error[E0133]: use of extern static is unsafe and requires unsafe function or block
   --> components/script/window_named_properties.rs:177:11
    |
177 |     oOps: &ProxyObjectOps,
    |           ^^^^^^^^^^^^^^^ use of extern static
    |
    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

error: aborting due to 3 previous errors

[jdm]

I implemented getters for id_map and name_map to see if the build succeeds, but that got me into scary territory:

error[E0133]: use of extern static is unsafe and requires unsafe function or block
   --> components/script/window_named_properties.rs:174:11
    |
174 |     cOps: &ProxyClassOps,
    |           ^^^^^^^^^^^^^^ use of extern static
    |
    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

error[E0133]: use of extern static is unsafe and requires unsafe function or block
   --> components/script/window_named_properties.rs:176:10
    |
176 |     ext: &ProxyClassExtension,
    |          ^^^^^^^^^^^^^^^^^^^^ use of extern static
    |
    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

error[E0133]: use of extern static is unsafe and requires unsafe function or block
   --> components/script/window_named_properties.rs:177:11
    |
177 |     oOps: &ProxyObjectOps,
    |           ^^^^^^^^^^^^^^^ use of extern static
    |
    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

error: aborting due to 3 previous errors

Luckily, all of those just require wrapping it in a unsafe like cOps: unsafe { &ProxyClassOps } (or possibly the surrounding struct value instead if that doesn't work).

[pylbrecht]

That did it, thanks!

Are there any related tests I can run? @jdm, or can you trigger a CI build to see what tests pass/fail?

// edit: Nevermind, I think some of these should do:

▶ find tests/wpt -name "*named*prop*"
tests/wpt/metadata/html/browsers/the-window-object/named-access-on-the-window-object/window-named-properties.html.ini
tests/wpt/metadata/WebIDL/ecmascript-binding/class-string-named-properties-object.window.js.ini
tests/wpt/web-platform-tests/html/browsers/the-window-object/named-access-on-the-window-object/window-named-properties.html
tests/wpt/web-platform-tests/WebIDL/ecmascript-binding/class-string-named-properties-object.window.js
tests/wpt/web-platform-tests/dom/collections/namednodemap-supported-property-names.html
tests/wpt/web-platform-tests/shadow-dom/untriaged/shadow-trees/upper-boundary-encapsulation/window-named-properties-002.html
tests/wpt/web-platform-tests/shadow-dom/untriaged/shadow-trees/upper-boundary-encapsulation/window-named-properties-001.html
tests/wpt/web-platform-tests/shadow-dom/untriaged/shadow-trees/upper-boundary-encapsulation/window-named-properties-003.html
tests/wpt/metadata-layout-2020/WebIDL/ecmascript-binding/class-string-named-properties-object.window.js.ini

[pylbrecht]

So, I started to work on this TODO:

// TODO: Handle the document-tree child browsing context name property set.

The spec for named-access-on-the-window-object states:

1. If window's browsing context is null, then return the empty list.

NamedGetter() returns Option<NonNull<JSObject>>, but I have no idea what DOM type (#[dom_struct]) corresponds to "empty list" here. I skimmed through the script::dom docs, but I feel I'm still missing some bits and pieces.

Can you help me out?

[pshaughn]

So, I started to work on this TODO:

// TODO: Handle the document-tree child browsing context name property set.

The spec for named-access-on-the-window-object states:

1. If window's browsing context is null, then return the empty list.

NamedGetter() returns Option<NonNull<JSObject>>, but I have no idea what DOM type (#[dom_struct]) corresponds to "empty list" here. I skimmed through the script::dom docs, but I feel I'm still missing some bits and pieces.

Can you help me out?

The document-tree child browsing context name property set isn't what you return to the caller, so you aren't wrapping it into a JSObject. The spec's invoking the concept of the document-tree child browsing context name property set just to have a check for whether something is in it; if it's an empty list, then the thing you're checking for isn't in it. I think you might never have to actually construct the list itself at all, actually!

The actual named getter is this part: https://heycam.github.io/webidl/#dfn-determine-the-value-of-a-named-property and it's just calling up to that part for the one test; the return in that part just returns back to the determine-the-value steps, not the caller.

[pylbrecht]

The document-tree child browsing context name property set isn't what you return to the caller, so you aren't wrapping it into a JSObject. The spec's invoking the concept of the document-tree child browsing context name property set just to have a check for whether something is in it; if it's an empty list, then the thing you're checking for isn't in it. I think you might never have to actually construct the list itself at all, actually!

The actual named getter is this part: https://heycam.github.io/webidl/#dfn-determine-the-value-of-a-named-property and it's just calling up to that part for the one test; the return in that part just returns back to the determine-the-value steps, not the caller.

Oh, that makes sense. Thanks a lot for clarifying! Couldn't see the forest for the trees..

[pylbrecht]

Let childBrowsingContexts be all document-tree child browsing contexts of window's browsing context

I know how to get the window's browsing context (document.browsing_context()), but I am not sure how to get its children. Is there a method to get all children by specifying its parent? Or is there another way to traverse the document tree?

// edit: Am I on the right track with Constellation::all_descendant_browsing_contexts_iter()? Further digging revealed Window::send_to_constellation(), but I couldn't find a ScriptMsg corresponding to all_descendant_browsing_contexts_iter(). Only GetChildBrowsingContextId, but that seems a bit tedious for getting all children of a browsing context.

[pshaughn]

Let childBrowsingContexts be all document-tree child browsing contexts of window's browsing context

I know how to get the window's browsing context (document.browsing_context()), but I am not sure how to get its children. Is there a method to get all children by specifying its parent? Or is there another way to traverse the document tree?

// edit: Am I on the right track with Constellation::all_descendant_browsing_contexts_iter()? Further digging revealed Window::send_to_constellation(), but I couldn't find a ScriptMsg corresponding to all_descendant_browsing_contexts_iter(). Only GetChildBrowsingContextId, but that seems a bit tedious for getting all children of a browsing context.

To be a "document-tree child browsing context", it has to be in the document tree, which means you already have a WindowProxy handy in the DOM. I think that's all you need to be able to figure out its name, and the name is really all you need here. I might be overlooking some corner case though.

[pylbrecht]

Now I'm confused, but that's probably due to a lot of missing domain knowledge. So my apologies if the following statements/questions make absolutely no sense. I'm just trying to make sense of the spec in combination with your comments, @pshaughn.

What I understand by reading the named access on the window object spec, is:

1. a window has a browsing context
2. browsing contexts may have multiple child browsing contexts

In order to get the names of all child browsing contexts, I somehow have to get all child browsing contexts in the first place, don't I? How to actually get these child browsing contexts is what I'm currently missing.

To be a "document-tree child browsing context", it has to be in the document tree, which means you already have a WindowProxy handy in the DOM.

I don't understand how that is helping to get the child browsing contexts of a window. Each browsing context has a corresponding WindowProxy. WindowProxy has methods to get the top level browsing context or the parent, but that only helps traversing up the tree.

I get the sense of me missing fundamental knowledge about the relations between windows, browsing contexts, and the document tree. Maybe you can help me fill in the blanks.

[pshaughn]

Now I'm confused, but that's probably due to a lot of missing domain knowledge. So my apologies if the following statements/questions make absolutely no sense. I'm just trying to make sense of the spec in combination with your comments, @pshaughn.

What I understand by reading the named access on the window object spec, is:

1. a window has a browsing context

2. browsing contexts may have multiple child browsing contexts

In order to get the names of all child browsing contexts, I somehow have to get all child browsing contexts in the first place, don't I? How to actually get these child browsing contexts is what I'm currently missing.

To be a "document-tree child browsing context", it has to be in the document tree, which means you already have a WindowProxy handy in the DOM.

I don't understand how that is helping to get the child browsing contexts of a window. Each browsing context has a corresponding WindowProxy. WindowProxy has methods to get the top level browsing context or the parent, but that only helps traversing up the tree.

I get the sense of me missing fundamental knowledge about the relations between windows, browsing contexts, and the document tree. Maybe you can help me fill in the blanks.

From what I recall, the only way you can get a child browsing context in servo is by having an iframe in the DOM. You don't need more distant descendant browsing contexts, just the immediate children of the current one, so you can just look at what iframes you have and ask what browsing contexts those are the containers of. Also, step 3 of https://html.spec.whatwg.org/multipage/window-object.html#named-access-on-the-window-object means you only care about the case where the browsing context's name is also the value of the "name" attribute of the iframe element, so if we already know what name we're testing for, you only need to look at iframes that have that name, not every iframe in the DOM tree.

[Belatedly I'm realizing this doesn't actually involve the iframe's WindowProxy, and my mentioning WindowProxies is probably what confused you, sorry about that]

[pylbrecht]

Thank you very much for elaborating! Now I have a good idea of how to approach this. And it turned out much simpler than I imagined it to be in the beginning.

[pshaughn]

Thank you very much for elaborating! Now I have a good idea of how to approach this. And it turned out much simpler than I imagined it to be in the beginning.

The spec phrasing about this is particularly awful. I'm glad I could help.

[pylbrecht]

Alright, so I feel I know what to do on an abstract level, but mapping it to code is quite a challenge for me.

So far I found two things that might be useful:

• Document.iter_iframes() to get all IFrames
• HTMLIFrameElement.browsing_context_id, which could be a gateway to the corresponding BrowsingContext

let child_browsing_context_ids = document.iter_iframes().map(|iframe| {
    iframe.browsing_context_id()
}).filter(|browsing_context_id| {
    browsing_context_id.is_some()
});

What I am missing:

• how to get a BrowsingContext by BrowsingContextId
• how to get the name of a BrowsingContext (and HTMLIFrameElement for that matter)

Are there any resources I can read up on regarding all these concepts (DOM, windows, IFrames, browsing contexts, etc.), which are easier to digest than the spec? I feel I'm missing the big picture to make sense of the details.

[pshaughn]

The name of an HTMLIFrameElement is just its name content attribute, like most nameable elements. I believe the name of a browsing context, in servo, is the name field of its associated WindowProxy, so you can call https://github.com/servo/servo/blob/master/components/script/dom/htmliframeelement.rs#L555-L559 and then ask the returned WindowProxy if any. My memory might be faulty here.

As for the question about resources, I can't point to any really good ones. One useful thing when stuck on a particular issue is to look at how other browsers do it; Servo takes after Firefox more often than not, and Firefox source code is sometimes better documented. It might also help to read web platform tests for concepts you're unclear on, to see how DOM concepts manifest as seen from the Javascript side.

[jdm]

https://github.com/servo/servo/blob/a955ff280d252468e22082beabcc9ca35577f3d9/components/script/script_thread.rs#L527 is a map of BrowsingContextId to WindowProxy, which I believe answers your first question!

[pylbrecht]

Thank you @pshaughn and @jdm for the hints and pointers!

[pylbrecht]

I just want to leave a sign of activity here.

Our son is currently giving us a hard time during the nights, so I'm mostly recharging in my free time, which means I'm not working as actively on this as I'd like to.

In case this issue becomes urgent, please feel free to assign this to someone else, who can work on this more regularly. Otherwise I'll of course keep hacking on this!

[jdm]

Not to worry! If this is still something that you'd like to keep hacking on, you're welcome to it!

[pylbrecht]

I'm back with unsuccessfully trying to build my most recent changes:

error[E0271]: type mismatch resolving `<std::iter::Filter<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [closure@components/script/do
m/window.rs:1447:21: 1447:70]> as std::iter::IntoIterator>::Item == dom::bindings::root::Root<dom::bindings::root::Dom<dom::htmliframeelement::HTMLIFrameEleme
nt>>`
    --> components/script/dom/window.rs:1467:41
     |
1467 |         let mut elements = iframes_iter.chain(name_iter).chain(id_iter);
     |                                         ^^^^^ expected reference, found struct `dom::bindings::root::Root`
     |
     = note: expected reference `&dom::bindings::root::Dom<dom::element::Element>`
                   found struct `dom::bindings::root::Root<dom::bindings::root::Dom<dom::htmliframeelement::HTMLIFrameElement>>`

error[E0599]: no method named `chain` found for struct `std::iter::Chain<std::iter::Filter<impl std::iter::Iterator, [closure@components/script/dom/window.rs:
1434:21: 1439:10 name:_]>, std::iter::Filter<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [closure@components/script/dom/window.rs:1
447:21: 1447:70]>>` in the current scope
    --> components/script/dom/window.rs:1467:58
     |
1467 |           let mut elements = iframes_iter.chain(name_iter).chain(id_iter);
     |                                                            ^^^^^ method not found in `std::iter::Chain<std::iter::Filter<impl std::iter::Iterator, [clo
sure@components/script/dom/window.rs:1434:21: 1439:10 name:_]>, std::iter::Filter<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [clos
ure@components/script/dom/window.rs:1447:21: 1447:70]>>`
     |
     = note: the method `chain` exists but the following trait bounds were not satisfied:
             `<std::iter::Filter<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [closure@components/script/dom/window.rs:1447:21: 1447
:70]> as std::iter::Iterator>::Item = dom::bindings::root::Root<dom::bindings::root::Dom<dom::htmliframeelement::HTMLIFrameElement>>`
             which is required by `std::iter::Chain<std::iter::Filter<impl std::iter::Iterator, [closure@components/script/dom/window.rs:1434:21: 1439:10 name
:_]>, std::iter::Filter<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [closure@components/script/dom/window.rs:1447:21: 1447:70]>>: s
td::iter::Iterator`
             `std::iter::Chain<std::iter::Filter<impl std::iter::Iterator, [closure@components/script/dom/window.rs:1434:21: 1439:10 name:_]>, std::iter::Filt
er<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [closure@components/script/dom/window.rs:1447:21: 1447:70]>>: std::iter::Iterator`
             which is required by `&mut std::iter::Chain<std::iter::Filter<impl std::iter::Iterator, [closure@components/script/dom/window.rs:1434:21: 1439:10
 name:_]>, std::iter::Filter<std::slice::Iter<'_, dom::bindings::root::Dom<dom::element::Element>>, [closure@components/script/dom/window.rs:1447:21: 1447:70]
>>: std::iter::Iterator`

I don't know how to turn a dom::bindings::root::Root<dom::bindings::root::Dom<dom::htmliframeelement::HTMLIFrameElement>> into a &dom::bindings::root::Dom<dom::element::Element> or if that is even the right approach for that matter.

Could you help me out?

[pshaughn]

I have to admit that this is a part of Servo's architecture I've only been able to work with through trial and error and asking for help. Whenever I've hit this kind of type error, the solution usually ended up being some combination of & and * operators and a call to .upcast<Element>(), but I never came away with a complete understanding of why a particular arrangement of those worked in a particular situation.

[jdm]

I'm pretty sure if you make iframes_iter:

        let iframes_iter = document
            .iter_iframes()
            .filter(|iframe| {
                if let Some(window) = iframe.GetContentWindow() {
                    return window.get_name() == name;
                }
                false
            })
            .map(|iframe| Root::upcast::<Element>(iframe))
            .map(|element| &**element);

that it will fix that problem. The goal here is to convert the Root<Dom<HTMLIFrameElement>> to a Root<Dom<Element>>, then use the fact that dereferencing a Root<T> returns a &T to get us the required &Dom<Element> that the chain expects.

[jdm]

I was wrong; it's a bit more subtle than that because dereferencing a Root<Dom<T>> returns &T instead. I'm tinkering with it locally.

[jdm]

This makes the build succeed for me. I originally attempted to unify the three iterators to be over &Element instead of two of them being over &Dom<Element>, but that exposed that I couldn't do the upcast for the iframe iterator because of Rust's borrow rules. It's valid to make a new stable collection from that iterator and upcast those elements, however, which gives us our three iterators over &Element as original desired.

diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs
index 7ef1b81dd2..bfaf604e4b 100644
--- a/components/script/dom/window.rs
+++ b/components/script/dom/window.rs
@@ -1400,25 +1400,32 @@ impl WindowMethods for Window {
         let document = self.Document();

         // https://html.spec.whatwg.org/multipage/#document-tree-child-browsing-context-name-property-set
-        let iframes_iter = document
+        let iframes: Vec<_> = document
             .iter_iframes()
             .filter(|iframe| {
                 if let Some(window) = iframe.GetContentWindow() {
                     return window.get_name() == name;
                 }
                 false
-            });
+            })
+            .collect();
+
+        let iframes_iter = iframes
+            .iter()
+            .map(|iframe| iframe.upcast::<Element>());

-        let name = Atom::from(name);
+        let name = Atom::from(&*name);

         // Step 1.
         let elements_with_name = document.get_elements_with_name(&name);
         let name_iter = elements_with_name
             .iter()
+            .map(|element| &**element)
             .filter(|elem| is_named_element_with_name_attribute(elem));
         let elements_with_id = document.get_elements_with_id(&name);
         let id_iter = elements_with_id
             .iter()
+            .map(|element| &**element)
             .filter(|elem| is_named_element_with_id_attribute(elem));

[pylbrecht]

I have to admit that this is a part of Servo's architecture I've only been able to work with through trial and error and asking for help.

Thanks, seeing someone else struggling with this is comforting.

@jdm, thanks for the elaborate explanation. It's not the first time I come across casting of DOM elements and it was a mystery to me. Seeing &**s now is starting to make sense.

[pylbrecht]

Since I have two weeks of vacation, I can finally spend some time on this again. Although this branch became so stale that rebasing on latest master became the next blocker.

Is it safe to always choose the latest version of a package, if Cargo.lock shows conflicts?